[Feature] Allow oneOf in JSON schemas (with limited support)
#982
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #982 +/- ##
===========================================
+ Coverage 44.74% 61.23% +16.49%
===========================================
Files 62 62
Lines 4392 4401 +9
===========================================
+ Hits 1965 2695 +730
+ Misses 2427 1706 -721 ☔ View full report in Codecov by Sentry. |
|
As a side note, we can at least have our tests check that the correct warning was issued, but doing this does still make me a bit queasy. |
| with pytest.warns() as record: | ||
| generate_and_check(target_obj, schema_obj) | ||
| assert len(record) == 1 | ||
| assert record[0].message.args[0].startswith("oneOf not fully supported") |
There was a problem hiding this comment.
Thanks for including this check (wrote my 'main' comment before seeing the code).
| @@ -55,12 +57,14 @@ class Keyword(str, Enum): | |||
| IGNORED_KEYS = { | |||
| "$schema", | |||
| "$id", | |||
| "id", | |||
There was a problem hiding this comment.
I believe that our problem with id (in the FHIR schema) is only at top level. I think that id is a perfectly fine name for a property on an object.
There was a problem hiding this comment.
validate_json_node_keys is only being called on dictionaries that represent full (sub)schemas, which doesn't include the dictionary specified by the properties key of a (sub)schema. Therefore the set of ignored keys should have no impact on what property names are valid -- worth checking if you have doubts :)
There was a problem hiding this comment.
I'd suggest adding a test with an object with an id property.
There was a problem hiding this comment.
Happy to do so :)
guidance/library/_json.py
Outdated
| "$comment", | ||
| "title", | ||
| "description", | ||
| "default", | ||
| "examples", | ||
| "required", # TODO: implement and remove from ignored list | ||
| "discriminator", # TODO: alternatively we could implement this in a stateful way |
There was a problem hiding this comment.
MIght want to add a link to:
https://json-schema.org/blog/posts/validating-openapi-and-json-schema
which mentions how discriminator is part of the OpenAPI dialect of JSON-Schema.
There was a problem hiding this comment.
Done. Additionally added a longer comment about how disambiguating the grammar on this field could possibly improve performance or quality.
There was a problem hiding this comment.
If you have a discriminator, oneOf is equivalent to anyOf (since options are exclusive).
There was a problem hiding this comment.
Nice observation and lets us more selectively warn the user. Might have to check that the discriminator is actually unique, but otoh maybe it's just up to the user to actually give us a valid schema...
| @@ -1981,3 +1981,13 @@ def test_no_additionalProperties(self, compact): | |||
| maybe_whitespace=True, | |||
| compact=compact, | |||
| ) | |||
|
|
|||
| def test_ignored_keys_allowed_as_properties(): | |||
There was a problem hiding this comment.
@riedgar-ms is this test acceptable to you here? It doesn't explicitly test id; rather it checks against all ignored keys. Happy to specialize it to id if you like that better.
There was a problem hiding this comment.
Even better to check them all
oneOfis allowed when only a single schema is provided in theoneOflistanyOfif multiple schemas are provided, raising a warning to the user.oneOfasanyOfwill not cause any problems in a majority of use-cases. @Harsha-Nori do you have any opinions here?idanddiscriminatorto ignored keys, expanding the schemas we support.oneOfanddiscriminatorwhen converted to a JSON schema.