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

ADX-960 foreign key validation #98

Closed

Conversation

ChasNelson1990
Copy link
Member

@ChasNelson1990 ChasNelson1990 commented May 12, 2023

Description

Please note that this is a local Fjelltopp PR for initial review before creating an upstream PR. This ensure that the solution works for us before we merge upstream.

See ADX-960 for our ticket.

See ckan#84 for the community discussion on this.

Basically, this is a rewrite of upstream to use the frictionless Package object, instead of the Resource object.

This allows us to then pass in multiple resources and do the following:

  • if "foreignKeys" is in the schema we collect together `schema["foreignKeys"][i]["reference"]["resource"]
  • we go through and check if the referenced resource is a url (path), json object (data) or (the fall back) is a resource name that can be found within this ckan dataset; if none are true then we return a ValidationError
  • these can be added to our Package object for frictionless framework to utilise during validation

!BREAKING CHANGE!

Actually, currently unaids_data_specifications uses schema names for schema["foreignKeys"][i]["reference"]["resource"] but this probably doesn't generalise as it very much assumes our fork of ckanext-scheming. Instead, for this PR, I am changing this to use what we call resource_type... I think this has the pro that we won't have to update the our table_schemas if we updates our package_schemas - foreign keys will now always follow the package_schema whereas (theroretically) you could have had a table_schema with a foreign key using a different schema version than a package_schema, which seems silly.

TODO

  • exploratory tests with our unaids_data_specifications
  • exploratory tests with vanilla ckan and upstream ckanext-scheming
  • run upstream test suite on vanilla ckan with upstream ckanext-scheming
  • automated tests (recommendations welcome)
  • documentation update
  • ADX-989

see also fjelltopp/adx_deploy#132
see also fjelltopp/unaids_data_specifications#58

Testing

TODO

Documentation

TODO

Checklist

  • The Jira ticket for this issue has been updated to "Ready to Review" or equivalent.
  • I have developed these changes in discussion with the appropriate project manager.
  • My code follows the general Fjelltopp documentation (see Confluence).
  • I have made corresponding changes to the Fjelltopp documentation (see Confluence).
  • I have rebased this branch with master.
  • New dependency changes have been committed.
  • I have added automated tests that prove my fix is effective or that my feature works.
  • New and existing tests pass locally with my changes.
  • My changes generate no new warnings.
  • I have performed a self-review of my own code.
  • I have assigned at least one reviewer.

@ChasNelson1990 ChasNelson1990 force-pushed the foreign-key-validation branch from 3f0eebe to 812eba6 Compare May 24, 2023 09:20
@ChasNelson1990 ChasNelson1990 force-pushed the foreign-key-validation branch 13 times, most recently from 169a8fc to 7b517b9 Compare July 21, 2023 16:45
@ChasNelson1990 ChasNelson1990 force-pushed the foreign-key-validation branch 2 times, most recently from 244b1c6 to 1645701 Compare July 21, 2023 18:05
@ChasNelson1990 ChasNelson1990 force-pushed the foreign-key-validation branch from 1645701 to 2ffa7e5 Compare July 21, 2023 18:13
@ChasNelson1990
Copy link
Member Author

made stale by #103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant