Skip to content
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

Merged

Conversation

rawwar
Copy link
Collaborator

@rawwar rawwar commented Nov 23, 2024

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?

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Nov 23, 2024
@rawwar rawwar changed the title forbids extra fields in DataModels forbids extra fields in BaseModel Nov 23, 2024
@rawwar rawwar changed the title forbids extra fields in BaseModel forbid extra fields in BaseModel Nov 23, 2024
pierrejeambrun
pierrejeambrun previously approved these changes Nov 25, 2024
Copy link
Member

@pierrejeambrun pierrejeambrun left a 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 👍

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Nov 25, 2024

Note: Add this to the breaking change of the API tracked here:
#43378

(We do not create the newsfragments yet, format is not settled for those).

@rawwar rawwar force-pushed the kalyan/AIP-84/model_conf_extra_forbid branch from 7cf2095 to 9d36d40 Compare November 27, 2024 16:47
@rawwar rawwar marked this pull request as ready for review November 27, 2024 19:32
@rawwar rawwar added the area:API Airflow's REST/HTTP API label Nov 29, 2024
Copy link
Member

@pierrejeambrun pierrejeambrun left a 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.

airflow/api_fastapi/core_api/datamodels/plugins.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/dags.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/plugins.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/dags.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/datamodels/plugins.py Outdated Show resolved Hide resolved
@jscheffl
Copy link
Contributor

jscheffl commented Jan 1, 2025

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
Copy link
Member

@rawwar any progress on that one ? #44986 also depends on that, I think it would be a great improvement for the API.

Let me know if you need some help.

@rawwar
Copy link
Collaborator Author

rawwar commented Jan 20, 2025

@rawwar any progress on that one ? #44986 also depends on that, I think it would be a great improvement for the API.

Let me know if you need some help.

No progress. I'll start working on this

@rawwar
Copy link
Collaborator Author

rawwar commented Jan 26, 2025

@pierrejeambrun , while i was working on this, I noticed the following:

ConnectionBody(**patch_body.model_dump())

As the models can have an alias, this ConnectionBody(**patch_body.model_dump()) will fail when there are alias. Especially when few are validation/serialization alias.

Example test:

{"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "extra": '{"key": "var"}'},

Here, it always fails when extra=forbit with following error:

'{"detail":[{"type":"extra_forbidden","loc":["schema_"],"msg":"Extra inputs are not permitted","input":null,"url":"https://errors.pydantic.dev/2.10/v/extra_forbidden"}]}'

we can't use ConnectionBody(**patch_body.model_dump(by_alias=True)) because, connection_id is a required field. Its transformed to conn_id because of serialization_alias

Also, Do we even need to have this specific check? ConnectionBody(**patch_body.model_dump()) feels redundant, given input is already validated during request.

@pierrejeambrun
Copy link
Member

Also, Do we even need to have this specific check? ConnectionBody(**patch_body.model_dump()) feels redundant, given input is already validated during request.

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 PathBody with a lot of None field and without specifying an update mask, we verify that this payload can be saved to the db.

I would say that we need to keep that, but we still need to figure out the other problem that you are mentioning. extra_forbidden and aliases should play nicely together.

@rawwar rawwar force-pushed the kalyan/AIP-84/model_conf_extra_forbid branch from 6bcbc40 to 2b36ae0 Compare January 29, 2025 17:21
@rawwar rawwar requested a review from pierrejeambrun January 29, 2025 19:07
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@pierrejeambrun pierrejeambrun merged commit f7bb606 into apache:main Jan 30, 2025
45 checks passed
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Jan 30, 2025
* use extra=forbid

* fix backfill test

* use Response model to fix plugin test failures

* use BaseModel and StrictBaseModel

* fix

* update docstring
niklasr22 pushed a commit to niklasr22/airflow that referenced this pull request Feb 8, 2025
* use extra=forbid

* fix backfill test

* use Response model to fix plugin test failures

* use BaseModel and StrictBaseModel

* fix

* update docstring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants