model_json_schema export with Annotated types misses 'required' parameters#8793
Merged
sydney-runkle merged 3 commits intopydantic:mainfrom Feb 13, 2024
Merged
Conversation
CodSpeed Performance ReportMerging #8793 will not alter performanceComparing Summary
|
Contributor
Author
|
please review First open source contribution 🤞 |
sydney-runkle
suggested changes
Feb 13, 2024
Contributor
sydney-runkle
left a comment
There was a problem hiding this comment.
Looks great overall, just 2 quick change requests. Thanks!
|
|
||
|
|
||
| def test_json_schema_annotated_with_field() -> None: | ||
| """Check if the ellipsis in the signature is considered as a required field.""" |
Contributor
There was a problem hiding this comment.
Suggested change
| """Check if the ellipsis in the signature is considered as a required field.""" | |
| """Ensure field specified with Annotated in create_model call is still marked as required.""" |
|
|
||
| Model = create_model( | ||
| 'test_model', | ||
| bar=(Annotated[int, Field(description='Bar description')], ...), |
Contributor
There was a problem hiding this comment.
Could you include this example as well? baz=(Annotated[int, Field(..., description="Baz description")], ...),
Contributor
There was a problem hiding this comment.
Maybe with this model:
Model = create_model(
'test_model',
foo=(int, ...),
bar=(Annotated[int, Field(description="Bar description")], ...),
baz=(Annotated[int, Field(..., description="Baz description")], ...),
)
Contributor
|
So, with a slight modification to the logic in your PR, we can actually fix many related issues. My proposal is the following: Which fixes:
Would you be willing to add a test for the second issue as well? Thanks for your contribution! |
Contributor
Author
if field_info.default is not PydanticUndefined and default_override is not PydanticUndefined:
field_info.default = default_overrideI think this was the wrong proposal because if a field is initialized with a default = PydanticUndefined, the overide won't work 😄 My modification: if default_override is not PydanticUndefined:
field_info.default = default_override |
dmontagu
approved these changes
Feb 13, 2024
sydney-runkle
approved these changes
Feb 13, 2024
Contributor
|
Looks great, thanks for the help with the fix! |
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
3 participants
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.
Change Summary
When using an Annotation with a FieldInfo and an Ellipsis, the Ellipsis was considered as a default value. Therefore, the field was not considered mandatory.
Related issue number
fix #8780
fix #8634
Checklist
Selected Reviewer: @lig