Copilot commented on code in PR #63995: URL: https://github.com/apache/airflow/pull/63995#discussion_r3066487756
########## airflow-core/src/airflow/api_fastapi/execution_api/datamodels/common.py: ########## @@ -0,0 +1,26 @@ +# 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 airflow.api_fastapi.core_api.base import BaseModel +from pydantic import Field + + Review Comment: Import order in this new module is inconsistent with the rest of execution_api datamodels and likely to fail isort/ruff (third-party imports like `pydantic` should come before first-party `airflow` imports; e.g. `execution_api/datamodels/connection.py:20-22`). Please reorder the imports accordingly. ```suggestion from pydantic import Field from airflow.api_fastapi.core_api.base import BaseModel ``` ########## airflow-core/src/airflow/api_fastapi/execution_api/routes/variables.py: ########## @@ -110,6 +110,7 @@ def put_variable( def delete_variable( variable_key: Annotated[str, Path(min_length=1)], team_name: Annotated[str | None, Depends(get_team_name_dep)], -): +) -> MessageResponse: """Delete an Airflow Variable.""" Variable.delete(key=variable_key, team_name=team_name) Review Comment: This endpoint always returns a “successfully deleted” message even when nothing is deleted (Variable.delete returns a row count and does not raise when the key is missing; the unit test `test_should_not_delete_variable` expects the DB to be unchanged). Please use the return value to either (a) return a message that reflects idempotent behavior, or (b) return 404 when no rows are deleted (which would also better match the router’s documented 404 response). ```suggestion deleted_rows = Variable.delete(key=variable_key, team_name=team_name) if not deleted_rows: raise HTTPException( status.HTTP_404_NOT_FOUND, detail={ "reason": "not_found", "message": f"Variable with key '{variable_key}' not found", }, ) ``` ########## airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_variables.py: ########## @@ -261,7 +261,7 @@ def test_should_delete_variable(self, client, session, keys_to_create, key_to_de response = client.delete(f"/execution/variables/{key_to_delete}") - assert response.status_code == 204 + assert response.status_code == 200 Review Comment: The test was updated to assert the new 200 status code, but it doesn’t validate the new structured response body. Since the endpoint now returns a `MessageResponse`, please assert the JSON payload (e.g. that it contains the expected `message`) to prevent regressions in the schema this PR is standardizing. ########## airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_variables.py: ########## @@ -274,7 +274,7 @@ def test_should_not_delete_variable(self, client, session): response = client.delete("/execution/variables/non_existent_key") - assert response.status_code == 204 + assert response.status_code == 200 Review Comment: The test was updated to assert the new 200 status code, but it doesn’t validate the response body returned by the endpoint. Please also assert the JSON payload (at least the standardized `message` field) so the schema change is actually covered by tests. -- 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]
