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 on execution api #44986

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jx2lee
Copy link
Contributor

@jx2lee jx2lee commented Dec 17, 2024

closes: #44978
related: #44306


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@jx2lee jx2lee force-pushed the extra-field-execution branch from 65215a4 to 0419c2b Compare December 17, 2024 15:45
@jx2lee jx2lee changed the title Add strict validation to FastAPI response models Add extra-forbid validation to FastAPI request/response models Dec 18, 2024
@jx2lee jx2lee force-pushed the extra-field-execution branch from 0419c2b to 8f8fe21 Compare December 18, 2024 12:10
@jx2lee jx2lee force-pushed the extra-field-execution branch from f3f92f6 to 823a6f0 Compare December 19, 2024 00:19
@jx2lee jx2lee force-pushed the extra-field-execution branch from a0ae953 to ac39ba4 Compare December 19, 2024 01:54
@jx2lee jx2lee marked this pull request as ready for review December 19, 2024 01:54
@jx2lee
Copy link
Contributor Author

jx2lee commented Dec 19, 2024

Adding extra field in xcoms/variable datamodels, test passed.. so I added only task_instance test. Do i miss anything?
e.g in, TestGetVariable.test_variable_get_from_db, request with params extra fields but test passed (expected this test to fail, but...😭)

    def test_variable_get_from_db(self, client, session):
        Variable.set(key="var1", value="value", session=session)
        session.commit()

        response = client.get("/execution/variables/var1", params={"foo": "bar"})

        assert response.status_code == 200
        assert response.json() == {"key": "var1", "value": "value"}

        # Remove connection
        Variable.delete(key="var1", session=session)
        session.commit()

image

@jx2lee jx2lee changed the title Add extra-forbid validation to FastAPI request/response models Add extra-forbid validation to request/response models on execution api Dec 19, 2024
@amoghrajesh amoghrajesh self-requested a review December 19, 2024 05:14
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.

There's a PR opened for doing that in the core_apis. More information here:
#44306

I suggest to consolidate the approach for all of our APIs.

Most likely Bodies / input payloads have forbid_extra=True, but Responses do not. (because this is useful for filtering down a subpart of a model, and this feature is used by some endpoints).

@jx2lee
Copy link
Contributor Author

jx2lee commented Dec 20, 2024

@pierrejeambrun Thanks for sharing issue.

I suggest to consolidate the approach for all of our APIs.

Most likely Bodies / input payloads have forbid_extra=True, but Responses do not. (because this is useful for filtering down a subpart of a model, and this feature is used by some endpoints).

I'll review the shared PR and make adjustments based on it!
(Once the above PR is completed..!)

@jx2lee jx2lee changed the title Add extra-forbid validation to request/response models on execution api forbid extra fields on execution api Dec 21, 2024
@jx2lee jx2lee changed the title forbid extra fields on execution api Forbid extra fields on execution api Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forbid extra fields in execution API request and response models
2 participants