Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.2k
Fix JSON Schema generation with referenceable core schemas holding JSON metadata#11475
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ON metadataThis caused bugs in particular when using PEP 695 type aliases.
| @@ -128,89 +128,6 @@ def annotated_type(tp: Any, /) -> Any | None: | |||
| return get_args(tp)[0] if is_annotated(tp) else None | |||
| def unpack_annotated(annotation: Any, /) -> tuple[Any, list[Any]]: | |||
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.
This logic is not lost, it is present intyping-inspection and type aliases unpacking is configurable there
cloudflare-workers-and-pagesbot commentedFeb 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Deploying pydantic-docs with |
| Latest commit: | df48cc9 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a9ece144.pydantic-docs.pages.dev |
| Branch Preview URL: | https://no-type-alias-unpack.pydantic-docs.pages.dev |
e824efc to55d30b3CompareDeploying pydantic-docs with |
| Latest commit: | 55d30b3 |
| Status: | ⚡️ Build in progress... |
Deploying pydantic-docs with |
| Latest commit: | 55d30b3 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cd680f65.pydantic-docs.pages.dev |
| Branch Preview URL: | https://no-type-alias-unpack.pydantic-docs.pages.dev |
Coverage reportClick to see where and how coverage changed
This report was generated bypython-coverage-comment-action | ||||||||||||||||||||||||||||||||||||||||||
codspeed-hqbot commentedFeb 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#11475 willnot alter performanceComparing Summary
|
It's admittedly minor, but none of these commits attribute my changes (from#10913) to me. Would you mind folding my commits in or usinghttps://github.blog/news-insights/product-news/commit-together-with-co-authors/ |
@thejcannon yes, I'm planning on adding you as a co-author but this will happen when squash merging the PR |
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.
This is very much so in the typing weeds - I'm impressed with your understanding here.
Looking good generally, just left a few questions.
Uh oh!
There was an error while loading.Please reload this page.
| # Ideally, we should delegate all this to `_typing_extra.unpack_annotated`, e.g.: | ||
| # `typ, annotations = _typing_extra.unpack_annotated(annotated_type); schema = self.apply_annotations(...)` | ||
| # if it was able to use a `NsResolver`. But because `unpack_annotated` is also used | ||
| # when constructing `FieldInfo` instances (where we don't have access to a `NsResolver`), | ||
| # the implementation of the function does *not* resolve forward annotations. This could | ||
| # be solved by calling `unpack_annotated` directly inside `collect_model_fields`. | ||
| # For now, we at least resolve the annotated type if it is a forward ref, but note that | ||
| # unexpected results will happen if you have something like `Annotated[Alias, ...]` and | ||
| # `Alias` is a PEP 695 type alias containing forward references. |
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.
Are we giving up hope here?
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.
This is no longer an issue because we no longer eagerly unpack type aliases here. So all the concerns written here don't stand anymore, as if you have something like:
typeAlias=list['ForwardRef']GenerateSchema().generate_schema(Annotated[Alias, ...])
We previously tried to callunpack_annotated withAlias (and we couldn't resolve forward annotations insideunpack_annotated).
Now, We just leaveAlias as is, generate a schema for it (so it will go throughGenerateSchema._type_alias_type_schema(), and we can use the type namespace here if we have forward references), and apply the annotations after.
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.
Makes sense, thanks for clarifying!
| if _core_utils.is_core_schema(schema_or_field): | ||
| json_schema = populate_defs(schema_or_field, json_schema) | ||
| return json_schema | ||
| return js_modify_function(schema_or_field, current_handler) |
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.
Surprised we drop the populate_defs here - why is that?
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.
Forgot to add an explanation, but basically thisnew_handler_func is the last handler to be defined. It is the first one to be called, but by callingjs_modify_function, it will trigger the chain of calls to the inner handlers, meaning at this point (after L556, onmain), all the handlers have been called.
On L560 (on this branch), we also callpopulate_defs, meaning the now removed call topopulate_defs was redundant
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.
Approved, thanks for the clarifcations :)
67e4f59 intomainUh oh!
There was an error while loading.Please reload this page.
…ON metadata (#11475)This change reverts the previous solution which was to eagerly unpack type aliases during fields collection, so that in the event that those aliases where annotated forms, we could detect some field-specific metadata such as aliases or defaults.This broke the assumption that type aliases are referenceable and as such should be kept as is. An alternative fix provided by Joshua Cannon only affects JSON Schema generation and was used.The downside is that we don't support field-specific annotated metadata in such aliases, but this is now documented as expected (and generally makes sense).Co-authored-by: Joshua Cannon <3956745+thejcannon@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Change Summary
Fixes#11470,closes#11467.
Annotatedform #11109.Documentation was updated to warn about the valid usage of field metadata in PEP 695 type aliases. In the core schema generation process, we could warn/error when such metadata is encountered, but this is probably too disruptive for 2.11.
Related issue number
Checklist