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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
