kaxil commented on code in PR #66073: URL: https://github.com/apache/airflow/pull/66073#discussion_r3170621778
########## airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_state.py: ########## @@ -0,0 +1,121 @@ +# 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. +""" +Execution API routes for asset state. + +Asset state is keyed by asset *name* (not integer id) in the URL — asset names +are unique, and callers (task SDK accessors) have the name from their Asset +object without needing a DB lookup. The route resolves name → asset_id +internally for the state backend scope. + +Per-task asset registration checks are intentionally not implemented here +(deferred to AIP-93 — see TODO comment below). +""" + +from __future__ import annotations + +from typing import Annotated + +from cadwyn import VersionedAPIRouter +from fastapi import HTTPException, Path, status +from sqlalchemy import select + +from airflow._shared.state import AssetScope +from airflow.api_fastapi.common.db.common import SessionDep +from airflow.api_fastapi.execution_api.datamodels.asset_state import ( + AssetStatePutBody, + AssetStateResponse, +) +from airflow.models.asset import AssetModel +from airflow.state import get_state_backend + +# TODO(AIP-103): enforce that the requesting task is registered with the asset +# (via task_inlet_asset_reference or task_outlet_asset_reference) before +# allowing reads/writes. Currently any task with a valid execution token can +# access any asset's state — the same gap exists in /assets and /asset-events. +# Proper fix is a unified asset-registration check across all asset routes, +# not just here. +router = VersionedAPIRouter( + responses={ + status.HTTP_401_UNAUTHORIZED: {"description": "Unauthorized"}, + status.HTTP_404_NOT_FOUND: {"description": "Not found"}, + }, +) + + +def _resolve_asset_id(name: str, session: SessionDep) -> int: + """Resolve asset name → integer asset_id, 404 if not found.""" + asset_id = session.scalar(select(AssetModel.id).where(AssetModel.name == name)) Review Comment: The other execution-API asset routes filter by `AssetModel.active.has()` when resolving by name (`assets.py:43,55`, `asset_events.py:89,91`). This one doesn't, so a row in `asset` that no longer has an `asset_active` partner (asset removed from its DAG but row still present) still resolves to an `asset_id` and lets workers read/write state against it. Worth matching the existing pattern. ########## airflow-core/src/airflow/api_fastapi/execution_api/routes/task_state.py: ########## @@ -0,0 +1,127 @@ +# 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 typing import Annotated +from uuid import UUID + +from cadwyn import VersionedAPIRouter +from fastapi import HTTPException, Path, Query, Security, status +from sqlalchemy.orm import Session + +from airflow._shared.state import TaskScope +from airflow.api_fastapi.common.db.common import SessionDep +from airflow.api_fastapi.execution_api.datamodels.task_state import ( + TaskStatePutBody, + TaskStateResponse, +) +from airflow.api_fastapi.execution_api.security import require_auth +from airflow.models.taskinstance import TaskInstance as TI +from airflow.state import get_state_backend + +router = VersionedAPIRouter( + responses={ + status.HTTP_401_UNAUTHORIZED: {"description": "Unauthorized"}, + status.HTTP_403_FORBIDDEN: {"description": "Access denied"}, + status.HTTP_404_NOT_FOUND: {"description": "Not found"}, + }, + dependencies=[Security(require_auth, scopes=["ti:self"])], +) + + +def _get_task_scope_for_ti(task_instance_id: UUID, session: Session) -> TaskScope: + ti = session.get(TI, task_instance_id) + if ti is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "reason": "not_found", + "message": f"Task instance {task_instance_id} not found", + }, + ) + return TaskScope(dag_id=ti.dag_id, run_id=ti.run_id, task_id=ti.task_id, map_index=ti.map_index) + + [email protected]("/{task_instance_id}/{key}") +def get_task_state( + task_instance_id: UUID, + key: Annotated[str, Path(min_length=1)], + session: SessionDep, +) -> TaskStateResponse: + """Get value for a task state.""" + scope = _get_task_scope_for_ti(task_instance_id, session) + value = get_state_backend().get(scope, key) + if value is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "reason": "not_found", + "message": f"Task state key {key!r} not found", + }, + ) + return TaskStateResponse(value=value) + + [email protected]("/{task_instance_id}/{key}", status_code=status.HTTP_204_NO_CONTENT) +def set_task_state( + task_instance_id: UUID, + key: Annotated[str, Path(min_length=1)], + body: TaskStatePutBody, + session: SessionDep, +) -> None: + """Set a task state key, creating or updating the row.""" + scope = _get_task_scope_for_ti(task_instance_id, session) + get_state_backend().set(scope, key, body.value) + + [email protected]("/{task_instance_id}/{key}", status_code=status.HTTP_204_NO_CONTENT) +def delete_task_state( + task_instance_id: UUID, + key: Annotated[str, Path(min_length=1)], + session: SessionDep, +) -> None: + """Delete a single task state key.""" + scope = _get_task_scope_for_ti(task_instance_id, session) + get_state_backend().delete(scope, key) + + [email protected]("/{task_instance_id}", status_code=status.HTTP_204_NO_CONTENT) +def clear_task_state( + task_instance_id: UUID, + session: SessionDep, + all_map_indices: Annotated[bool, Query()] = False, Review Comment: Router-level `Security(require_auth, scopes=["ti:self"])` confirms the JWT subject matches `path_params["task_instance_id"]`, but `?all_map_indices=true` then wipes state for every mapped sibling, which are different TIs with different tokens. A worker for `map_index=0` can clear state for `map_index=1..N`. Either the docstring should declare this as intentionally fleet-wide (and still safe because the caller passed an authenticated TI of the same task), or this branch needs a stronger scope (say `ti:fleet`) or a leader check (`map_index in (-1, 0)`). ########## airflow-core/src/airflow/api_fastapi/execution_api/routes/task_state.py: ########## @@ -0,0 +1,127 @@ +# 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 typing import Annotated +from uuid import UUID + +from cadwyn import VersionedAPIRouter +from fastapi import HTTPException, Path, Query, Security, status +from sqlalchemy.orm import Session + +from airflow._shared.state import TaskScope +from airflow.api_fastapi.common.db.common import SessionDep +from airflow.api_fastapi.execution_api.datamodels.task_state import ( + TaskStatePutBody, + TaskStateResponse, +) +from airflow.api_fastapi.execution_api.security import require_auth +from airflow.models.taskinstance import TaskInstance as TI +from airflow.state import get_state_backend + +router = VersionedAPIRouter( + responses={ + status.HTTP_401_UNAUTHORIZED: {"description": "Unauthorized"}, + status.HTTP_403_FORBIDDEN: {"description": "Access denied"}, + status.HTTP_404_NOT_FOUND: {"description": "Not found"}, + }, + dependencies=[Security(require_auth, scopes=["ti:self"])], +) + + +def _get_task_scope_for_ti(task_instance_id: UUID, session: Session) -> TaskScope: + ti = session.get(TI, task_instance_id) + if ti is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "reason": "not_found", + "message": f"Task instance {task_instance_id} not found", + }, + ) + return TaskScope(dag_id=ti.dag_id, run_id=ti.run_id, task_id=ti.task_id, map_index=ti.map_index) + + [email protected]("/{task_instance_id}/{key}") +def get_task_state( + task_instance_id: UUID, + key: Annotated[str, Path(min_length=1)], + session: SessionDep, +) -> TaskStateResponse: + """Get value for a task state.""" + scope = _get_task_scope_for_ti(task_instance_id, session) + value = get_state_backend().get(scope, key) + if value is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "reason": "not_found", + "message": f"Task state key {key!r} not found", + }, + ) + return TaskStateResponse(value=value) + + [email protected]("/{task_instance_id}/{key}", status_code=status.HTTP_204_NO_CONTENT) +def set_task_state( + task_instance_id: UUID, + key: Annotated[str, Path(min_length=1)], + body: TaskStatePutBody, + session: SessionDep, +) -> None: + """Set a task state key, creating or updating the row.""" + scope = _get_task_scope_for_ti(task_instance_id, session) + get_state_backend().set(scope, key, body.value) Review Comment: `_get_task_scope_for_ti` uses the route's `SessionDep`, but `get_state_backend().set(...)` opens a fresh session via `@provide_session` on `MetastoreStateBackend.set` (`state/metastore.py:81-82`). Two consequences: (a) existence check and write run in different transactions, so the TI can be deleted or its `run_id` changed between them, leading to an orphan row or FK error, and (b) you pay an extra connection per request. Passing `session=session` into the backend call closes both. Same shape on `delete_task_state`, `clear_task_state`, and all four asset_state routes. ########## airflow-core/src/airflow/api_fastapi/execution_api/routes/__init__.py: ########## @@ -52,5 +54,7 @@ authenticated_router.include_router(variables.router, prefix="/variables", tags=["Variables"]) authenticated_router.include_router(xcoms.router, prefix="/xcoms", tags=["XComs"]) authenticated_router.include_router(hitl.router, prefix="/hitlDetails", tags=["Human in the Loop"]) +authenticated_router.include_router(task_state.router, prefix="/state/ti", tags=["Task State"]) +authenticated_router.include_router(asset_state.router, prefix="/state/asset", tags=["Asset State"]) Review Comment: 8 new endpoints land here without a corresponding `endpoint(...).didnt_exist` entry in the unreleased version file (`versions/v2026_04_17.py`). A newer SDK calling these routes against an older API server won't fail cleanly through Cadwyn's version negotiation. See `contributing-docs/19_execution_api_versioning.rst`. ########## airflow-core/src/airflow/api_fastapi/execution_api/routes/task_state.py: ########## @@ -0,0 +1,127 @@ +# 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 typing import Annotated +from uuid import UUID + +from cadwyn import VersionedAPIRouter +from fastapi import HTTPException, Path, Query, Security, status +from sqlalchemy.orm import Session + +from airflow._shared.state import TaskScope +from airflow.api_fastapi.common.db.common import SessionDep +from airflow.api_fastapi.execution_api.datamodels.task_state import ( + TaskStatePutBody, + TaskStateResponse, +) +from airflow.api_fastapi.execution_api.security import require_auth +from airflow.models.taskinstance import TaskInstance as TI +from airflow.state import get_state_backend + +router = VersionedAPIRouter( + responses={ + status.HTTP_401_UNAUTHORIZED: {"description": "Unauthorized"}, + status.HTTP_403_FORBIDDEN: {"description": "Access denied"}, + status.HTTP_404_NOT_FOUND: {"description": "Not found"}, + }, + dependencies=[Security(require_auth, scopes=["ti:self"])], Review Comment: The new `ti:self` enforcement is the primary protection on these routes, but `airflow-core/tests/unit/api_fastapi/execution_api/conftest.py:56-60` overrides `require_auth` with a mock that always builds a `TIToken` whose `id` equals `path_params["task_instance_id"]`, so the mismatch branch in `security.py:185-191` is never exercised here. Worth a regression test that uses the real `require_auth` and asserts a token bound to a different TI gets a 403 against this router. Same gap means there's currently no asset_state coverage at all for the auth model that's planned to land via the AIP-93 follow-up. ########## airflow-core/src/airflow/state/__init__.py: ########## @@ -36,3 +36,14 @@ def resolve_state_backend() -> type[BaseStateBackend]: f"Your custom state backend `{clazz.__name__}` is not a subclass of `BaseStateBackend`." ) return clazz + + +_backend_instance: BaseStateBackend | None = None + + +def get_state_backend() -> BaseStateBackend: + """Return a cached instance of the configured state backend.""" + global _backend_instance + if _backend_instance is None: + _backend_instance = resolve_state_backend()() Review Comment: Two threads racing through `if _backend_instance is None` can each call `resolve_state_backend()()`; whichever assigns last wins, the other gets GC'd. Mostly harmless for `MetastoreStateBackend`, but a custom backend with constructor side effects (file handle, network client, registered metrics, schema-creation hook) would double-init under load. A small `threading.Lock`, or wrapping a `functools.cache`'d helper, closes it. ########## airflow-core/src/airflow/api_fastapi/execution_api/routes/asset_state.py: ########## @@ -0,0 +1,121 @@ +# 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. +""" +Execution API routes for asset state. + +Asset state is keyed by asset *name* (not integer id) in the URL — asset names +are unique, and callers (task SDK accessors) have the name from their Asset +object without needing a DB lookup. The route resolves name → asset_id +internally for the state backend scope. + +Per-task asset registration checks are intentionally not implemented here +(deferred to AIP-93 — see TODO comment below). +""" + +from __future__ import annotations + +from typing import Annotated + +from cadwyn import VersionedAPIRouter +from fastapi import HTTPException, Path, status +from sqlalchemy import select + +from airflow._shared.state import AssetScope +from airflow.api_fastapi.common.db.common import SessionDep +from airflow.api_fastapi.execution_api.datamodels.asset_state import ( + AssetStatePutBody, + AssetStateResponse, +) +from airflow.models.asset import AssetModel +from airflow.state import get_state_backend + +# TODO(AIP-103): enforce that the requesting task is registered with the asset +# (via task_inlet_asset_reference or task_outlet_asset_reference) before +# allowing reads/writes. Currently any task with a valid execution token can +# access any asset's state — the same gap exists in /assets and /asset-events. +# Proper fix is a unified asset-registration check across all asset routes, +# not just here. +router = VersionedAPIRouter( + responses={ + status.HTTP_401_UNAUTHORIZED: {"description": "Unauthorized"}, + status.HTTP_404_NOT_FOUND: {"description": "Not found"}, + }, +) + + +def _resolve_asset_id(name: str, session: SessionDep) -> int: + """Resolve asset name → integer asset_id, 404 if not found.""" + asset_id = session.scalar(select(AssetModel.id).where(AssetModel.name == name)) + if asset_id is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={"reason": "not_found", "message": f"Asset {name!r} not found"}, + ) + return asset_id + + [email protected]("/{name}/{key}") +def get_asset_state( + name: Annotated[str, Path(min_length=1)], + key: Annotated[str, Path(min_length=1)], + session: SessionDep, +) -> AssetStateResponse: + """Get an asset state value.""" + asset_id = _resolve_asset_id(name, session) + value = get_state_backend().get(AssetScope(asset_id=asset_id), key) + if value is None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={ + "reason": "not_found", + "message": f"Asset state key {key!r} not found", + }, + ) + return AssetStateResponse(value=value) + + [email protected]("/{name}/{key}", status_code=status.HTTP_204_NO_CONTENT) +def set_asset_state( + name: Annotated[str, Path(min_length=1)], + key: Annotated[str, Path(min_length=1)], + body: AssetStatePutBody, + session: SessionDep, +) -> None: + """Set an asset state value.""" + asset_id = _resolve_asset_id(name, session) + get_state_backend().set(AssetScope(asset_id=asset_id), key, body.value) Review Comment: Mirrors the task_state comment: `_resolve_asset_id` uses the route's `SessionDep`, then `get_state_backend().set(...)` opens a fresh session via `@provide_session`. The PUT/DELETE/clear handlers (lines 100, 110, 120) all share this shape. Passing `session=session` into the backend call keeps the lookup and the write in one transaction. ########## airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_state.py: ########## @@ -0,0 +1,238 @@ +# 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 typing import TYPE_CHECKING +from uuid import uuid4 + +import pytest +from sqlalchemy import delete, select + +from airflow._shared.timezones import timezone +from airflow.models.dagrun import DagRun +from airflow.models.task_state import TaskStateModel +from airflow.utils.session import create_session + +if TYPE_CHECKING: + from fastapi.testclient import TestClient + + from tests_common.pytest_plugin import CreateTaskInstance + +pytestmark = pytest.mark.db_test + + [email protected](autouse=True) +def reset_state_tables(): + with create_session() as session: + session.execute(delete(TaskStateModel)) + session.execute(delete(DagRun)) + + +def _api_url(ti_id, key: str | None = None) -> str: + base = f"/execution/state/ti/{ti_id}" + return f"{base}/{key}" if key else base + + +class TestGetTaskState: + def test_get_returns_value(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + client.put(_api_url(ti.id, "job_id"), json={"value": "spark_001"}) + + response = client.get(_api_url(ti.id, "job_id")) + + assert response.status_code == 200 + assert response.json() == {"value": "spark_001"} + + def test_get_missing_key_returns_404(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + + response = client.get(_api_url(ti.id, "never_set")) + + assert response.status_code == 404 + assert response.json()["detail"]["reason"] == "not_found" + + def test_get_missing_ti_returns_404(self, client: TestClient): + response = client.get(_api_url(uuid4(), "any_key")) + + assert response.status_code == 404 + assert "Task instance" in response.json()["detail"]["message"] + + +class TestPutTaskState: + def test_put_creates_row(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + + response = client.put(_api_url(ti.id, "job_id"), json={"value": "spark_001"}) + + assert response.status_code == 204 + with create_session() as session: + row = session.scalar( + select(TaskStateModel).where( + TaskStateModel.dag_id == ti.dag_id, + TaskStateModel.run_id == ti.run_id, + TaskStateModel.task_id == ti.task_id, + TaskStateModel.key == "job_id", + ) + ) + assert row is not None + assert row.value == "spark_001" + + def test_put_overwrites_existing(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + client.put(_api_url(ti.id, "job_id"), json={"value": "spark_001"}) + + response = client.put(_api_url(ti.id, "job_id"), json={"value": "spark_002"}) + + assert response.status_code == 204 + assert client.get(_api_url(ti.id, "job_id")).json() == {"value": "spark_002"} + + def test_put_empty_body_returns_422(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + + response = client.put(_api_url(ti.id, "job_id"), json={}) + + assert response.status_code == 422 + + def test_put_extra_field_returns_422(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + + response = client.put(_api_url(ti.id, "job_id"), json={"value": "x", "extra": "y"}) + + assert response.status_code == 422 + + def test_put_null_value_returns_422(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + + response = client.put(_api_url(ti.id, "job_id"), json={"value": None}) + + assert response.status_code == 422 + + def test_put_missing_ti_returns_404(self, client: TestClient): + response = client.put(_api_url(uuid4(), "job_id"), json={"value": "x"}) + + assert response.status_code == 404 + + +class TestDeleteTaskState: + def test_delete_removes_key(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + client.put(_api_url(ti.id, "job_id"), json={"value": "spark_001"}) + + response = client.delete(_api_url(ti.id, "job_id")) + + assert response.status_code == 204 + assert client.get(_api_url(ti.id, "job_id")).status_code == 404 + + def test_delete_missing_key_is_noop(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + + response = client.delete(_api_url(ti.id, "never_existed")) + + assert response.status_code == 204 + + def test_delete_only_targets_one_key(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + client.put(_api_url(ti.id, "job_id"), json={"value": "a"}) + client.put(_api_url(ti.id, "checkpoint"), json={"value": "b"}) + + client.delete(_api_url(ti.id, "job_id")) + + assert client.get(_api_url(ti.id, "job_id")).status_code == 404 + assert client.get(_api_url(ti.id, "checkpoint")).json() == {"value": "b"} + + +class TestClearTaskState: + def test_clear_removes_all_keys(self, client: TestClient, create_task_instance: CreateTaskInstance): + ti = create_task_instance() + for k, v in [("job_id", "a"), ("checkpoint", "b"), ("retry_count", "c")]: + client.put(_api_url(ti.id, k), json={"value": v}) + + response = client.delete(_api_url(ti.id)) + + assert response.status_code == 204 + with create_session() as session: + count = session.scalar( + select(TaskStateModel) + .where(TaskStateModel.dag_id == ti.dag_id, TaskStateModel.task_id == ti.task_id) + .with_only_columns(TaskStateModel.key) + ) + assert count is None Review Comment: `count` is `session.scalar(select(...).with_only_columns(TaskStateModel.key))`, so it returns the first matching `key` value (or `None` when empty), not a count. The test passes only because the table is empty after `clear`; it would also pass spuriously if a row's `key` were ever `None`, and it'd fail with a confusing message on any non-empty wrong result. `session.scalar(select(func.count()).select_from(TaskStateModel).where(...))` plus `assert count == 0` matches the name and the intent. -- 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]
