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

[Feature] Expand JSON $ref resolution #1061

Merged
merged 17 commits into from
Oct 29, 2024

Conversation

hudson-ai
Copy link
Collaborator

  1. Refs are now resolved in full generality, allowing anchors, relative refs, and absolute refs.
  2. Rather than pre-computing the grammars for schemas that happen to live in $defs, we now lazily compute grammars upon first reference.
  3. Existing ref tests are replaced with the full set of tests from the JSON Schema test suite. Some are xfailed if they depend on other features that we do not (yet) support.
  4. Keys in objects are now properly escaped in order to guarantee json.loads-ability (discovered this issue when running the new tests)

NOTE: this introduces a new dependency: referencing, which handles ref lookup for us (a fairly non-trivial task).

@hudson-ai hudson-ai requested a review from riedgar-ms October 24, 2024 00:44
@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.03%. Comparing base (77fc399) to head (a024812).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1061      +/-   ##
==========================================
- Coverage   65.33%   64.03%   -1.31%     
==========================================
  Files          63       63              
  Lines        4795     4810      +15     
==========================================
- Hits         3133     3080      -53     
- Misses       1662     1730      +68     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@riedgar-ms
Copy link
Collaborator

A couple of thoughts:

  1. Should we have an option to prevent remote references? That is a (small) potential security boundary
  2. Not for this PR, but does test_json.py need to become a json/ directory with multiple subfiles? It's getting to be quite a sizeable beast

@hudson-ai
Copy link
Collaborator Author

  1. Should we have an option to prevent remote references? That is a (small) potential security boundary

referencing doesn't seem to support remote references by default (although if the security concern is that this behavior will change in the future... fair enough). The API is such that you can do a bit of dependency injection to add this functionality if you desire. It might actually be nice to either expose this or to provide our own implementation for resolving remote refs (which would be turned off by default). See their docs here.

  1. Not for this PR, but does test_json.py need to become a json/ directory with multiple subfiles? It's getting to be quite a sizeable beast

Yes, absolutely agreed.

@hudson-ai
Copy link
Collaborator Author

@riedgar-ms they actually note the security concern in their docs

In the case of JSON Schema, the specifications generally discourage implementations from automatically retrieving these sorts of external resources over the network due to potential security implications. See schema-references in particular.
referencing will of course therefore not do any such thing automatically, and this section generally assumes that you have personally considered the security implications for your own use case.

@hudson-ai hudson-ai merged commit da80081 into guidance-ai:main Oct 29, 2024
100 checks passed
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.

3 participants