Skip to content

Commit bd9270d

Browse files
authored
Fix bulk create+overwrite silently resetting unset fields on pools and connections (#68645)
A bulk 'create' with action_on_existence=overwrite dumped the whole request body and set every field on the existing record, so fields the request omitted were reset to their defaults. For multi-team setups this silently nulled an existing pool's or connection's team_name ownership when an overwrite changed only e.g. slots; description and include_deferred were affected too. Only write the fields the request actually provided (model_dump(exclude_unset=True)), so omitted fields keep their current value while explicitly-set fields (even None) are still applied. Adds regression tests for both pools and connections.
1 parent 3d0fb9f commit bd9270d

4 files changed

Lines changed: 113 additions & 2 deletions

File tree

airflow-core/src/airflow/api_fastapi/core_api/services/public/connections.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,12 @@ def handle_bulk_create(
121121
if connection.connection_id in create_connection_ids:
122122
if connection.connection_id in matched_connection_ids:
123123
existed_connection = existed_connections_dict[connection.connection_id]
124-
for key, val in connection.model_dump(by_alias=True).items():
124+
# Only overwrite fields the request actually provided (see pools.py for the
125+
# full rationale). Plain ``model_dump()`` resets omitted fields to their
126+
# defaults on the existing connection — e.g. silently nulling ``team_name``
127+
# multi-team ownership. ``exclude_unset=True`` writes only the fields present
128+
# in the request body.
129+
for key, val in connection.model_dump(by_alias=True, exclude_unset=True).items():
125130
setattr(existed_connection, key, val)
126131
else:
127132
self.session.add(Connection(**connection.model_dump(by_alias=True)))

airflow-core/src/airflow/api_fastapi/core_api/services/public/pools.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,14 @@ def handle_bulk_create(self, action: BulkCreateAction[PoolBody], results: BulkAc
149149
if pool.pool in create_pool_names:
150150
if pool.pool in matched_pool_names:
151151
existed_pool = existing_pools_dict[pool.pool]
152-
for key, val in pool.model_dump().items():
152+
# Only overwrite fields the request actually provided. Plain ``model_dump()``
153+
# emits every field at its default, so an overwrite that omits e.g.
154+
# ``team_name``/``description``/``include_deferred`` silently resets them on the
155+
# existing pool — most damagingly nulling its multi-team ``team_name`` ownership.
156+
# ``exclude_unset=True`` writes only fields present in the request body, so
157+
# omitted fields keep their current value while an explicitly-set field (even
158+
# ``None``) is still applied.
159+
for key, val in pool.model_dump(exclude_unset=True).items():
153160
setattr(existed_pool, key, val)
154161
else:
155162
self.session.add(Pool(**pool.model_dump()))

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1984,6 +1984,40 @@ def test_bulk_rejects_team_name_when_multi_team_is_disabled(self, test_client):
19841984
expected_error_conn_ids = {err["input"]["connection_id"] for err in detail}
19851985
assert sorted(expected_error_conn_ids) == ["test_conn_id_2", "test_conn_id_3"]
19861986

1987+
@conf_vars({("core", "multi_team"): "True"})
1988+
def test_bulk_create_overwrite_preserves_unset_team_name(self, test_client, testing_team, session):
1989+
"""A bulk create+overwrite that omits ``team_name`` must NOT reset an existing connection's
1990+
``team_name`` to ``None`` (parity with the pools fix). Overwriting with only ``conn_type``
1991+
previously clobbered every unset field via ``model_dump(by_alias=True)`` — silently nulling
1992+
the connection's multi-team ownership. ``exclude_unset=True`` preserves omitted fields.
1993+
"""
1994+
self.create_connection(team_name=testing_team.name)
1995+
before = session.scalar(select(Connection).where(Connection.conn_id == TEST_CONN_ID))
1996+
assert before.team_name == testing_team.name
1997+
1998+
response = test_client.patch(
1999+
"/connections",
2000+
json={
2001+
"actions": [
2002+
{
2003+
"action": "create",
2004+
"action_on_existence": "overwrite",
2005+
"entities": [{"connection_id": TEST_CONN_ID, "conn_type": "new_type"}],
2006+
}
2007+
]
2008+
},
2009+
)
2010+
assert response.status_code == 200
2011+
assert response.json()["create"]["success"] == [TEST_CONN_ID]
2012+
2013+
session.expire_all()
2014+
after = session.scalar(select(Connection).where(Connection.conn_id == TEST_CONN_ID))
2015+
assert after.conn_type == "new_type" # provided field is applied
2016+
assert after.team_name == testing_team.name, (
2017+
"bulk overwrite that omitted team_name must preserve existing ownership, "
2018+
f"got team_name={after.team_name!r}"
2019+
)
2020+
19872021

19882022
class TestPostConnectionExtraBackwardCompatibility(TestConnectionEndpoint):
19892023
def test_post_should_accept_empty_string_as_extra(self, test_client, session):

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1240,3 +1240,68 @@ def test_bulk_rejects_team_name_when_multi_team_is_disabled(self, test_client):
12401240

12411241
expected_error_names = {err["input"]["name"] for err in detail}
12421242
assert sorted(expected_error_names) == ["pool_2", "pool_3"]
1243+
1244+
@conf_vars({("core", "multi_team"): "True"})
1245+
def test_bulk_create_overwrite_preserves_unset_team_name(self, test_client, session):
1246+
"""A bulk create+overwrite that omits ``team_name`` must NOT reset an existing pool's
1247+
``team_name`` to ``None``.
1248+
1249+
``POOL1_NAME`` is owned by team ``test``. Overwriting it with a body that changes only
1250+
``slots`` (no ``team_name``) previously clobbered every unset field back to its default via
1251+
``model_dump()`` — silently nulling the pool's multi-team ownership. With
1252+
``model_dump(exclude_unset=True)`` the omitted ``team_name`` keeps its current value.
1253+
"""
1254+
self.create_pools()
1255+
before = session.scalar(select(Pool).where(Pool.pool == POOL1_NAME))
1256+
assert before.team_name == "test"
1257+
1258+
response = test_client.patch(
1259+
"/pools",
1260+
json={
1261+
"actions": [
1262+
{
1263+
"action": "create",
1264+
"action_on_existence": "overwrite",
1265+
"entities": [{"name": POOL1_NAME, "slots": 99}],
1266+
}
1267+
]
1268+
},
1269+
)
1270+
assert response.status_code == 200
1271+
assert response.json()["create"]["success"] == [POOL1_NAME]
1272+
1273+
session.expire_all()
1274+
after = session.scalar(select(Pool).where(Pool.pool == POOL1_NAME))
1275+
assert after.slots == 99 # the field that WAS provided is applied
1276+
assert after.team_name == "test", (
1277+
"bulk overwrite that omitted team_name must preserve existing ownership, "
1278+
f"got team_name={after.team_name!r}"
1279+
)
1280+
1281+
@conf_vars({("core", "multi_team"): "True"})
1282+
def test_bulk_create_overwrite_applies_explicit_team_name(self, test_client, session):
1283+
"""An explicitly-provided ``team_name`` on a bulk overwrite is still applied (the fix only
1284+
skips *omitted* fields, it must not skip fields the request actually set)."""
1285+
_create_team()
1286+
session.add(Team(name="other"))
1287+
session.commit()
1288+
_create_pools() # POOL1 owned by team "test"
1289+
1290+
response = test_client.patch(
1291+
"/pools",
1292+
json={
1293+
"actions": [
1294+
{
1295+
"action": "create",
1296+
"action_on_existence": "overwrite",
1297+
"entities": [{"name": POOL1_NAME, "slots": 7, "team_name": "other"}],
1298+
}
1299+
]
1300+
},
1301+
)
1302+
assert response.status_code == 200
1303+
1304+
session.expire_all()
1305+
after = session.scalar(select(Pool).where(Pool.pool == POOL1_NAME))
1306+
assert after.team_name == "other"
1307+
assert after.slots == 7

0 commit comments

Comments
 (0)