Copilot commented on code in PR #63351:
URL: https://github.com/apache/airflow/pull/63351#discussion_r3066495821
##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -40,6 +40,20 @@
from airflow.utils.session import NEW_SESSION, provide_session
log = logging.getLogger(__name__)
+
+
+class ConnectionFieldDecryptionError(AirflowException):
+ """Raised when an encrypted connection field cannot be decrypted."""
+
+ def __init__(self, conn_id: str | None, field_name: str):
+ self.conn_id = conn_id
+ self.field_name = field_name
+ super().__init__(
+ f"Failed to decrypt {field_name} for connection {conn_id!r}. "
+ "This may happen after migrating with a different Fernet key."
+ )
Review Comment:
Defining the exception in `airflow.models.connection` and importing it from
`airflow.api_fastapi.common.exceptions` couples API-layer code to the ORM model
import path (and may increase import-time side effects for the API server).
Prefer moving `ConnectionFieldDecryptionError` to a more neutral module (e.g.
`airflow.exceptions` or a small `airflow.models.exceptions`) and importing it
from there.
##########
airflow-core/src/airflow/api_fastapi/common/exceptions.py:
##########
@@ -122,4 +126,57 @@ def exception_handler(self, request: Request, exc:
DeserializationError):
)
-ERROR_HANDLERS: list[BaseErrorHandler] = [_UniqueConstraintErrorHandler(),
DagErrorHandler()]
+def _connection_decryption_error_response(detail: str) -> JSONResponse:
+ return JSONResponse(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
content={"detail": detail})
+
+
+class
ConnectionFieldDecryptionErrorHandler(BaseErrorHandler[ConnectionFieldDecryptionError]):
+ """Handle undecryptable connection fields in API responses."""
+
+ def __init__(self):
+ super().__init__(ConnectionFieldDecryptionError)
+
+ def exception_handler(self, request: Request, exc:
ConnectionFieldDecryptionError):
+ """Handle connection field decryption errors."""
+ return _connection_decryption_error_response(str(exc))
+
+
+class
ResponseValidationErrorHandler(BaseErrorHandler[ResponseValidationError]):
+ """Handle response validation errors caused by undecryptable connection
fields."""
+
+ def __init__(self):
+ super().__init__(ResponseValidationError)
+
+ def exception_handler(self, request: Request, exc:
ResponseValidationError):
+ """Handle response validation errors."""
+ if "/connections" in request.url.path:
+ return _connection_decryption_error_response(
+ "Failed to serialize connection because an encrypted field
could not be decrypted. "
+ "This may happen after migrating with a different Fernet key."
+ )
+ raise exc
+
+
+class
PydanticSerializationErrorHandler(BaseErrorHandler[PydanticSerializationError]):
+ """Handle serialization errors caused by undecryptable connection
fields."""
+
+ def __init__(self):
+ super().__init__(PydanticSerializationError)
+
+ def exception_handler(self, request: Request, exc:
PydanticSerializationError):
+ """Handle serialization errors."""
+ if "/connections" in request.url.path and
"ConnectionFieldDecryptionError" in str(exc):
+ return _connection_decryption_error_response(
+ "Failed to serialize connection because an encrypted field
could not be decrypted. "
+ "This may happen after migrating with a different Fernet key."
+ )
+ raise exc
Review Comment:
Detecting the root cause via string matching
(`\"ConnectionFieldDecryptionError\" in str(exc)`) is brittle and may break
across Pydantic/FastAPI versions or with message changes. Prefer checking the
exception cause/context chain (e.g., walking `__cause__`/`__context__`) for
`ConnectionFieldDecryptionError`, and apply the same guidance as above
regarding `raise exc` vs letting the error fall through.
##########
airflow-core/src/airflow/api_fastapi/common/exceptions.py:
##########
@@ -122,4 +126,57 @@ def exception_handler(self, request: Request, exc:
DeserializationError):
)
-ERROR_HANDLERS: list[BaseErrorHandler] = [_UniqueConstraintErrorHandler(),
DagErrorHandler()]
+def _connection_decryption_error_response(detail: str) -> JSONResponse:
+ return JSONResponse(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
content={"detail": detail})
+
+
+class
ConnectionFieldDecryptionErrorHandler(BaseErrorHandler[ConnectionFieldDecryptionError]):
+ """Handle undecryptable connection fields in API responses."""
+
+ def __init__(self):
+ super().__init__(ConnectionFieldDecryptionError)
+
+ def exception_handler(self, request: Request, exc:
ConnectionFieldDecryptionError):
+ """Handle connection field decryption errors."""
+ return _connection_decryption_error_response(str(exc))
+
+
+class
ResponseValidationErrorHandler(BaseErrorHandler[ResponseValidationError]):
+ """Handle response validation errors caused by undecryptable connection
fields."""
+
+ def __init__(self):
+ super().__init__(ResponseValidationError)
+
+ def exception_handler(self, request: Request, exc:
ResponseValidationError):
+ """Handle response validation errors."""
+ if "/connections" in request.url.path:
+ return _connection_decryption_error_response(
+ "Failed to serialize connection because an encrypted field
could not be decrypted. "
+ "This may happen after migrating with a different Fernet key."
+ )
+ raise exc
Review Comment:
Re-raising `exc` from inside an exception handler can lead to the same
handler being invoked again (depending on how handlers are
registered/iterated), potentially causing an infinite loop or masking the
original error handling flow. Instead, let the error fall through to the
default behavior by returning `None` (if your handler chain supports it) or
re-raising a different mapped exception (e.g., `HTTPException`) when you intend
to handle it. If you need to preserve traceback, use bare `raise` rather than
`raise exc`.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -197,6 +197,64 @@ def
test_get_should_not_overmask_short_password_value_in_extra(self, test_client
assert body["login"] == "a"
assert body["extra"] == '{"key": "value"}'
+ def test_get_connection_with_undecryptable_password(self, test_client,
session):
+ """Connection with an undecryptable password should return a clear 500
error."""
+ conn = Connection(conn_id="bad_fernet_conn", conn_type="generic")
+ session.add(conn)
+ session.flush()
+ # Simulate a password encrypted with a different Fernet key by writing
+ # directly to _password and marking as encrypted.
+ session.execute(
+ Connection.__table__.update()
+ .where(Connection.__table__.c.conn_id == "bad_fernet_conn")
+ .values(password="not-a-valid-fernet-token", is_encrypted=True)
+ )
+ session.expire_all()
+ session.commit()
+
+ response = test_client.get("/connections/bad_fernet_conn")
+ assert response.status_code == 500
+ assert "different Fernet key" in response.json()["detail"]
Review Comment:
The PR description says `Connection.get_password()` / `get_extra()` now
'catch decryption failures, log a warning, and return `None`', which would
typically allow these endpoints to succeed (200) with `password`/`extra`
omitted or null. However, the implementation raises
`ConnectionFieldDecryptionError` and the tests assert a 500 response. Please
align either (a) the implementation/tests to return `None` (and adjust API
behavior accordingly) or (b) the PR description to reflect the new behavior of
returning a clearer 500.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py:
##########
@@ -197,6 +197,64 @@ def
test_get_should_not_overmask_short_password_value_in_extra(self, test_client
assert body["login"] == "a"
assert body["extra"] == '{"key": "value"}'
+ def test_get_connection_with_undecryptable_password(self, test_client,
session):
+ """Connection with an undecryptable password should return a clear 500
error."""
Review Comment:
The PR description says `Connection.get_password()` / `get_extra()` now
'catch decryption failures, log a warning, and return `None`', which would
typically allow these endpoints to succeed (200) with `password`/`extra`
omitted or null. However, the implementation raises
`ConnectionFieldDecryptionError` and the tests assert a 500 response. Please
align either (a) the implementation/tests to return `None` (and adjust API
behavior accordingly) or (b) the PR description to reflect the new behavior of
returning a clearer 500.
--
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]
