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

Add DSR Concepts to Fideslang [#94] #95

Merged
merged 12 commits into from
Dec 13, 2022
Merged

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Dec 8, 2022

Closes https://github.com/ethyca/fideslang/issues/94

Code Changes

  • Move FidesDatasetReference, FidesMeta, FidesCollectionKey, and CollectionMeta, and EdgeDirection concepts from Fides -> Fideslang.
  • Rename Dataset.fidesctl_meta to Dataset.fides_meta - merging the ops and ctl concepts. Currently this just has the resource_id field and a new after field that allows you to specify the Datasets which should precede the current Dataset when running a DSR
  • Add DatasetCollection.fides_meta which currently only holds an after field, allowing you to specify which collections should run before the current collection in a DSR
  • Add fides_meta fields to DatasetField which is a FidesMeta resource. This has several attributes, including which fields the current field references or is referenced by, if the current field can be queried directly using identity data supplied for a DSR, whether the field is a primary key, a fides-supported datatype and other less common attributes.
    • Add additional DatasetField.fields validation for fields of object type, related to what data a DSR may return
    • Add additional fides_meta validation
  • Add a backwards compatible mixin that allows fidesops_meta to be specified on Datasets/Collections/Fields, and this is turned into a fides_meta attribute on instantiation
  • Have FidesValidationError be a subclass of ValueError instead of Exception so Pydantic can pick this up and raise ValidationErrors that are handled properly in FastAPI (note: this was one of the reasons why we opted to override FidesKey in Fidesops. See the "FidesOpsKey" concept in Fides.
  • Add "<", ">" to allowed keys in FidesKey (need approval that this is OK) - this is useful in the saas datasets where we want to specify fields in yaml that are eventually replaced with other text

Steps to Confirm

I think the best way to validate is to pull this fides branch down and QA it, which is specifically using this fideslang branch ethyca/fides#2009

  • First check out main. In the Admin UI, under connections http://localhost:3000/datastore-connection, create a Postgres Connection and a Saas connector like sentry. Verify in the db that their datasetconfig.datasets uses fidesops_meta. Run a privacy request
  • Still in main, also go to the Datasets tab and create a Dataset http://localhost:3000/dataset. Verify existence in ctl_datasets table
  • Now switch to Use New Fideslang DSR-updated models [#1761] ethyca/fides#2009 branch. View the previously created ops dataset Postgres configs and saas configs. View the previously created ctl dataset
  • Run another privacy request that should use the previously saved connection configs with fidesops_meta
  • Delete those connectors and create new ones - Verify in the db that their datasetconfig.datasets uses fides_meta. now
  • Run a privacy request, verify output is what is expected.
  • go to the Datasets tab and create another dataset http://localhost:3000/dataset. Verify existence in ctl_datasets table
  • Verify ctl_datasets table has fides_meta column instead of fidesctl_meta.
  • Spot check various cli commands

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Fidesops expanded Fideslang Dataset/Collection/Field concepts in its own repo primarily to add concepts to carry out DSR's. The most important of these are the DatasetField.fidesops_meta information which summarizes things like how collections are related to each other or if a collection is an entrypoint (what identity data we should start with to query the collection).

Move several DSR concepts from Fides back to Fideslang.

TheAndrewJackson and others added 10 commits November 16, 2022 12:15
This also included a few extra helper functions from fides. It's possible that those shouldn't live in `fideslang`. I'll look into that in the future. The main purpose of this commit is to get everything moved over.
…re defined before they are being used.

- Remove validation on data categories in last commit. This restricts valid data categories to just those in the default taxonomy.  Defer to caller to validate this how they wish.\
- Allow both fidesops_meta and fides_meta to be specified on Dataset/Collection/DatasetField for backwards-compatibility but have validator move fidesops_meta to fides_meta.
- Move validating data type code to fideslang/validation
- Update FidesKey validation to allow angle brackets.  There are several fides use cases where angle brackets might be in an initial dataset and then replaced with other text.
- Override FidesValidationError to be a subclass of ValueError not Exception so Pydantic can pick up and raise as a ValidationError.
- Add unit tests for core fidesops-concepts brought over to fideslang.
- Remove "allow" extra on Taxonomy config.  We can restore later if there's a valid use case.
- Add more tests for FidesCollectionKey and edge cases on "return_all_elements" and data categories being enforced at object level.
… avoid potential error "got multiple values for keyword argument".
@pattisdr pattisdr changed the title [WIP] Add DSR Concepts to Fideslang [#94] Add DSR Concepts to Fideslang [#94] Dec 9, 2022
@pattisdr pattisdr marked this pull request as ready for review December 9, 2022 15:33
@pattisdr pattisdr self-assigned this Dec 9, 2022
Copy link
Contributor

@SteveDMurphy SteveDMurphy left a comment

Choose a reason for hiding this comment

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

Added a couple of comments but installed and tested this in fides for validation purposes. The only thing that might be nice to do is see if there is a way to reduce or remove the use of Any if possible but not something that should hold this back

Also great job on the backwards compatibility addition!! 🔥 🔥

@pattisdr
Copy link
Contributor Author

thanks so much for your comments @SteveDMurphy 🏆

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

This is awesome! Thanks for taking over this work. Everything worked for me. I was unsure of exactly how the ops specific validations would split up between fideslang and fides. I like how everything worked out. I love the thorough tests for the fidesops_meta mixin 🚀

@ThomasLaPiana
Copy link
Contributor

these steps to confirm are 🔥 thank you for being so thorough @pattisdr

@pattisdr
Copy link
Contributor Author

Thanks so much for your comments @TheAndrewJackson and @ThomasLaPiana. I'll address today!

- Revert disallow_any_explicit change to True
- Make the descriptions of dsr additions to fideslang more generic
- Update fides_meta fields to use Field(description=...) syntax so it is picked up by automatically generated docs
- Update object fields validation to call out the name of the field in the error for easier debugging
- Make DATA_TYPE_NAMES all caps
@pattisdr
Copy link
Contributor Author

So I think we can go ahead and merge this! For fides to be able to use this, there are some slight tweaks needed but we are handling as part of the unified fides resources. Fides is pinned to a specific fideslang version so I think this should be fine.

@pattisdr pattisdr merged commit f8e66a2 into main Dec 13, 2022
@pattisdr pattisdr deleted the fides_lang_94_add_dsr_constructs branch December 13, 2022 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upstream DSR and connector related metadata from Fides
5 participants