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.
| ) | ||
| 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.
There was a problem hiding this comment.
Where can I subscribe to see when this happens?
Apologies if this seems weird to respond to now, please let me know if I should turn this into a proper issue or some such for visibility.
I just spent a few hours adjusting to the v2.12 release, which broke our filter-system. In essence, we're creating lots of filters dynamically using a MetaClass and these filters have pydantic Fields. We usually just pass some extra information by setting
field.json_schema_extra = {"sqla_column": name}This used to work with previous versions, but now we need to add
field._attributes_set["json_schema_extra"] = {"sqla_column": name}to make the final pydantic.BaseModel.__new__() call recognize the json_schema_extra attribute.
So thanks for providing the fix in this comment :)
There was a problem hiding this comment.
Not really a specific issue for _attributes_set, but I gathered everything into #12374. To be help to give some feedback on your approach (and if extending our metaclass is the only possible option=, I would just require a small example of what your library achieves if possible!
There was a problem hiding this comment.
Thanks for your offer and sorry for the late reply! In the meantime, our library was refactored quite substantially, which eliminated our usage of _attributes_set :)
| 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 |
||||||||||||||||||||||||||||||||||||
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| } | ||
| ] | ||
|
|
||
| # 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) |
|
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)
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.