Fix trigger kwargs decoding for asset watcher triggers#65992
Conversation
|
Hello @ashb @bolkedebruin, |
Given this is not yet merged there is no way it can new included in any release, especially not one released already |
|
@jedcunningham @amoghrajesh can you take a look please? |
Thanks for the quick reply. What do you mean by "released already"? Are you talking about version 3.2.1 here? I do not see version 3.2.2 in the release notes yet on the Airflow documentation site. Also, if I am not mistaken, the milestone for version 3.2.2 is not complete yet. That's why I asked if this would be available in the 3.2.2 release. |
|
My bad, getting version numbers mixed up! Same though, if it's reviewed and merged, it'll be in the next bug fix release |
|
Hello, Sorry to bother y'all here again, but can we have the merge conflicts resolved, @shaealh? And then, can we have this reviewed? I am really hoping that this will be included in the next 3.2.x release, so we can finally upgrade to Airflow 3.2.x version without worrying about breaking any of our production functionalities and finally get to enjoy all the great improvements that have been added to v3.2.0+ versions. |
|
@shaealh — Removing the The label's contract is that the PR is ready for maintainer review — a regression like this means the PR temporarily isn't. Rebase your branch onto the latest
No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
|
@ashb @jedcunningham @amoghrajesh anything I can do on my end to help get your review so I can prepare to merge? |
|
@ashb @bolkedebruin @jedcunningham @potiuk @amoghrajesh |
|
@ashb @bolkedebruin @jedcunningham @potiuk @amoghrajesh |
|
@vatsrahul1001 @ashb @bolkedebruin @jedcunningham @potiuk @amoghrajesh |
|
@potiuk A couple of months under review, any updates? Thanks |
|
@bolkedebruin @ashb can you approve for merge? |
|
Dug into |
|
Thanks for the detailed review @Vamsi-klu and for validating the root cause and fix. Good catch on the redundant traversal. Since it is idempotent and the kwargs are small, I’ll leave it as is unless a maintainer (@potiuk, @ashb, @bolkedebruin) prefers that it be cleaned up before merging. |
Fixes #65973
Trigger kwargs for asset watcher triggers can pass through both BaseSerialization and SDK serde. When this happens, old serialization marker keys can be stored as the stringified enum names
Encoding.TYPEandEncoding.VAR, which prevents triggerer-side cleanup from decoding nested kwargs before trigger execution.This updates trigger kwargs decoding to normalize those stringified marker keys and recursively decode nested serialized kwargs before trigger instantiation.
Tests:
AIRFLOW_HOME=/tmp/airflow-65973-test uv run pytest airflow-core/tests/unit/jobs/test_triggerer_job.py::TestTriggerRunner::test_trigger_kwargs_cleanup_decodes_stringified_encoding_keys airflow-core/tests/unit/jobs/test_triggerer_job.py::TestTriggerRunner::test_trigger_kwargs_serialization_cleanupuv run ruff check airflow-core/src/airflow/serialization/decoders.py airflow-core/tests/unit/jobs/test_triggerer_job.py