Skip to content

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 3 times, most recently 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()

@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
@jx2lee jx2lee force-pushed the extra-field-execution branch from ac39ba4 to 76fcae9 Compare January 15, 2025 12:25
@jx2lee jx2lee force-pushed the extra-field-execution branch 3 times, most recently from 51e685a to a358b7d Compare February 2, 2025 07:06
@jx2lee jx2lee changed the title Forbid extra fields on execution api Enforce to forbid extra fields under execution_api Feb 2, 2025
@jx2lee jx2lee requested a review from pierrejeambrun February 2, 2025 07:15
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. A few suggestions, but looking good.

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Feb 4, 2025

One test to remove and we should be good.

@jx2lee
Copy link
Contributor Author

jx2lee commented Feb 4, 2025

One test to remove and we should be good.

I'm in gym now, go back to fix soon!

@jx2lee jx2lee force-pushed the extra-field-execution branch 2 times, most recently from be2e289 to c22d21a Compare February 4, 2025 14:10
@pierrejeambrun
Copy link
Member

Re-running failed job

@jx2lee jx2lee force-pushed the extra-field-execution branch from c22d21a to 5111226 Compare February 5, 2025 10:09
@jx2lee
Copy link
Contributor Author

jx2lee commented Feb 5, 2025

@pierrejeambrun Do i need to modify code?

@pierrejeambrun
Copy link
Member

CI is green, merging, thanks.

@pierrejeambrun pierrejeambrun merged commit cb4590a into apache:main Feb 5, 2025
63 checks passed
insomnes pushed a commit to insomnes/airflow that referenced this pull request Feb 6, 2025
* extra forbidden

* test

* gen_file

* fix gen_file

* rm clear_db_*

* rm testcase

* rm testcase # 2
niklasr22 pushed a commit to niklasr22/airflow that referenced this pull request Feb 8, 2025
* extra forbidden

* test

* gen_file

* fix gen_file

* rm clear_db_*

* rm testcase

* rm testcase # 2
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 17, 2025
* extra forbidden

* test

* gen_file

* fix gen_file

* rm clear_db_*

* rm testcase

* rm testcase # 2
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