-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
forbid extra fields in BaseModel #44306
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). |
7cf2095
to
9d36d40
Compare
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.
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.
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. |
@pierrejeambrun , while i was working on this, I noticed the following:
As the models can have an alias, this Example test:
Here, it always fails when extra=forbit with following error:
we can't use Also, Do we even need to have this specific check? |
The idea behind this check is to ensure that the given payload is a fully formed entity before moving further and saving that to the DB. Why we do that is because the endpoint can accept partial updates, when we give a partial I would say that we need to keep that, but we still need to figure out the other problem that you are mentioning. |
6bcbc40
to
2b36ae0
Compare
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.
Nice, thanks!
* use extra=forbid * fix backfill test * use Response model to fix plugin test failures * use BaseModel and StrictBaseModel * fix * update docstring
* use extra=forbid * fix backfill test * use Response model to fix plugin test failures * use BaseModel and StrictBaseModel * fix * update docstring
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?