Refactor FieldInfo creation implementation
#11898
Conversation
Required to fix a type checking issue
The existing implementation did not warn when a callable was used first, then a dict. Also use a plain `UserWarning` as `PydanticJsonSchemaWarning` is meant to be used *during* JSON Schema generation.
Both were actually inconsistent, in particular when using the `warnings.deprecated` class as metadata, we would only set the message string `FieldInfo.deprecated`.
We don't do any mutations of the assigned `Field()` anymore.
Viicos
added
relnotes-fix
| ) | ||
| default = assigned_value.default.__get__(None, cls) | ||
| assigned_value.default = default | ||
| assigned_value._attributes_set['default'] = default |
There was a problem hiding this comment.
Next step would be to get rid of the _attributes_set logic when merging instances, which is error prone as you need to update it whenever you do a manual assignment on a FieldInfo instance.
| @@ -213,7 +213,7 @@ def __init__(self, **kwargs: Unpack[_FieldInfoInputs]) -> None: | |||
|
|
|||
| See the signature of `pydantic.fields.Field` for more details about the expected arguments. | |||
| """ | |||
| self._attributes_set = {k: v for k, v in kwargs.items() if v is not _Unset} | |||
| self._attributes_set = {k: v for k, v in kwargs.items() if v is not _Unset and k not in self.metadata_lookup} | |||
There was a problem hiding this comment.
This is kind of important, although it also works without the change:
When we merge field infos, we don't want to reinclude kwargs that are transformed to metadata elements (gt -> annotated_types.Gt, etc). This will result in unnecessary metadata classes to be created (at the end of the FieldInfo.__init__() logic).
CodSpeed Performance ReportMerging #11898 will degrade performances by 13.42%Comparing Summary
Benchmarks breakdown
|
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
Viicos
added
the
needs-blogpost-entry
Deploying pydantic-docs with
|
| Latest commit: |
3ce5218
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://96843f8e.pydantic-docs.pages.dev |
| Branch Preview URL: | https://fieldinfo-cleanup.pydantic-docs.pages.dev |
| def _copy(self) -> Self: | ||
| """Return a copy of the `FieldInfo` instance.""" | ||
| # Note: we can't define a custom `__copy__()`, as `FieldInfo` is being subclassed | ||
| # by some third-party libraries with extra attributes defined (and as `FieldInfo` | ||
| # is slotted, we can't make a copy of the `__dict__`). |
There was a problem hiding this comment.
Alternatively, we can drop slots on FieldInfo, and do the following:
def __copy__(self) -> Self:
copied = type(self)()
copied.__dict__ = self.__dict__
for attr_name in ('metadata', '_attributes_set', '_qualifiers'):
# Apply "deep-copy" behavior on collections attributes:
value = getattr(self, attr_name).copy()
setattr(copied, attr_name, value)
return copied
| @@ -505,28 +505,6 @@ class AnnotatedFieldModel(BaseModel): | |||
| } | |||
| ] | |||
|
|
|||
| # Ensure that the inner annotation does not override the outer, even for metadata: | |||
There was a problem hiding this comment.
Due to one of the bugs being fixed in this PR (related to the usage of forward references), the constraint in Annotated was dropped. Now the test is failing because the unexpected order (described in #10507) applies.
| # HACK 2: FastAPI is subclassing `FieldInfo` and historically expected the actual | ||
| # instance's type to be preserved when constructing new models with its subclasses as assignments. | ||
| # This code is never reached by Pydantic itself, and in an ideal world this shouldn't be necessary. | ||
| if not metadata and isinstance(default, FieldInfo) and type(default) is not FieldInfo: | ||
| field_info = default._copy() | ||
| field_info._attributes_set.update(attr_overrides) | ||
| for k, v in attr_overrides.items(): | ||
| setattr(field_info, k, v) |
There was a problem hiding this comment.
Unfortunate stuff introduced since #6862..
|
cc @zmievsa, I've tried looking into the Cadwyn issues but got too scared by the code base. |
|
Apologies for not fixing it yesterday. I'll fix it in a second Update: oh, I see that it's unrelated to my recent problems with Cadwyn CI. Well, I'll fix it now anyways :) |
|
No rush, the third-party CI is not blocking and this is only going to be included in 2.12 anyway. |
|
@Viicos WDYT about this approach for extracting metadata? Is this the correct interface to use? (it works according to my tests but I am worried that I'm using the wrong abstraction from pydantic for extracting it |
…or type checkers)
pydantic-hooky
bot
added
the
awaiting author revision
Viicos
force-pushed
the
fieldinfo-cleanup
branch
from
a825961 to
35c4f95
Compare
Change Summary
(best reviewed commit per commit).
Fixes #11870, fixes #11876, fixes #11978.
Fixes #11122 (the three bugs described in the issue).
Issue #10507 isn't fixed with this, but at least now properly documented as a workaround in the implementation.
Most of the context of this PR is described in #11122.
The main thing being done here is the added
_construct()classmethod, that centralizes most of the merging logic and avoids repetition (which actually wasn't mirrored in every code path, that's why many inconsistencies existed depending on whether you assigned aField()to an attribute). We avoid copyingFieldInfoinstances everywhere, and doing buggy metadata handling.As a result, the
merge_field_infos()method is unused (you can compare both this method and_construct()to see what changed — the latter is much simpler). We need to deprecate it as third-party projects rely on it (unfortunately..). I propose marking it as deprecated only for type checkers for now, and have a runtime deprecation emitted in V3?openapi-python-client third party failure is unrelated, their CI is currently failing.