You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "haseebmalik18 (via GitHub)" <gi...@apache.org> on 2026/05/16 00:11:43 UTC
[PR] Strengthen trigger-hash tests with a fully-serialized JSON guard and broader fixtures #66413 [airflow]
haseebmalik18 opened a new pull request, #67018:
URL: https://github.com/apache/airflow/pull/67018
Adds a `_assert_fully_serialized` helper that runs `json.dumps` on the output of `encode_trigger` and checks nothing got double-wrapped. It's wired into the existing parametrized tests so every fixture goes through it.
Also broadens `_TRIGGER_PARAMS` to cover the shapes the issue called out: datetime, timedelta, nested dict/tuple, set/frozenset, falsy primitives, str-Enum, and Path.
closes: #66413
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Strengthen trigger-hash tests with a fully-serialized JSON guard and broader fixtures #66413 [airflow]
Posted by "haseebmalik18 (via GitHub)" <gi...@apache.org>.
haseebmalik18 commented on PR #67018:
URL: https://github.com/apache/airflow/pull/67018#issuecomment-4568892009
Reviewed the co pilot changes which were valid and I made those requested changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Strengthen trigger-hash tests with a fully-serialized JSON guard and broader fixtures #66413 [airflow]
Posted by "haseebmalik18 (via GitHub)" <gi...@apache.org>.
haseebmalik18 commented on PR #67018:
URL: https://github.com/apache/airflow/pull/67018#issuecomment-4615021090
@jason810496 Following up on this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Strengthen trigger-hash tests with a fully-serialized JSON guard and broader fixtures #66413 [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on PR #67018:
URL: https://github.com/apache/airflow/pull/67018#issuecomment-4618832615
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Strengthen trigger-hash tests with a fully-serialized JSON guard and broader fixtures #66413 [airflow]
Posted by "haseebmalik18 (via GitHub)" <gi...@apache.org>.
haseebmalik18 commented on PR #67018:
URL: https://github.com/apache/airflow/pull/67018#issuecomment-4465131595
@jason810496 Ready to review whenever you get the chance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Strengthen trigger-hash tests with a fully-serialized JSON guard and broader fixtures #66413 [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 merged PR #67018:
URL: https://github.com/apache/airflow/pull/67018
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Strengthen trigger-hash tests with a fully-serialized JSON guard and broader fixtures #66413 [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #67018:
URL: https://github.com/apache/airflow/pull/67018#discussion_r3301249150
##########
airflow-core/tests/unit/serialization/test_encoders.py:
##########
@@ -88,8 +118,86 @@ def serialize(self) -> tuple[str, dict[str, Any]]:
),
id="all_non_primitive_kwargs",
),
+ pytest.param(
+ _GenericKwargsTrigger(
+ moment=datetime.datetime(2026, 1, 15, 12, 30, tzinfo=datetime.timezone.utc),
+ ),
+ id="datetime_kwarg",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(delta=datetime.timedelta(hours=2, minutes=30, seconds=15)),
+ id="timedelta_kwarg",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(
+ mapping={"outer": {"inner": [1, 2, 3]}, "sibling": ["a", "b"]},
+ nested_records={"items": [{"k": "v"}, {"k2": "v2"}]},
+ tuple_of_tuples=(("a", "b"), ("c", "d")),
+ ),
+ id="nested_containers",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(
+ tags={"a", "b", "c"},
+ frozen=frozenset(["x", "y"]),
+ ),
+ id="set_and_frozenset_kwargs",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(
+ none_val=None,
+ zero=0,
+ empty_str="",
+ false_val=False,
+ empty_list=[],
+ empty_dict={},
+ ),
+ id="falsy_primitives",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(color=_StrColor.RED),
+ id="str_enum_kwarg",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(path=pathlib.Path("/tmp/test/file.txt")),
+ id="path_kwarg",
+ ),
]
+# datetime_kwarg is excluded from the DB round-trip tests: BaseSerialization's
+# DATETIME wrapper cannot round-trip through Trigger.encrypt_kwargs / _decrypt_kwargs yet.
+_DB_ROUND_TRIP_PARAMS = [p for p in _TRIGGER_PARAMS if p.id != "datetime_kwarg"]
Review Comment:
Excluding `datetime_kwarg` from the DB round-trip parametrization reduces coverage for a trigger shape that exists in-core (e.g. `DateTimeTrigger` uses a datetime-like `moment`). If this is currently failing due to a known limitation, consider keeping it in the DB tests as an explicit `xfail` (with a reason / issue link) or adding a dedicated regression test that demonstrates the current failure, so the suite will start exercising the DB round-trip automatically once the limitation is fixed.
##########
airflow-core/tests/unit/serialization/test_encoders.py:
##########
@@ -88,8 +118,86 @@ def serialize(self) -> tuple[str, dict[str, Any]]:
),
id="all_non_primitive_kwargs",
),
+ pytest.param(
+ _GenericKwargsTrigger(
+ moment=datetime.datetime(2026, 1, 15, 12, 30, tzinfo=datetime.timezone.utc),
+ ),
+ id="datetime_kwarg",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(delta=datetime.timedelta(hours=2, minutes=30, seconds=15)),
+ id="timedelta_kwarg",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(
+ mapping={"outer": {"inner": [1, 2, 3]}, "sibling": ["a", "b"]},
+ nested_records={"items": [{"k": "v"}, {"k2": "v2"}]},
+ tuple_of_tuples=(("a", "b"), ("c", "d")),
+ ),
+ id="nested_containers",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(
+ tags={"a", "b", "c"},
+ frozen=frozenset(["x", "y"]),
+ ),
+ id="set_and_frozenset_kwargs",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(
+ none_val=None,
+ zero=0,
+ empty_str="",
+ false_val=False,
+ empty_list=[],
+ empty_dict={},
+ ),
+ id="falsy_primitives",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(color=_StrColor.RED),
+ id="str_enum_kwarg",
+ ),
+ pytest.param(
+ _GenericKwargsTrigger(path=pathlib.Path("/tmp/test/file.txt")),
+ id="path_kwarg",
+ ),
]
+# datetime_kwarg is excluded from the DB round-trip tests: BaseSerialization's
+# DATETIME wrapper cannot round-trip through Trigger.encrypt_kwargs / _decrypt_kwargs yet.
+_DB_ROUND_TRIP_PARAMS = [p for p in _TRIGGER_PARAMS if p.id != "datetime_kwarg"]
+
+
+def _assert_fully_serialized(encoded_kwargs: dict[str, Any]) -> None:
+ """Assert encoded kwargs are fully JSON-safe and not immediately re-wrapped.
+
+ The hash check in :class:`TestTriggerHashConsistency` only catches asymmetric
+ breakage. This guard also catches non-JSON values left in
+ ``encode_trigger`` output via a ``str()`` fallback, which would still break
+ :func:`Trigger.encrypt_kwargs` at its ``json.dumps`` step.
Review Comment:
The docstring here says a `str()` fallback would still break `Trigger.encrypt_kwargs` at the `json.dumps` step, but `BaseSerialization.default_serialization()` already coerces unexpected objects to `str` (when `strict=False`), which *would* be JSON-serializable and therefore wouldn’t fail `json.dumps`. Consider rewording this to describe the actual failure mode you’re guarding against (e.g. unwrapped / non-JSON-safe values leaking through) to avoid misleading future readers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org