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

fides support for privacy declarations in datamap export #3184

Merged
merged 7 commits into from
Apr 28, 2023

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Apr 27, 2023

partially closes https://github.com/ethyca/fidesplus/issues/821

‼️ should be cherry-picked into 2.12.0 release branch after merging into main

Code Changes

  • include a filtered, bulk custom field retrieval method that allows us to get all of our custom fields in a single db query
    • this should remove one of the n+1 query patterns we have in this code path (there is still at least one more to retrieve our resources initially)
    • in refactoring, i also made sure we retrieve the custom fields on privacy declarations
  • update our export code to add the new privacy_declaration custom field data to the export table
  • fix a bug in the export code that added data_use and data_subject custom fields to the column header row, but did not populate their values in the data rows, which led to offset issues if someone had created a custom field definition on data_use or data_subject

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

Updates on the fides side to support exporting privacy_declaration (i.e. system:data_use) custom fields in the datamap export API. Requires integration with the fidesplus datamap export API code.

Also includes a bug fix where the datamap export got messed up when a data_use or data_subject custom field definition had been created.

@cypress
Copy link

cypress bot commented Apr 27, 2023

Passing run #1642 ↗︎

0 3 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 9e5d66c into 00ed06d...
Project: fides Commit: 5e1681838f ℹ️
Status: Passed Duration: 00:34 💡
Started: Apr 28, 2023 3:37 PM Ended: Apr 28, 2023 3:37 PM

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

@adamsachs adamsachs force-pushed the privacy-declarations-in-datamap-export-fides branch 3 times, most recently from 680f952 to eab3bdf Compare April 27, 2023 21:04
…roperly disregard taxonomy custom fields for now

eventually we may want to include taxonomy custom fields in the datamap export, but for now we will disregard them entirely.
previously, they'd been included in the column header row, but their values were not being included, which misaligned the export.
now, they will not be included in the column header row.

also some related cleanup of code to make it a bit easier to follow.
@adamsachs adamsachs force-pushed the privacy-declarations-in-datamap-export-fides branch from eab3bdf to 5fd1a22 Compare April 27, 2023 22:30
@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Patch coverage: 38.88% and project coverage change: -0.21 ⚠️

Comparison is base (cf3f547) 87.62% compared to head (f98ed2e) 87.41%.

❗ Current head f98ed2e differs from pull request most recent head 9e5d66c. Consider uploading reports for the commit 9e5d66c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3184      +/-   ##
==========================================
- Coverage   87.62%   87.41%   -0.21%     
==========================================
  Files         313      313              
  Lines       18178    18225      +47     
  Branches     2347     2360      +13     
==========================================
+ Hits        15928    15931       +3     
- Misses       1823     1863      +40     
- Partials      427      431       +4     
Impacted Files Coverage Δ
src/fides/core/export_helpers.py 81.56% <22.22%> (-14.21%) ⬇️
src/fides/core/export.py 74.04% <34.48%> (-7.38%) ⬇️
src/fides/api/ctl/database/crud.py 74.48% <75.00%> (-0.14%) ⬇️

... 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.

@adamsachs adamsachs marked this pull request as ready for review April 27, 2023 23:06
@adamsachs
Copy link
Contributor Author

adamsachs commented Apr 27, 2023

i threw too many reviewers on here just to increase visibility and hopefully someone has the time to look this over relatively quickly since there's a fidesplus PR dependent on it, both of which are needed for the upcoming release.

@SteveDMurphy i think you're most familiar with the export code at this point, maybe @ThomasLaPiana too? @TheAndrewJackson also included you since this relates to the current custom fields push and because i know you've also gotten dirty with the export code somewhat recently! @pattisdr i also included you on here since you've gotten more exposed to the custom fields world recently, and also because a decent chunk of this is a fun SQLalchemy query that you may have good insight on!

currently there are a fair number of codecov misses, a lot of which are explained by test coverage coming for this on the fidesplus side...we could try to add in coverage here, but since we want to move this code over to fidesplus soon anyway, i'm tempted to ignore them for now...

@ThomasLaPiana
Copy link
Contributor

@adamsachs I decided to go ahead and make some refactors here, but everything looks good to me from a logic standpoint

Copy link
Contributor

@ThomasLaPiana ThomasLaPiana left a comment

Choose a reason for hiding this comment

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

Approving with the assumption testing with the plus branch goes smoothly

@adamsachs
Copy link
Contributor Author

@adamsachs I decided to go ahead and make some refactors here, but everything looks good to me from a logic standpoint

thank you @ThomasLaPiana! appreciate being proactive. i'll look over your changes more closely, but generally they look great - appreciate the cleanup. i'll also go ahead and push a tag to give this an integrated test with fidesplus,

@adamsachs
Copy link
Contributor Author

@adamsachs I decided to go ahead and make some refactors here, but everything looks good to me from a logic standpoint

thank you @ThomasLaPiana! appreciate being proactive. i'll look over your changes more closely, but generally they look great - appreciate the cleanup. i'll also go ahead and push a tag to give this an integrated test with fidesplus,

@ThomasLaPiana - testing looked generally good on fidesplus but one minor typo that it identified, and i've addressed here (a good check to make sure the tests are doing their job!). i've pushed up a new tag and will go through another round of integration tests, should have results shortly and i think this will be ready to merge.

@SteveDMurphy @pattisdr any objections to me merging once i've tested this?

@adamsachs adamsachs merged commit 8e69d87 into main Apr 28, 2023
@adamsachs adamsachs deleted the privacy-declarations-in-datamap-export-fides branch April 28, 2023 15:55
adamsachs added a commit that referenced this pull request May 1, 2023
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