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.

Sign up for GitHub

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

Jump to bottom

JSON schema: fix extra parameter handling #7810

Merged
merged 3 commits into from
Oct 20, 2023
Merged

JSON schema: fix extra parameter handling #7810

merged 3 commits into from
Oct 20, 2023

Conversation

me-and
Copy link
Contributor

@me-and me-and commented Oct 12, 2023 •
edited by pydantic-hooky bot
Loading

Change Summary

The core schema for typed dicts uses the "extra_behavior" key to determine whether additional unspecified keys are allowed in the dict; update the JSON schema generation to expect that, rather than to look for the non-existent "extra" key.

Related issue number

Fixes #7809.

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @davidhewitt

@me-and me-and mentioned this pull request Oct 12, 2023
Closed
1 task
@me-and
Copy link
Contributor Author

me-and commented Oct 12, 2023

Hmm. The naive fix here seems to be broken by there being a field in ConfigDict that means the same thing but is currently called extra. Possibly the correct solution here is to check for both extra and extra_behavior, using whichever is present if there's only one, and with some prioritisation TBD if both are set.

@me-and
JSON schema: fix extra parameter handling
79b130e 
The core schema for typed dicts has an "extra_behavior" key for checking
whether additional unspecified keys are allowed, but the JSON schema
generator ignores this and only checks the configuration dict.  Have the
JSON schema generator check both places, and prefer the value set in the
core schema over the one in the config dictionary if both are present.

Fixes pydantic#7809.
@me-and me-and force-pushed the json-schema-typed-dict-extras branch from e144602 to 79b130e Compare October 12, 2023 10:10
@me-and
Copy link
Contributor Author

me-and commented Oct 12, 2023

Okay, I think that's good. Please review!

I've not managed to contrive a scenario where both the config value and the schema value are set, so that isn't included in the new tests, but the tests do cover checking extra_behavior in the schema.

@sydney-runkle sydney-runkle added the relnotes-fix Used for bugfixes. label Oct 13, 2023
sydney-runkle
sydney-runkle reviewed Oct 13, 2023
View reviewed changes
Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

LGTM at a first glance. @me-and, thanks so much for your contribution 🌟.

@davidhewitt, could you give this a review as well?

@sydney-runkle
Copy link
Member

@me-and,

The tests look good. Could you simplify the 3 tests you added a bit by using pytest.mark.parametrize on one test with the extra_behavior and the expected additionalProperties result in the resulting json schema? I think you should be able to use dict unpacking to achieve a pretty clean assert.

@sydney-runkle
Copy link
Member

Please update

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Oct 16, 2023
@pydantic-hooky pydantic-hooky bot assigned me-and and unassigned davidhewitt Oct 16, 2023
davidhewitt
davidhewitt reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, just one performance suggestion!

pydantic/json_schema.py Outdated Show resolved Hide resolved
@sydney-runkle @davidhewitt
Update pydantic/json_schema.py with faster check for extra_behavior
f1c12e9 
Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@sydney-runkle
Copy link
Member

@me-and,

The tests look good. Could you simplify the 3 tests you added a bit by using pytest.mark.parametrize on one test with the extra_behavior and the expected additionalProperties result in the resulting json schema? I think you should be able to use dict unpacking to achieve a pretty clean assert.

Looks fine for now. I see you modeled this after other tests that were already present.

@sydney-runkle sydney-runkle removed the awaiting author revision awaiting changes from the PR author label Oct 20, 2023
sydney-runkle
sydney-runkle approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution 🥳

@sydney-runkle sydney-runkle enabled auto-merge (squash) October 20, 2023 14:06
@sydney-runkle sydney-runkle merged commit 7cda00e into pydantic:main Oct 20, 2023
@me-and
Copy link
Contributor Author

me-and commented Oct 20, 2023

Ah, fantastic, got to it before I managed to. Thank you @sydney-runkle!

@sydney-runkle
Copy link
Member

@me-and,

Sure thing, thanks for the help! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt left review comments

@sydney-runkle sydney-runkle sydney-runkle approved these changes

Assignees

@me-and me-and

Labels
relnotes-fix Used for bugfixes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Typed dict JSON schema does not pay attention to extra_behavior argument
3 participants
@me-and @sydney-runkle @davidhewitt