Repository navigation
Traverse class ancestors looking for rules to apply in JSON Schema output - #1805
Merged
Merged
Conversation
…tput These changes also clean up the way value_presence was being handled. It used to produce the non-standard construct `enum: []` to prohibit any value. This changes it to use an inverted required keyword.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1805 +/- ##
=======================================
Coverage 79.76% 79.76%
=======================================
Files 99 99
Lines 11370 11374 +4
Branches 2934 2935 +1
=======================================
+ Hits 9069 9073 +4
+ Misses 1751 1750 -1
- Partials 550 551 +1 ☔ View full report in Codecov by Sentry. |
cmungall
approved these changes
Jan 3, 2024
This was referenced Oct 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes might warrant a bit of discussion.
You can see a use case described in #1802. In that very simple scenario I think it seems intuitive that
MySubClasswould inherit therulesdefined onMyClass. However it becomes murky if bothMySubClassandMyClassdefinerules. Should it be additive and allrulesfrom both levels of the hierarchy apply? Should the subclassrulesreplace the superclassrules? Should a subclass be allowed to remove therulesof a superclass without adding any of its own? Or maybe all of this is going against the metamodel becauserulesis not marked withinherited: truein the first place?Separately these changes also clean up the way
value_presenceis translated into JSON Schema. Previously the generator emittedenum: []when it encounteredvalue_presence: ABSENT. It also emitted this when encounteringvalue_presence: PRESENT:Both of these are non-standard JSON Schema (
enumshould not be empty andrequiredmust be an array of strings), and a strict JSON Schema validator would not accept it. Admittedly, making fields required is one of the most annoying things about JSON Schema because it doesn't follow the same patterns as other keywords (likeminimumorpattern, for example). It also makes it really awkward to express "a value is not permitted" (a lavalue_presence: ABSENT). As far as I can tell the most standard way to do it looks like this:{ "properties": { "value_must_be_present": { "type": "string" }, "value_must_be_absent": { "type": "string" } }, "required": [ "value_must_be_present", ], "not": { "required": ["value_must_be_absent"] }, "title": "test", "type": "object" }The
"required": ["value_must_be_absent"]subschema evaluates to True if thevalue_must_be_absentfield has any value and wrapping that in anotkeyword inverts that. So it really is saying "no value is allowed" despite looking like it's saying it's simply not required. Again,requiredin JSON Schema is extremely unintuitive.In fleshing out some of those
requiredkeyword changes I tried adding some additional test cases to an existing test and realized we're not handling this one: #1803. That one could be a bit icky to figure out.I didn't realize at the outset that I was going to be making two somewhat unrelated sets of changes in this branch. If it makes sense to split out the
rulesstuff from thevalue_presencestuff I can do that.Fixes #1802