-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
forbid extra fields in BaseModel #44306
base: main
Are you sure you want to change the base?
Conversation
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.
Yes I think it's a good idea.
If a user PATCH "satte"
instead of "state"
this will give him an explicit message instead of accepting the PATCH request and basically ignoring the field, resulting in a silent error for the user.
It's worth fixing test and getting it ready to merge 👍
Note: Add this to the breaking change of the API tracked here: (We do not create the newsfragments yet, format is not settled for those). |
…AIP-84/model_conf_extra_forbid
7cf2095
to
9d36d40
Compare
@@ -91,6 +92,13 @@ def file_token(self) -> str: | |||
serializer = URLSafeSerializer(conf.get_mandatory_value("webserver", "secret_key")) | |||
return serializer.dumps(self.fileloc) | |||
|
|||
@model_validator(mode="before") | |||
@classmethod | |||
def remove_file_token(cls, data): |
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.
@pierrejeambrun , is this the right way to do this?
priority_weight_strategies: list[Any] | ||
admin_views: list[Any] | ||
menu_links: 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.
I'm not sure what types to add here.
I tried to add list[type[PriorityWeightStrategy]] to priority_weight_strategies
. But, its throwing following error when generating openapi spec.
raceback (most recent call last):
File "/opt/airflow/scripts/in_container/run_update_fastapi_api_spec.py", line 38, in <module>
get_openapi(
File "/usr/local/lib/python3.9/site-packages/fastapi/openapi/utils.py", line 493, in get_openapi
field_mapping, definitions = get_definitions(
File "/usr/local/lib/python3.9/site-packages/fastapi/_compat.py", line 231, in get_definitions
field_mapping, definitions = schema_generator.generate_definitions(
File "/usr/local/lib/python3.9/site-packages/pydantic/json_schema.py", line 361, in generate_definitions
self.generate_inner(schema)
File "/usr/local/lib/python3.9/site-packages/pydantic/json_schema.py", line 441, in generate_inner
if 'ref' in schema:
File "/usr/local/lib/python3.9/_collections_abc.py", line 769, in __contains__
self[key]
File "/usr/local/lib/python3.9/site-packages/pydantic/_internal/_mock_val_ser.py", line 41, in __getitem__
return self._get_built().__getitem__(key)
File "/usr/local/lib/python3.9/site-packages/pydantic/_internal/_mock_val_ser.py", line 58, in _get_built
raise PydanticUserError(self._error_message, code=self._code)
pydantic.errors.PydanticUserError: `TypeAdapter[typing.Annotated[airflow.api_fastapi.core_api.datamodels.plugins.PluginCollectionResponse, FieldInfo(annotation=PluginCollectionResponse, required=True)]]` is not fully defined; you should define `typing.Annotated[airflow.api_fastapi.core_api.datamodels.plugins.PluginCollectionResponse, FieldInfo(annotation=PluginCollectionResponse, required=True)]` and all referenced types, then call `.rebuild()` on the instance.
As of now, we allow any fields to be in the Data Models. This PR ensures that requests have the exact required fields.
As of now, if we pass the wrong fields, the data models simply ignore them.
@pierrejeambrun , do you think this is a good idea?