pierrejeambrun commented on code in PR #61279:
URL: https://github.com/apache/airflow/pull/61279#discussion_r2769881286
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_grid.py:
##########
@@ -857,6 +857,85 @@ def
test_structure_includes_historical_removed_task_with_proper_shape(self, sess
assert "is_mapped" not in t4
assert "children" not in t4
+ def test_task_converted_to_taskgroup_doesnt_crash(self, session,
test_client):
+ """Test that converting a task to TaskGroup with same name doesn't
crash grid view.
+
+ Regression test for https://github.com/apache/airflow/issues/61208
+ When a task is converted to a TaskGroup between DAG versions, old runs
have
+ children=None while new runs have children=[...]. The merge should
handle this.
+ """
+ from airflow.models import DagRun
+ from airflow.providers.standard.operators.python import PythonOperator
+ from airflow.sdk import DAG
+
+ dag_id = "test_task_to_group_conversion"
+
+ # Version 1: task_a is a simple task
+ with DAG(
+ dag_id=dag_id,
+ start_date=pendulum.datetime(2024, 1, 1, tz="UTC"),
+ schedule=None,
+ ) as dag_v1:
+ task_a = PythonOperator(task_id="task_a", python_callable=lambda:
True) # noqa: F841
+ task_b = PythonOperator(task_id="task_b", python_callable=lambda:
True)
+
+ # Create a DagRun with the old version
+ dag_bag = DBDagBag()
+ dag_bag.bag_dag(dag_v1, root_dag=dag_v1)
+ session.commit()
+
+ dr1 = DagRun(
+ dag_id=dag_id,
+ run_id="test_run_1",
+ run_type=DagRunType.MANUAL,
+ run_after=pendulum.datetime(2024, 1, 1, tz="UTC"),
+ )
+ session.add(dr1)
+ session.commit()
+
+ # Version 2: task_a becomes a TaskGroup with subtasks
+ with DAG(
+ dag_id=dag_id,
+ start_date=pendulum.datetime(2024, 1, 1, tz="UTC"),
+ schedule=None,
+ ) as dag_v2:
+ with TaskGroup(group_id="task_a") as task_a_group: # noqa: F841
+ task_a1 = PythonOperator(task_id="task_a1",
python_callable=lambda: True) # noqa: F841
+ task_a2 = PythonOperator(task_id="task_a2",
python_callable=lambda: True) # noqa: F841
+ task_b = PythonOperator(task_id="task_b", python_callable=lambda:
True) # noqa: F841
Review Comment:
Remove the variable assignment it appears you don't need it. So you don't
have to `# noqa: F841`
##########
airflow-core/tests/unit/api_fastapi/core_api/services/ui/test_grid.py:
##########
@@ -0,0 +1,156 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from airflow.api_fastapi.core_api.services.ui.grid import _merge_node_dicts
+
+
+class TestMergeNodeDicts:
+ """Unit tests for _merge_node_dicts function."""
+
+ def test_merge_simple_nodes(self):
+ """Test merging simple nodes without children."""
+ current = [{"id": "task1", "label": "Task 1", "children": None}]
+ new = [{"id": "task2", "label": "Task 2", "children": None}]
+
+ _merge_node_dicts(current, new)
+
+ assert len(current) == 2
+ assert current[0]["id"] == "task1"
+ assert current[1]["id"] == "task2"
+
+ def test_merge_with_none_new_list(self):
+ """Test that merging with None doesn't crash (regression test for
issue #61208)."""
+ current = [{"id": "task1", "label": "Task 1", "children": None}]
+ new = None
+
+ # Should not raise TypeError
+ _merge_node_dicts(current, new)
+
+ # Current should remain unchanged
+ assert len(current) == 1
+ assert current[0]["id"] == "task1"
+
+ def test_merge_task_to_taskgroup_conversion(self):
+ """Test merging when a task is converted to a TaskGroup.
+
+ This is the main regression test for issue #61208.
+ Old version has task with children=None, new version has TaskGroup
with children=[...].
+ """
+ # Old version: task_a is a simple task
+ current = [{"id": "task_a", "label": "Task A", "children": None}]
+
+ # New version: task_a is now a TaskGroup with children
+ new = [
+ {
+ "id": "task_a",
+ "label": "Task A",
+ "children": [
+ {"id": "task_a.subtask1", "label": "Subtask 1",
"children": None},
+ {"id": "task_a.subtask2", "label": "Subtask 2",
"children": None},
+ ],
+ }
+ ]
+
+ # Should not raise TypeError: 'NoneType' object is not iterable
+ _merge_node_dicts(current, new)
+
+ # Current should now have the new structure (TaskGroup)
+ assert len(current) == 1
+ assert current[0]["id"] == "task_a"
+ # The old node with children=None is kept (not merged into children)
Review Comment:
There are a lot of auto generated code comments that aren't helpful. Those
just describe the code and I would remove them.
##########
airflow-core/tests/unit/api_fastapi/core_api/services/ui/test_grid.py:
##########
@@ -0,0 +1,156 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from airflow.api_fastapi.core_api.services.ui.grid import _merge_node_dicts
+
+
+class TestMergeNodeDicts:
+ """Unit tests for _merge_node_dicts function."""
+
+ def test_merge_simple_nodes(self):
+ """Test merging simple nodes without children."""
+ current = [{"id": "task1", "label": "Task 1", "children": None}]
+ new = [{"id": "task2", "label": "Task 2", "children": None}]
+
+ _merge_node_dicts(current, new)
+
+ assert len(current) == 2
+ assert current[0]["id"] == "task1"
+ assert current[1]["id"] == "task2"
+
+ def test_merge_with_none_new_list(self):
+ """Test that merging with None doesn't crash (regression test for
issue #61208)."""
+ current = [{"id": "task1", "label": "Task 1", "children": None}]
+ new = None
+
+ # Should not raise TypeError
+ _merge_node_dicts(current, new)
+
+ # Current should remain unchanged
+ assert len(current) == 1
+ assert current[0]["id"] == "task1"
+
+ def test_merge_task_to_taskgroup_conversion(self):
+ """Test merging when a task is converted to a TaskGroup.
+
+ This is the main regression test for issue #61208.
+ Old version has task with children=None, new version has TaskGroup
with children=[...].
+ """
+ # Old version: task_a is a simple task
+ current = [{"id": "task_a", "label": "Task A", "children": None}]
+
+ # New version: task_a is now a TaskGroup with children
+ new = [
+ {
+ "id": "task_a",
+ "label": "Task A",
+ "children": [
+ {"id": "task_a.subtask1", "label": "Subtask 1",
"children": None},
+ {"id": "task_a.subtask2", "label": "Subtask 2",
"children": None},
+ ],
+ }
+ ]
+
+ # Should not raise TypeError: 'NoneType' object is not iterable
+ _merge_node_dicts(current, new)
+
+ # Current should now have the new structure (TaskGroup)
+ assert len(current) == 1
+ assert current[0]["id"] == "task_a"
+ # The old node with children=None is kept (not merged into children)
+
+ def test_merge_taskgroup_to_task_conversion(self):
+ """Test merging when a TaskGroup is converted to a simple task."""
+ # Old version: task_a is a TaskGroup
+ current = [
+ {
+ "id": "task_a",
+ "label": "Task A",
+ "children": [
+ {"id": "task_a.subtask1", "label": "Subtask 1",
"children": None},
+ ],
+ }
+ ]
+
+ # New version: task_a is now a simple task
+ new = [{"id": "task_a", "label": "Task A", "children": None}]
+
+ # Should not crash
+ _merge_node_dicts(current, new)
+
+ assert len(current) == 1
+ assert current[0]["id"] == "task_a"
+
+ def test_merge_nested_children(self):
+ """Test merging nodes with nested children."""
+ current = [
+ {
+ "id": "group1",
+ "label": "Group 1",
+ "children": [
+ {"id": "group1.task1", "label": "Task 1", "children":
None},
+ ],
Review Comment:
what happens if current has no children ("None"), but new has children. I
don't think it will work in the current implementation.
##########
airflow-core/src/airflow/api_fastapi/core_api/services/ui/grid.py:
##########
@@ -32,19 +32,29 @@
log = structlog.get_logger(logger_name=__name__)
-def _merge_node_dicts(current, new) -> None:
+def _merge_node_dicts(current: list[dict[str, any]], new: list[dict[str, any]]
| None) -> None:
+ """Merge node dictionaries from different DAG versions, handling structure
changes."""
+ # Handle None case - can occur when merging old DAG versions
+ # where a task was converted to a TaskGroup or vice versa
+ if new is None:
+ return
+
current_ids = {node["id"] for node in current}
for node in new:
if node["id"] in current_ids:
current_node = _get_node_by_id(current, node["id"])
- # if we have children, merge those as well
- if current_node.get("children"):
- _merge_node_dicts(current_node["children"], node["children"])
+ # Only merge children if both nodes have children to avoid
TypeError
+ # This handles cases where a task is converted to a TaskGroup
between versions
+ current_children = current_node.get("children")
+ node_children = node.get("children")
+ if current_children is not None and node_children is not None:
+ _merge_node_dicts(current_children, node_children)
Review Comment:
I don't think this logic is correct.
What if `current_children` is None, but `node_children` is not None ? This
won't do anything and be skipped, while we actually expect the children from
the 'node_children' to be merged into the 'current_node'. (an array should be
created and children should be appended to that).
Instead I think we should skip if both are None, but all other cases, merge
and provider an empty list default to None arrays.
--
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]
