-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Fix XCom key handling when keys contain special characters like slash #58344
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
Conversation
potiuk
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.
It looks good to me from general approach - but it's not my area of expertise, so someone who works on API should take a look as this is an important part of our core APIs
airflow-core/src/airflow/api_fastapi/core_api/routes/public/xcom.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/routes/public/xcom.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/execution_api/routes/xcoms.py
Outdated
Show resolved
Hide resolved
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_xcom.py
Outdated
Show resolved
Hide resolved
ashb
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.
Not confinced the quote/unqoute behaviour is right. And if xcom_key is wrong -- it's a required field.
cd15ca0 to
5e89a44
Compare
5e89a44 to
542ba3a
Compare
jason810496
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.
Nice!
It would be nice to add pytest.mark.parametrize with slash value key in task-sdk-integration-tests/tests/task_sdk_tests/test_xcom_operations.py tests.
Thanks!
364ccfd to
1c2835e
Compare
airflow-core/src/airflow/api_fastapi/core_api/routes/public/xcom.py
Outdated
Show resolved
Hide resolved
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, @ashb do you mind reconsidering your request for change by any chance and giving this a second look? :)
bd882d9 to
7385d94
Compare
pierrejeambrun
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.
@henry3260 Do you mind rebasing to fix the conflicts. We can probably go ahead and merge, the PR is looking good.
Sure |
7a4b010 to
828390f
Compare
All CI tests have passed. Ready for merge! |
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.
A few nits to fix and we should be good to merge.
airflow-core/src/airflow/api_fastapi/core_api/routes/public/xcom.py
Outdated
Show resolved
Hide resolved
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_xcom.py
Outdated
Show resolved
Hide resolved
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_xcom.py
Outdated
Show resolved
Hide resolved
4e80248 to
658f49d
Compare
|
Thanks for the PR. |
This PR picks up the work from @Brunda10 in PR #56042, which had become stale.
Closes: #55410
^ 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.