-
Notifications
You must be signed in to change notification settings - Fork 399
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
Fix invalid regex in unconstrained arrays for json_schema.py #919
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -719,6 +719,46 @@ def test_format(schema, regex, examples): | |
('{"time":20:20:39Z}', False), # missing quotes for value | ||
], | ||
), | ||
# Unconstrained Object | ||
( | ||
{ | ||
"title": "Foo", | ||
"type": "object", | ||
}, | ||
[ | ||
("{}", True), | ||
('{"a": 1, "b": null}', True), | ||
('{"a": {"z": {"g": 4}}, "b": null}', True), | ||
("1234", False), # not an object | ||
('["a", "a"]', False), # not an array | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lapp0 are we missing a test case for having arrays inside object? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the comments! It's important that we create patterns that follow json-schema spec expectations. I'll ensure the two conditions you've referenced are tested in the refactor here lapp0#36 |
||
], | ||
), | ||
# Unconstrained Array | ||
( | ||
{ | ||
"type": "array", | ||
}, | ||
[ | ||
("[1, {}, false]", True), | ||
("[{}]", True), | ||
('[{"a": {"z": "q"}, "b": null}]', True), | ||
('[{"a": [1, 2, true], "b": null}]', True), | ||
('[{"a": [1, 2, true], "b": {"a": "b"}}, 1, true, [1, [2]]]', True), | ||
# too deep, default unconstrained depth limit = 2 | ||
( | ||
'[{"a": [1, 2, true], "b": {"a": "b"}}, 1, true, [1, [2, [3]]]]', | ||
False, | ||
), | ||
('[{"a": {"z": {"g": 4}}, "b": null}]', False), | ||
("[[[[1]]]]", False), | ||
# not an array | ||
("{}", False), | ||
('{"a": 1, "b": null}', False), | ||
('{"a": {"z": {"g": 4}}, "b": null}', False), | ||
("1234", False), # not an array | ||
('{"a": "a"}', False), # not an array | ||
], | ||
), | ||
], | ||
) | ||
def test_format_without_regex(schema, examples): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lapp0 - Right now the array will have at least one element. I think there is missing a parentheses to denote that we want the entire array to be optional and not just the elements which start with
,
i.e.Note the starting bracket after
rf"\[
and closing bracket before{allow_empty}
.Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored a lot of this code so we can do constrained yaml. It's a lot more liberal with parenthesis use. Do you think this makes sense to fix the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didnt get you. I just meant that right now the regex is this:
and I think it should be this:
For e.g its this
[(true|false)(,(true|false)){0,}?]
vs[((true|false)(,(true|false)){0,})?]
. The former wont allow[]
whereas the latter will allow itDoes this make sense?