Copilot commented on code in PR #63994:
URL: https://github.com/apache/airflow/pull/63994#discussion_r3025331793
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/variables.py:
##########
@@ -24,6 +24,7 @@
from airflow._shared.secrets_masker import redact
from airflow.api_fastapi.core_api.base import BaseModel, StrictBaseModel,
make_partial_model
+from airflow.configuration import conf
Review Comment:
`@model_validator` is used but not imported in this module, which will raise
a `NameError` at import time. Add the appropriate Pydantic import (consistent
with the other datamodel files).
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py:
##########
@@ -91,3 +98,9 @@ class PoolBody(BasePool, StrictBaseModel):
description: str | None = None
include_deferred: bool = False
team_name: str | None = Field(max_length=50, default=None)
+
+ @model_validator(mode="after")
+ def validate_team_name(self) -> PoolBody:
+ if self.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise ValueError("team_name cannot be set when multi_team mode is
disabled")
+ return self
Review Comment:
The same multi-team gating validator logic is duplicated across multiple
models (here in both `PoolPatchBody` and `PoolBody`, and similarly in other
datamodel modules). Consider extracting a small shared helper/mixin (or a
shared validator function) to centralize the behavior and message, reducing the
chance of future inconsistencies and making updates easier. This is optional,
but would improve maintainability.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py:
##########
@@ -83,6 +84,12 @@ class PoolPatchBody(StrictBaseModel):
include_deferred: bool | None = None
team_name: str | None = Field(max_length=50, default=None)
+ @model_validator(mode="after")
+ def validate_team_name(self) -> PoolPatchBody:
+ if self.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise ValueError("team_name cannot be set when multi_team mode is
disabled")
+ return self
Review Comment:
The same multi-team gating validator logic is duplicated across multiple
models (here in both `PoolPatchBody` and `PoolBody`, and similarly in other
datamodel modules). Consider extracting a small shared helper/mixin (or a
shared validator function) to centralize the behavior and message, reducing the
chance of future inconsistencies and making updates easier. This is optional,
but would improve maintainability.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/variables.py:
##########
@@ -60,6 +61,12 @@ class VariableBody(StrictBaseModel):
description: str | None = Field(default=None)
team_name: str | None = Field(max_length=50, default=None)
+ @model_validator(mode="after")
+ def validate_team_name(self) -> VariableBody:
+ if self.team_name is not None and not conf.getboolean("core",
"multi_team"):
+ raise ValueError("team_name cannot be set when multi_team mode is
disabled")
+ return self
Review Comment:
`@model_validator` is used but not imported in this module, which will raise
a `NameError` at import time. Add the appropriate Pydantic import (consistent
with the other datamodel files).
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py:
##########
@@ -416,6 +417,24 @@ def test_patch_pool3_should_respond_200(self, test_client,
session):
assert response.json() == expected_response
check_last_log(session, dag_id=None, event="patch_pool",
logical_date=None)
+ def test_patch_pool_rejects_team_name_when_multi_team_disabled(self,
test_client):
+ self.create_pools()
+ with conf_vars({("core", "multi_team"): "False"}):
+ response = test_client.patch(
+ f"/pools/{POOL2_NAME}",
+ json={
+ "name": POOL2_NAME,
+ "slots": POOL2_SLOT,
+ "include_deferred": POOL2_INCLUDE_DEFERRED,
+ "team_name": "test",
+ },
+ )
+ assert response.status_code == 422
+ assert (
+ response.json()["detail"][0]["msg"]
+ == "Value error, team_name cannot be set when multi_team mode is
disabled"
Review Comment:
These tests assert the full Pydantic-formatted `msg` including the `\"Value
error,\"` prefix, which is relatively brittle across Pydantic/FastAPI versions
and error formatting changes. Prefer asserting on the stable portion (e.g.,
that the message contains/endswith `\"team_name cannot be set when multi_team
mode is disabled\"`), or assert on the error `type` plus the message substring
to reduce churn.
```suggestion
"team_name cannot be set when multi_team mode is disabled"
in response.json()["detail"][0]["msg"]
```
--
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]
