Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update custom field definition uniqueness criteria #3215

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented May 3, 2023

Partially closes https://github.com/ethyca/fidesplus/issues/826

Code Changes

  • create the proper unique (functional) index for the uniqueness criteria
    • include a migration that "handles" (i.e. reports with a cleaner error message) potentially invalid data on migration
    • migration also has some small unrelated cleanup on plus_custom_field_definition table since we're touching it
  • put in some more graceful handling of the unique constraint in the sqlalchemy model class
  • add in some test coverage directly on the sqlalchemy model

Steps to Confirm

  • any manual testing here would require hitting the DB directly
  • manual testing at the API level will come in corresponding fidesplus PR that has the API which exposes this model

Pre-Merge Checklist

Description Of Changes

We'll now ensure custom field definition uniqueness with a case insensitive name check, per resource_type (location). This implements that constraint on the DB level and also has some handling of the error to give a "cleaner" exception for consumption higher up in the stack.

@cypress
Copy link

cypress bot commented May 3, 2023

Passing run #1777 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge a6e2753 into f3d4631...
Project: fides Commit: 0486dbcdfa ℹ️
Status: Passed Duration: 01:02 💡
Started: May 5, 2023 11:40 AM Ended: May 5, 2023 11:41 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch coverage: 90.47% and project coverage change: -0.01 ⚠️

Comparison is base (f3d4631) 87.43% compared to head (a6e2753) 87.43%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3215      +/-   ##
==========================================
- Coverage   87.43%   87.43%   -0.01%     
==========================================
  Files         314      314              
  Lines       18322    18341      +19     
  Branches     2382     2384       +2     
==========================================
+ Hits        16020    16036      +16     
- Misses       1868     1869       +1     
- Partials      434      436       +2     
Impacted Files Coverage Δ
src/fides/api/ctl/sql_models.py 97.63% <89.47%> (-0.67%) ⬇️
src/fides/api/ctl/database/session.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

criteria is now case-insensitive name per resource type
@adamsachs adamsachs force-pushed the fidesplus-826-improve-handling-of-duplicate-values-for-custom-field-definition-names branch from 553235f to 61312cc Compare May 3, 2023 21:28
@adamsachs adamsachs marked this pull request as ready for review May 4, 2023 13:35
@adamsachs adamsachs requested a review from pattisdr May 4, 2023 13:35
@adamsachs
Copy link
Contributor Author

@pattisdr this is primarily a migration/db model update, so i'd appreciate a quick review when you have a chance!

(it's not a major change, and also not blocking any of my work currently).

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@adamsachs
Copy link
Contributor Author

thanks for the quick review @pattisdr! just merged in for changelog conflict resolution, will let CI run to completion and then merge into main

@adamsachs adamsachs merged commit dd7cfeb into main May 5, 2023
@adamsachs adamsachs deleted the fidesplus-826-improve-handling-of-duplicate-values-for-custom-field-definition-names branch May 5, 2023 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants