Copilot commented on code in PR #64498:
URL: https://github.com/apache/airflow/pull/64498#discussion_r3025333075
##########
airflow-core/src/airflow/cli/commands/connection_command.py:
##########
@@ -427,10 +427,13 @@ def _import_helper(file_path: str, overwrite: bool) ->
None:
# The conn_ids match, but the PK of the new entry must also be
the same as the old
conn.id = existing_conn_id
-
- session.merge(conn)
- session.commit()
- print(f"Imported connection {conn_id}")
+ try:
+ session.merge(conn)
+ session.commit()
+ print(f"Imported connection {conn_id}")
+ except Exception as e:
+ print(f"Failed to import connection {conn_id}: {e!r}")
+ session.rollback()
Review Comment:
Printing the raw exception repr (`{e!r}`) to stdout is likely to leak
sensitive connection details (SQLAlchemy errors can include bound parameters;
validation errors may include URIs/passwords). Since `print()` bypasses
Airflow’s log masking, please sanitize/redact the error output (e.g., use
`airflow._shared.secrets_masker.redact(str(e))` or only print the exception
class + a generic message) before displaying it.
##########
airflow-core/src/airflow/cli/commands/connection_command.py:
##########
@@ -427,10 +427,13 @@ def _import_helper(file_path: str, overwrite: bool) ->
None:
# The conn_ids match, but the PK of the new entry must also be
the same as the old
conn.id = existing_conn_id
-
- session.merge(conn)
- session.commit()
- print(f"Imported connection {conn_id}")
+ try:
+ session.merge(conn)
+ session.commit()
+ print(f"Imported connection {conn_id}")
+ except Exception as e:
+ print(f"Failed to import connection {conn_id}: {e!r}")
+ session.rollback()
Review Comment:
Catching broad `Exception` here makes it hard to distinguish
expected/import-related failures (e.g. `IntegrityError`, `ValueError`) from
programming/operational errors (e.g. `KeyboardInterrupt`, `SystemExit`,
`MemoryError`). Please narrow the exception types to the ones that are expected
during import (at least SQLAlchemy `SQLAlchemyError` / `IntegrityError`) and
let truly unexpected exceptions propagate, similar to other CLI commands.
##########
airflow-core/src/airflow/cli/commands/connection_command.py:
##########
@@ -427,10 +427,13 @@ def _import_helper(file_path: str, overwrite: bool) ->
None:
# The conn_ids match, but the PK of the new entry must also be
the same as the old
conn.id = existing_conn_id
-
- session.merge(conn)
- session.commit()
- print(f"Imported connection {conn_id}")
+ try:
+ session.merge(conn)
+ session.commit()
+ print(f"Imported connection {conn_id}")
+ except Exception as e:
+ print(f"Failed to import connection {conn_id}: {e!r}")
+ session.rollback()
Review Comment:
`_import_helper` is inside `with create_session() as session:`, whose
context manager will call `session.commit()` again on exit after the loop. With
per-item `commit()` inside the loop, this extra commit is unnecessary and can
re-raise/commit a partially-failed transaction state, potentially causing the
overall CLI command to still fail even though individual errors were caught.
Consider using `create_session()` with an explicit `session.begin_nested()` per
item, or refactor to use `settings.Session()`/`provide_session` so you control
the final commit behavior (or avoid per-item commits and rely on the context
manager + flush/rollback handling).
```suggestion
with session.begin_nested() as nested:
try:
session.merge(conn)
except Exception as e:
print(f"Failed to import connection {conn_id}: {e!r}")
nested.rollback()
continue
print(f"Imported connection {conn_id}")
```
##########
airflow-core/src/airflow/cli/commands/connection_command.py:
##########
@@ -427,10 +427,13 @@ def _import_helper(file_path: str, overwrite: bool) ->
None:
# The conn_ids match, but the PK of the new entry must also be
the same as the old
conn.id = existing_conn_id
-
- session.merge(conn)
- session.commit()
- print(f"Imported connection {conn_id}")
+ try:
+ session.merge(conn)
+ session.commit()
+ print(f"Imported connection {conn_id}")
+ except Exception as e:
+ print(f"Failed to import connection {conn_id}: {e!r}")
+ session.rollback()
Review Comment:
The new behavior (continuing after a DB/merge/commit failure) isn’t covered
by existing CLI import tests. Please add a regression test in
`airflow-core/tests/unit/cli/commands/test_connection_command.py` that forces
`session.commit()` or `session.merge()` to raise (e.g., patch or insert invalid
data to trigger an `IntegrityError`) and asserts: (1) the failure message is
printed, (2) subsequent connections are still imported, and (3) the command
does not raise.
##########
airflow-core/src/airflow/cli/commands/connection_command.py:
##########
@@ -427,10 +427,13 @@ def _import_helper(file_path: str, overwrite: bool) ->
None:
# The conn_ids match, but the PK of the new entry must also be
the same as the old
conn.id = existing_conn_id
-
- session.merge(conn)
- session.commit()
- print(f"Imported connection {conn_id}")
+ try:
+ session.merge(conn)
+ session.commit()
+ print(f"Imported connection {conn_id}")
Review Comment:
PR description mentions reverting changes in `variable_command.py`, but this
PR diff only updates `connection_command.py`. Please either include the
`variable_command.py` change in this PR or update the description to match the
actual changes.
--
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]
