ephraimbuddy commented on code in PR #63871:
URL: https://github.com/apache/airflow/pull/63871#discussion_r3166962618
##########
airflow-core/src/airflow/serialization/helpers.py:
##########
@@ -61,25 +65,32 @@ def translate_tuples_to_lists(obj: Any):
return {key: translate_tuples_to_lists(value) for key, value in
obj.items()}
return obj
- def sort_dict_recursively(obj: Any) -> Any:
- """Recursively sort dictionaries to ensure consistent ordering."""
+ def sort_and_make_static_dict_recursively(obj: Any) -> Any:
Review Comment:
I don't think the name change is necessary. This is now mouthful. The
previous name can accommodate the changes.
##########
airflow-core/tests/unit/serialization/test_helpers.py:
##########
@@ -23,6 +23,8 @@
def test_serialize_template_field_with_very_small_max_length(monkeypatch):
"""Test that truncation message is prioritized even for very small
max_length."""
+ from airflow.serialization.helpers import serialize_template_field
Review Comment:
```suggestion
```
Redundant
##########
airflow-core/tests/unit/serialization/test_helpers.py:
##########
@@ -51,3 +53,34 @@ def test_argnotset_repr_and_str():
assert str(NOTSET) == "NOTSET"
assert repr(SET_DURING_EXECUTION) == "DYNAMIC (set during execution)"
assert str(SET_DURING_EXECUTION) == "DYNAMIC (set during execution)"
+
+
+def test_serialize_template_field_with_dict_value_callable():
+ from airflow.serialization.helpers import serialize_template_field
Review Comment:
```suggestion
```
##########
airflow-core/src/airflow/serialization/helpers.py:
##########
@@ -61,25 +65,32 @@ def translate_tuples_to_lists(obj: Any):
return {key: translate_tuples_to_lists(value) for key, value in
obj.items()}
return obj
- def sort_dict_recursively(obj: Any) -> Any:
- """Recursively sort dictionaries to ensure consistent ordering."""
+ def sort_and_make_static_dict_recursively(obj: Any) -> Any:
+ """Recursively sort dictionaries and make callable value static to
ensure consistent ordering."""
+ if callable(obj):
+ return make_callable_static(obj)
if isinstance(obj, dict):
- return {k: sort_dict_recursively(v) for k, v in
sorted(obj.items())}
+ return {k: sort_and_make_static_dict_recursively(v) for k, v in
sorted(obj.items())}
if isinstance(obj, list):
- return [sort_dict_recursively(item) for item in obj]
+ return [sort_and_make_static_dict_recursively(item) for item in
obj]
if isinstance(obj, tuple):
- return tuple(sort_dict_recursively(item) for item in obj)
+ return tuple(sort_and_make_static_dict_recursively(item) for item
in obj)
return obj
max_length = conf.getint("core", "max_templated_field_length")
if not is_jsonable(template_field):
try:
- serialized = template_field.serialize()
+ # If the callable objects are in dict values, it makes is_jsonable
False.
+ # So, exclude dicts here; they are handled separately in the logic
below.
Review Comment:
```suggestion
# Dicts may be non-jsonable because they contain callables;
normalize them here so str() is stable.
```
--
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]
