ColtenOuO opened a new pull request, #71245: URL: https://github.com/apache/airflow/pull/71245
### Sumarry Split out of the [#71011](https://github.com/apache/airflow/pull/71011) review. Remove the unreachable 404 from the create Variable endpoint ### The branch cannot be reached ```python Variable.set(**post_body.model_dump(), session=session) variable = session.scalar(select(Variable).where(Variable.key == post_body.key).limit(1)) if variable is None: raise HTTPException(status.HTTP_404_NOT_FOUND, ...) ``` `Variable.set()` upserts the row through the same session the read-back then queries, so the read cannot come up empty. The status is also wrong on its own terms: a `404` on a create endpoint tells a caller the variable they just created was not found. That left the endpoint with a choice between publishing a response it can never return and leaving its OpenAPI spec incomplete — which is what surfaced during #71011. ### Why it could not simply be deleted It was added in [#56813](https://github.com/apache/airflow/pull/56813) as part of the SQLAlchemy 2 typing cleanup. SQLAlchemy types the method as returning an optional: ```python @overload def scalar(self, statement: TypedReturnsRows[Tuple[_T]], ...) -> Optional[_T]: ... ``` So `variable` is `Variable | None`, and returning it fails: ``` error: Incompatible return value type (got "Variable | None", expected "VariableResponse") [return-value] ``` The `raise` is what fixes that — it never returns, so mypy narrows the value to `Variable` below it. Dropping the branch on its own puts the original error back. ### The change ```python return session.scalars(select(Variable).where(Variable.key == post_body.key)).one() ``` `ScalarResult.one()` is typed `-> _R`, not `-> Optional[_R]`, because its contract already is "exactly one row, or raise". The invariant moves into the query instead of being asserted by control flow, so nothing has to narrow a type and no HTTP status is spent doing it. If the invariant were ever violated, `NoResultFound` surfaces as a `500`, which is the honest answer for a write that silently did not happen. `.limit(1)` goes with it: `Variable.key` is `unique=True`, so `.one()` is already exact and the limit would only contradict it. ### Behaviour Unchanged — the removed branch was unreachable. No spec change either: the `404` was never declared in `responses=`, so the generated OpenAPI spec and UI client are untouched. -- 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]
