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

Remove RootedJsonData and revert to simpler validation #3822

Open
dafeder opened this issue Aug 23, 2022 · 1 comment
Open

Remove RootedJsonData and revert to simpler validation #3822

dafeder opened this issue Aug 23, 2022 · 1 comment
Milestone

Comments

@dafeder
Copy link
Member

dafeder commented Aug 23, 2022

We thought that having a dedicated constantly-self-validating data type for moving JSON metadata around in DKAN's PHP code would be valuable, but this doesn't seem to have been the case. It has added to the complexity of the application and still requires a lot of converting back and forth to other data types. It would be preferable to switch to moving either full MetastoreItem objects or simple stdClass objects around, and performing validations as needed. So:

  1. Change all RootedJsonData objects to stdClass objects in the code
  2. Perform JSON schema validations in all places in the code in which RootedJsonData objects were instantiated or modified directly, to ensure that we are not losing validation protection at any point in our workflows.
@dafeder dafeder added this to the 3.x milestone Aug 23, 2022
@github-actions github-actions bot added this to Incoming/Triage in DKAN 2 Issue Triage Aug 23, 2022
@dafeder
Copy link
Member Author

dafeder commented Aug 25, 2022

There are only a few points in the workflow where the metadata really needs to be validated, while there are many points in the workflow where metadata is expected to be temporarily invalid (for example, before it's been dereferenced) and we have to do extra work to bypass RootedJsonData's strict validation. We have places in the codebase where we expect JSON strings, others where we expect decoded stdClass objects, and others where we expect RootedJsonData objects. I think we should just use stdClass everywhere and convert to or from string only at the beginning or end of a workflow as needed.

There might be a case to made for using JsonObject from Galbar's JsonPath library, because it allows you use nested properties when you're doing dynamic property names. For instance, if we wanted to have a dynamic property name for "firstName" and in our schema it lives in a ContactInfo object under the main schema, we could have something like $firstName_property = "$.ContactInfo.firstName" with JsonPath. JsonPath is what we're using internally for data in RootedJsonData, but we're not actually using that kind of nested dynamic property name anywhere in DKAN as far as I can tell. And the trade-off would be that we still need to do a lot of conversion to work with any Symfony APIs or other external libraries that are expecting stdObject arguments for their methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
DKAN 2 Issue Triage
  
Incoming/Triage
Development

No branches or pull requests

1 participant