-
Notifications
You must be signed in to change notification settings - Fork 16.3k
SerDe: Check more strictly for pydantic model #56758
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
SerDe: Check more strictly for pydantic model #56758
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
09b4dac to
1fbf590
Compare
sjyangkevin
left a comment
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.
@Desdroid thanks for the PR. The change looks good to me.
While testing the change; I noticed to deserialize this pydantic dataclass, we do need to whitelist it, I hope it is something we also need to do before. The current implementation ensure to deserialize only known objects, due to security considerations.
To whitelist it, we can configure the following airflow config, below is an example.
|
I am not sure I understand you. |
1fbf590 to
5a8a275
Compare
|
@sjyangkevin So can you tell me what else is required? CI test are good. |
It looks good to me. Would be great to have a second eyes on it @amoghrajesh , @bolkedebruin |
amoghrajesh
left a comment
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.
Thanks for your contribution @Desdroid!
Some nits, otherwise looks good.
487fa59 to
5331042
Compare
|
Just rebased it to fix the CI |
|
@uranusjr can you take another look at this one? |
…sses are not detected as pydantic models. (apache#56739)
d14e69f to
7cf8a77
Compare
|
@bolkedebruin Alright, moved it back up then. |
amoghrajesh
left a comment
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.
LGTM, @bolkedebruin / @uranusjr?
* Check more strictly for pydantic models. Ensure that pydantic dataclasses are not detected as pydantic models. (apache#56739) * Move test to other file. (apache#56739) (cherry picked from commit 9613558) Co-authored-by: Desdroid <Desdroid@users.noreply.github.com>
|
Manual backport: http://github.com/apache/airflow/pull/57616 |
* Check more strictly for pydantic models. Ensure that pydantic dataclasses are not detected as pydantic models. (apache#56739) * Move test to other file. (apache#56739)
Ensure that pydantic dataclasses are not detected as pydantic models.
As discussed in #56739 check more strictly for pydantic models and avoid detecting pydantic dataclasses as pydantic models.
Went with the suggestion of @amoghrajesh and used stdlib dataclasses to check that the detected model is not a dataclass.
Fixes: #56739
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.