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

[Bug] [Feature] Fix issue #1039 by exposing And, Not regex operators from llguidance #1043

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

hudson-ai
Copy link
Collaborator

Fixes issue #1039 by ensuring that the keys for JSON properties validated against additionalProperties aren't allowed to match any of the keys for the properties themselves.

I'm still a bit unsure about the API of And and Not, as well as what the right type hierarchy is (e.g. should they be subclasses of RegularGrammar?), but I am comfortable with punting on those questions a bit as they are very much not part of the "public" API yet.

Comment on lines 497 to 508
names = set(properties.keys()) | set(required)
if len(names) > 0:
# If there are any properties, we need to disallow them as additionalProperties
key_grammar = as_regular_grammar(
And([
lexeme(r'"([^"\\]|\\["\\/bfnrt]|\\u[0-9a-fA-F]{4})*"'),
Not(lexeme('"(' + '|'.join(quote_regex(name) for name in names) + ')"')),
]),
lexeme = True,
)
else:
key_grammar = _gen_json_string()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the addition of new types and a bit of reorganization of _grammar.py, here's the meat of the bug fix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could put the quotes outside the whole thing and use Or over String. Maybe a bit cleaner

Comment on lines +1885 to +1893
def test_out_of_order_non_required_properties_not_validated_as_additionalProperties(self):
schema = {
"type": "object",
"properties": {"a": {"const": "foo"}, "b": {"const": "bar"}},
"required": ["b"],
}
test_string = '{"b": "bar", "a": "BAD"}'
grammar = gen_json(schema=schema)
assert grammar.match(test_string) is None
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for issue #1039 which fails on main

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2024

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

Codecov Report

Attention: Patch coverage is 96.61017% with 2 lines in your changes missing coverage. Please review.

Project coverage is 63.38%. Comparing base (63bfd9f) to head (1ae3b7b).

Files with missing lines Patch % Lines
guidance/_grammar.py 97.72% 1 Missing ⚠️
guidance/library/_subgrammar.py 83.33% 1 Missing ⚠️

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

❗ There is a different number of reports uploaded between BASE (63bfd9f) and HEAD (1ae3b7b). Click for more details.

HEAD has 69 uploads less than BASE
Flag BASE (63bfd9f) HEAD (1ae3b7b)
137 68
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
- Coverage   71.39%   63.38%   -8.02%     
==========================================
  Files          63       63              
  Lines        4670     4708      +38     
==========================================
- Hits         3334     2984     -350     
- Misses       1336     1724     +388     

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

Copy link
Collaborator

@mmoskal mmoskal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. The Or thing is a nitpick. Ignore.

@hudson-ai hudson-ai merged commit 8af45c1 into guidance-ai:main Oct 7, 2024
88 of 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