Copilot commented on code in PR #63994:
URL: https://github.com/apache/airflow/pull/63994#discussion_r3040828096
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py:
##########
@@ -180,4 +197,22 @@ def bulk_pools(
session: SessionDep,
) -> BulkResponse:
"""Bulk create, update, and delete pools."""
+ if not conf.getboolean("core", "multi_team"):
+ invalid_entities = []
+
+ for action in request.actions:
+ if action.action == BulkAction.CREATE or action.action ==
BulkAction.UPDATE:
+ for entity in action.entities:
+ if isinstance(entity, PoolBody) and entity.team_name is
not None:
+ invalid_entities.append(entity.pool)
+
+ if invalid_entities:
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ {
+ "message": MULTI_TEAM_ERROR_MESSAGE,
+ "invalid_pool_names": invalid_entities,
+ },
+ )
Review Comment:
bulk_pools can now raise an HTTP 400 when multi_team is disabled and any
entity includes team_name, but the bulk endpoint decorator doesn't declare a
400 response. This means the generated OpenAPI spec (and openapi-ts client
types) won't include this error case. Add HTTP_400_BAD_REQUEST to the bulk
route's documented responses.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py:
##########
@@ -180,4 +197,22 @@ def bulk_pools(
session: SessionDep,
) -> BulkResponse:
"""Bulk create, update, and delete pools."""
+ if not conf.getboolean("core", "multi_team"):
+ invalid_entities = []
+
+ for action in request.actions:
+ if action.action == BulkAction.CREATE or action.action ==
BulkAction.UPDATE:
+ for entity in action.entities:
+ if isinstance(entity, PoolBody) and entity.team_name is
not None:
+ invalid_entities.append(entity.pool)
+
+ if invalid_entities:
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ {
+ "message": MULTI_TEAM_ERROR_MESSAGE,
+ "invalid_pool_names": invalid_entities,
+ },
+ )
+
return BulkPoolService(session=session, request=request).handle_request()
Review Comment:
The new multi_team validation in bulk_pools isn't covered by tests: this
test module already has TestBulkPools, but none of its cases cover multi_team
disabled + team_name provided. Add a test that asserts the bulk endpoint
returns the expected 400 (and error payload shape) when any create/update
entity includes team_name under multi_team=False.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py:
##########
@@ -171,6 +182,24 @@ def bulk_connections(
session: SessionDep,
) -> BulkResponse:
"""Bulk create, update, and delete connections."""
+ if not conf.getboolean("core", "multi_team"):
+ invalid_entities = []
+
+ for action in request.actions:
+ if action.action == BulkAction.CREATE or action.action ==
BulkAction.UPDATE:
+ for entity in action.entities:
+ if isinstance(entity, ConnectionBody) and entity.team_name
is not None:
+ invalid_entities.append(entity.connection_id)
+
+ if invalid_entities:
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ {
+ "message": MULTI_TEAM_ERROR_MESSAGE,
+ "invalid_connection_ids": invalid_entities,
+ },
+ )
+
return BulkConnectionService(session=session,
request=request).handle_request()
Review Comment:
The new multi_team validation in bulk_connections isn't covered by tests:
there are bulk endpoint tests in this module, but none asserting that a bulk
create/update with team_name returns the expected 400 when multi_team is
disabled. Add a regression test covering at least one bulk create and one bulk
update case.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py:
##########
@@ -171,6 +182,24 @@ def bulk_connections(
session: SessionDep,
) -> BulkResponse:
"""Bulk create, update, and delete connections."""
+ if not conf.getboolean("core", "multi_team"):
+ invalid_entities = []
+
+ for action in request.actions:
+ if action.action == BulkAction.CREATE or action.action ==
BulkAction.UPDATE:
+ for entity in action.entities:
+ if isinstance(entity, ConnectionBody) and entity.team_name
is not None:
+ invalid_entities.append(entity.connection_id)
+
+ if invalid_entities:
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ {
+ "message": MULTI_TEAM_ERROR_MESSAGE,
+ "invalid_connection_ids": invalid_entities,
+ },
+ )
Review Comment:
bulk_connections now explicitly raises an HTTP 400 when multi_team is
disabled and any entity includes team_name, but this endpoint decorator doesn't
declare a 400 response in create_openapi_http_exception_doc. As a result, the
generated OpenAPI spec and clients won't show/handle this 400. Add a
responses=... including HTTP_400_BAD_REQUEST for the bulk endpoint as well
(similar to the single-entity POST/PATCH).
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py:
##########
@@ -190,4 +204,22 @@ def bulk_variables(
session: SessionDep,
) -> BulkResponse:
"""Bulk create, update, and delete variables."""
+ if not conf.getboolean("core", "multi_team"):
+ invalid_entities = []
+
+ for action in request.actions:
+ if action.action == BulkAction.CREATE or action.action ==
BulkAction.UPDATE:
+ for entity in action.entities:
+ if isinstance(entity, VariableBody) and entity.team_name
is not None:
+ invalid_entities.append(entity.key)
+
+ if invalid_entities:
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ {
+ "message": MULTI_TEAM_ERROR_MESSAGE,
+ "invalid_variable_keys": invalid_entities,
+ },
+ )
Review Comment:
bulk_variables can now raise an HTTP 400 when multi_team is disabled and any
entity includes team_name, but the bulk endpoint decorator doesn't declare a
400 response. This leaves the generated OpenAPI spec out of sync with runtime
behavior; add HTTP_400_BAD_REQUEST to the documented responses for this bulk
endpoint.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py:
##########
@@ -190,4 +204,22 @@ def bulk_variables(
session: SessionDep,
) -> BulkResponse:
"""Bulk create, update, and delete variables."""
+ if not conf.getboolean("core", "multi_team"):
+ invalid_entities = []
+
+ for action in request.actions:
+ if action.action == BulkAction.CREATE or action.action ==
BulkAction.UPDATE:
+ for entity in action.entities:
+ if isinstance(entity, VariableBody) and entity.team_name
is not None:
+ invalid_entities.append(entity.key)
+
+ if invalid_entities:
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ {
+ "message": MULTI_TEAM_ERROR_MESSAGE,
+ "invalid_variable_keys": invalid_entities,
+ },
+ )
+
return BulkVariableService(session=session,
request=request).handle_request()
Review Comment:
The new multi_team validation in bulk_variables isn't covered by tests: this
repo already has bulk variables tests, but none assert the new 400 behavior
when multi_team is disabled and a create/update entity includes team_name. Add
a regression test for this case to prevent future regressions/reintroduction of
500s.
--
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]
