Copilot commented on code in PR #63994:
URL: https://github.com/apache/airflow/pull/63994#discussion_r3025588529
##########
airflow-core/src/airflow/ui/src/queries/useEditVariable.ts:
##########
@@ -78,12 +78,13 @@ export const useEditVariable = (
const parsedDescription =
editVariableRequestBody.description === "" ? undefined :
editVariableRequestBody.description;
+ const teamName = editVariableRequestBody.team_name === "" ? undefined :
editVariableRequestBody.team_name;
Review Comment:
Converting an empty team_name ("") to undefined means the field will be
omitted from the JSON request, but updateMask still includes "team_name". Since
the backend only applies fields present in model_fields_set, this makes it
impossible to clear an existing team assignment from the UI (it will remain
unchanged). Consider sending null (to explicitly clear) when the user empties
the field, or only adding "team_name" to updateMask when you also include a
value in the request body.
```suggestion
const teamName = editVariableRequestBody.team_name === "" ? null :
editVariableRequestBody.team_name;
```
##########
airflow-core/src/airflow/ui/src/queries/useEditConnection.tsx:
##########
@@ -89,6 +89,11 @@ export const useEditConnection = (
if (requestBody.schema !== initialConnection.schema) {
updateMask.push("schema");
}
+ if (requestBody.team_name !== initialConnection.team_name) {
+ updateMask.push("team_name");
+ }
+
+ const teamName = requestBody.team_name === "" ? undefined :
requestBody.team_name;
Review Comment:
If the user clears team_name in the form (""), teamName becomes undefined
and will be omitted from the JSON payload, but updateMask still contains
"team_name". Because the API patch logic relies on model_fields_set, this
prevents clearing a previously set team_name. Prefer sending null to explicitly
clear the field (and keep updateMask), or adjust updateMask computation to
match what is actually sent.
```suggestion
const teamName = requestBody.team_name === "" ? null :
requestBody.team_name; // eslint-disable-line unicorn/no-null
```
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/connections.py:
##########
@@ -149,15 +149,24 @@ def get_connections(
"",
status_code=status.HTTP_201_CREATED,
responses=create_openapi_http_exception_doc(
- [status.HTTP_409_CONFLICT]
- ), # handled by global exception handler
+ [
+ status.HTTP_400_BAD_REQUEST,
+ status.HTTP_409_CONFLICT, # handled by global exception handler
+ ]
+ ),
dependencies=[Depends(requires_access_connection(method="POST")),
Depends(action_logging())],
)
def post_connection(
post_body: ConnectionBody,
session: SessionDep,
) -> ConnectionResponse:
"""Create connection entry."""
+ if post_body.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ "team_name cannot be set when multi_team mode is disabled. Please
contact your administrator.",
+ )
Review Comment:
This validation is added for single-entity POST/PATCH, but the bulk endpoint
(bulk_connections) still accepts ConnectionBody entries with team_name when
multi_team is disabled. That means clients can bypass this restriction via the
bulk API, likely reintroducing the original 500/incorrect behavior. Consider
enforcing the same check for bulk requests (either in the route handler before
delegating to the service, or inside the bulk service).
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py:
##########
@@ -147,21 +148,33 @@ def patch_variable(
update_mask: list[str] | None = Query(None),
) -> VariableResponse:
"""Update a variable by key."""
+ if patch_body.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ "team_name cannot be set when multi_team mode is disabled. Please
contact your administrator.",
+ )
+
Review Comment:
This validation is implemented in the route handlers and returns HTTP 400,
but the PR description states “pydantic validators” and mentions returning a
422. Please either align the implementation with the description (e.g.,
model-level validation that yields 422) or update the PR description to match
the actual behavior/status code.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py:
##########
@@ -143,6 +144,12 @@ def patch_pool(
update_mask: list[str] | None = Query(None),
) -> PoolResponse:
"""Update a Pool."""
+ if patch_body.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ "team_name cannot be set when multi_team mode is disabled. Please
contact your administrator.",
+ )
Review Comment:
The same literal error message is duplicated across multiple endpoints
(connections/pools/variables, POST and PATCH). To avoid drift and simplify
future changes/translations, consider extracting this message into a shared
constant/helper and reusing it in all three routes (and in tests).
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py:
##########
@@ -143,6 +144,12 @@ def patch_pool(
update_mask: list[str] | None = Query(None),
) -> PoolResponse:
"""Update a Pool."""
+ if patch_body.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ "team_name cannot be set when multi_team mode is disabled. Please
contact your administrator.",
+ )
+
Review Comment:
This check covers POST/PATCH for single pools, but bulk_pools can still
create/update pools with team_name while multi_team is disabled (PoolBody
includes team_name). That leaves a gap where the same invalid request can
succeed/fail differently depending on endpoint. Consider adding the same
validation for the bulk pools route/service.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/variables.py:
##########
@@ -147,21 +148,33 @@ def patch_variable(
update_mask: list[str] | None = Query(None),
) -> VariableResponse:
"""Update a variable by key."""
+ if patch_body.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ "team_name cannot be set when multi_team mode is disabled. Please
contact your administrator.",
+ )
+
variable = update_orm_from_pydantic(variable_key, patch_body, update_mask,
session)
return variable
@variables_router.post(
"",
status_code=status.HTTP_201_CREATED,
- responses=create_openapi_http_exception_doc([status.HTTP_409_CONFLICT]),
+ responses=create_openapi_http_exception_doc([status.HTTP_400_BAD_REQUEST,
status.HTTP_409_CONFLICT]),
dependencies=[Depends(action_logging()),
Depends(requires_access_variable("POST"))],
)
def post_variable(
post_body: VariableBody,
session: SessionDep,
) -> VariableResponse:
"""Create a variable."""
+ if post_body.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ "team_name cannot be set when multi_team mode is disabled. Please
contact your administrator.",
+ )
Review Comment:
Similar to pools/connections, bulk_variables still accepts VariableBody
entries with team_name when multi_team is disabled. If the goal is “ensure
multi_team is enabled when team_name is provided” across the APIs, the bulk
endpoint should enforce the same rule (route-level or in BulkVariableService).
--
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]
