Copilot commented on code in PR #62234:
URL: https://github.com/apache/airflow/pull/62234#discussion_r2926164423
##########
providers/fab/src/airflow/providers/fab/auth_manager/models/__init__.py:
##########
@@ -372,19 +391,19 @@ class RegisterUser(Model):
registration_hash: Mapped[str | None] = mapped_column(String(256))
+_idx_ab_user_username = Index("idx_ab_user_username",
func.lower(User.__table__.c.username), unique=True)
+_idx_ab_register_user_username = Index(
+ "idx_ab_register_user_username",
func.lower(RegisterUser.__table__.c.username), unique=True
+)
+
+
@event.listens_for(User.__table__, "before_create")
def add_index_on_ab_user_username_postgres(table, conn, **kw):
if conn.dialect.name != "postgresql":
- return
- index_name = "idx_ab_user_username"
- if not any(table_index.name == index_name for table_index in
table.indexes):
- table.indexes.add(Index(index_name, func.lower(table.c.username),
unique=True))
+ table.indexes.discard(_idx_ab_user_username)
@event.listens_for(RegisterUser.__table__, "before_create")
def add_index_on_ab_register_user_username_postgres(table, conn, **kw):
if conn.dialect.name != "postgresql":
- return
- index_name = "idx_ab_register_user_username"
- if not any(table_index.name == index_name for table_index in
table.indexes):
- table.indexes.add(Index(index_name, func.lower(table.c.username),
unique=True))
+ table.indexes.discard(_idx_ab_register_user_username)
Review Comment:
These `before_create` listeners call `table.indexes.discard(...)` for
non-PostgreSQL dialects. Since `table.indexes` is shared metadata, this
permanently removes the index from the model for the lifetime of the process,
which can lead to inconsistent behavior if the metadata is reused (e.g. tests
that create schemas on multiple dialects in one process, or subsequent
`create_all` on a different engine). Prefer a dialect-scoped DDL approach that
does not mutate global table metadata permanently (for example: only
attach/create the index for PostgreSQL, or remove it in `before_create` and
re-add it in a corresponding `after_create`).
##########
providers/fab/src/airflow/providers/fab/auth_manager/cli_commands/db_command.py:
##########
@@ -29,7 +29,9 @@ def resetdb(args):
print(f"DB: {settings.engine.url!r}")
if not (args.yes or input("This will drop existing tables if they exist.
Proceed? (y/n)").upper() == "Y"):
raise SystemExit("Cancelled")
- FABDBManager(settings.Session()).resetdb(skip_init=args.skip_init)
+ FABDBManager(settings.Session()).resetdb(
+ skip_init=args.skip_init, use_migration_files=args.use_migration_files
Review Comment:
`args.use_migration_files` is accessed unconditionally, but the FAB CLI
intentionally omits this argument on older Airflow versions (see
`_USE_MIGRATION_FILES_ARG` in the CLI definition). This will raise
`AttributeError` when the option is unavailable. Use `getattr(args,
"use_migration_files", False)` (and pass that through) to keep the command
backward-compatible.
```suggestion
skip_init=args.skip_init,
use_migration_files=getattr(args, "use_migration_files", False),
```
--
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]
