forbid extra fields in BaseModel #44306
Conversation
rawwar
changed the title
rawwar
changed the title
There was a problem hiding this comment.
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
rawwar
force-pushed
the
kalyan/AIP-84/model_conf_extra_forbid
branch
from
7cf2095 to
9d36d40
Compare
| priority_weight_strategies: list[Any] | ||
| admin_views: list[Any] | ||
| menu_links: list[Any] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes I think Generic inside Generic are not well supported T[K[V]]
There was a problem hiding this comment.
This new fields shouldn't be returned by the API, they are not needed at the moment.
There was a problem hiding this comment.
We can either pop them manually before using the model (not great because we would need to extend that to every endpoint constructing a XXXXResponse object that has a richer dict as a source), or allow extra for XXXXResponse and only apply it to XXXXBody
rawwar
requested review from
bbovenzi,
ryanahamilton,
jscheffl and
ephraimbuddy
as code owners
There was a problem hiding this comment.
Taking a closer look, I think that there are two cases:
- Response models, those are instantiated by the server, there we want to be able to provide 'more' for the model to select only what should be returned. (This way we don't need to manually pop keys or anything else every time we are constructing a response)
- Payload / Body datamodels, those are constructed by the client and send to the application, they should be strict and not allow unknown field.
I would recommend creating a different baseclass for Bodies. BaseBody or something else, with the model_config = ConfigDict(from_attributes=True, extra="forbid") and use that for all body models.
| priority_weight_strategies: list[Any] | ||
| admin_views: list[Any] | ||
| menu_links: list[Any] |
There was a problem hiding this comment.
Yes I think Generic inside Generic are not well supported T[K[V]]
| priority_weight_strategies: list[Any] | ||
| admin_views: list[Any] | ||
| menu_links: list[Any] |
There was a problem hiding this comment.
This new fields shouldn't be returned by the API, they are not needed at the moment.
| priority_weight_strategies: list[Any] | ||
| admin_views: list[Any] | ||
| menu_links: list[Any] |
There was a problem hiding this comment.
We can either pop them manually before using the model (not great because we would need to extend that to every endpoint constructing a XXXXResponse object that has a richer dict as a source), or allow extra for XXXXResponse and only apply it to XXXXBody
|
Note: As PR #45312 has been merged, the code formatting rules have changed for new UI. Please rebase and re-run pre-commit checks to ensure that formatting in folder airflow/ui is adjusted. |
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?