-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Emit warning when field-specific metadata is used in invalid contexts #12028
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| def _warn_on_nested_alias_in_annotation(ann_type: type[Any], ann_name: str) -> None: | ||
| FieldInfo = import_cached_field_info() | ||
|
|
||
| args = getattr(ann_type, '__args__', None) | ||
| if args: | ||
| for anno_arg in args: | ||
| if typing_objects.is_annotated(get_origin(anno_arg)): | ||
| for anno_type_arg in _typing_extra.get_args(anno_arg): | ||
| if isinstance(anno_type_arg, FieldInfo) and anno_type_arg.alias is not None: | ||
| warnings.warn( | ||
| f'`alias` specification on field "{ann_name}" must be set on outermost annotation to take effect.', | ||
| UserWarning, | ||
| ) | ||
| return |
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 was fragile logic implemented a long time ago, that is now fully covered by the new warning.
| with warnings.catch_warnings(): | ||
| # We kind of abused `Field()` default factories to be able to specify | ||
| # the `defaultdict`'s `default_factory`. As a consequence, we get warnings | ||
| # as normally `FieldInfo.default_factory` is unsupported in the context where | ||
| # `Field()` is used and our only solution is to ignore them (note that this might | ||
| # wrongfully ignore valid warnings, e.g. if the `value_type` to a PEP 695 type alias |
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 referring to the following use case:
class Model(BaseModel):
a: defaultdict[int, Annotated[list[int], Field(default_factory=lambda: MyList())]]| # Because we pass `field` as metadata above (required for attributes relevant for | ||
| # JSON Scheme generation), we need to ignore the potential warnings about `FieldInfo` | ||
| # attributes that will not be used: | ||
| check_unsupported_field_info_attributes=False, |
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 for validated function calls (see test_unsupported_field_attribute_nested_with_function()). We don't want to raise a warning for:
@validate_call
def func(a: Annotated[int, Field(alias='b')]): ...
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||
CodSpeed Performance ReportMerging #12028 will not alter performanceComparing Summary
|
Deploying pydantic-docs with
|
| Latest commit: |
705951a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e6f68bfe.pydantic-docs.pages.dev |
| Branch Preview URL: | https://field-info-attr-warning.pydantic-docs.pages.dev |
59be012 to
6c868fa
Compare
91fc505 to
30acc55
Compare
|
I'm wondering if, similar to usage errors, we should link to a new documentation section about warnings, to show examples of this issue. |
|
@Viicos If we haven't documented yet that this is unsupported, I agree we should |
|
Well it is documented, but I was thinking about linking to the docs from the warning message, so that users don't get confused when they see it. |
|
@Viicos Ah I like that! |
Actually this is going to be a bit hard to implement as is, as we'll need to set up the same logic as for errors (that is, construct the URL from the current version, etc). I improved the warning message and we can revisit if users still get confused. |
b87d649 to
705951a
Compare
Best reviewed commit per commit.
In a nutshell, this now raises a warning:
We still need to figure out if we want the warning to be raised for
defaultanddefault_factory. Currently, the following works:This is explained by the fact that we need to support defaults in type adapters:
And as a consequence it accidentally worked for models as well. However, things are inconsistent:
and JSON Schema generation is also acting weirdly: #12024.
Imo, I think we should keep the warning for
defaultanddefault_factory. Sure, validation behavior still works, but it is a footgun for the reasons mentioned above. Users can still ignore the warning if they feel like it.I've reached out to the FastAPI team regarding the failing tests. They will need to address them in some way.