kaxil commented on code in PR #62234:
URL: https://github.com/apache/airflow/pull/62234#discussion_r2914981222
##########
airflow-core/src/airflow/utils/db.py:
##########
@@ -1291,9 +1305,40 @@ def _resetdb_default(session: Session) -> None:
external_db_manager.drop_tables(session, connection)
+def _drop_remaining_tables() -> None:
+ """
+ Drop any tables still remaining in the database after the normal reset.
+
+ The squashed migration (0000_2_6_2) creates tables like FAB ab_* tables
that
+ are now managed by external auth managers. When the default auth manager
+ (SimpleAuthManager) has no DB manager, those tables are not dropped by
+ external_db_manager.drop_tables(). This function reflects the actual
database
+ and drops anything left over so the migration has a clean slate.
+ """
+ from sqlalchemy import MetaData
+
+ engine = settings.get_engine()
+ metadata = MetaData()
+ metadata.reflect(bind=engine)
+ if not metadata.tables:
+ return
+ remaining = list(metadata.tables.keys())
Review Comment:
`_drop_remaining_tables` reflects and drops *all* tables in the database
without any filtering. If there are non-Airflow tables in a shared schema, this
would drop those too. Probably fine for the dev workflow this targets, but
worth a note or a safety check.
Also, on MySQL with AUTOCOMMIT isolation, `resetdb` goes through
`_resetdb_mysql` which manages its own session/connection lifecycle.
`_drop_remaining_tables` creates a separate engine connection outside that
flow. Does this work correctly with MySQL's locking behavior?
##########
airflow-core/src/airflow/cli/commands/db_command.py:
##########
@@ -135,6 +135,7 @@ def run_db_migrate_command(args, command,
revision_heads_map: dict[str, str]):
to_revision=to_revision,
from_revision=from_revision,
show_sql_only=args.show_sql_only,
+ use_migration_files=getattr(args, "use_migration_files", False),
Review Comment:
`run_db_migrate_command` now passes `use_migration_files` to `command()`,
but this function is also called by `fab-db migrate` and `db-manager migrate`
where `command` is `FABDBManager.upgradedb` or `BaseDBManager.upgradedb`.
Neither of those accepts a `use_migration_files` kwarg, so `fab-db migrate` and
`db-manager migrate` will crash with `TypeError: upgradedb() got an unexpected
keyword argument 'use_migration_files'`.
The `getattr` guards the `args` side correctly, but the value is still
unconditionally forwarded to the command callable. One option would be to only
pass it when the attribute is truthy, or add `**kwargs` to
`BaseDBManager.upgradedb`.
##########
providers/fab/src/airflow/providers/fab/auth_manager/models/__init__.py:
##########
@@ -38,15 +39,23 @@
func,
select,
)
-from sqlalchemy.orm import Mapped, backref, declared_attr, relationship
+from sqlalchemy.orm import Mapped, backref, declared_attr, registry,
relationship
from airflow.api_fastapi.auth.managers.models.base_user import BaseUser
+from airflow.models.base import _get_schema, naming_convention
from airflow.providers.common.compat.sqlalchemy.orm import mapped_column
"""
Compatibility note: The models in this file are duplicated from Flask
AppBuilder.
"""
+metadata = MetaData(schema=_get_schema(), naming_convention=naming_convention)
+_mapper_registry = registry(metadata=metadata)
Review Comment:
`Model.metadata = Base.metadata` mutates FAB's Model class metadata at
import time. This is a global side effect that changes `Model.metadata` for all
code that imports Flask-AppBuilder's `Model`, not just this module. If anything
else depends on the original FAB metadata (its own naming conventions, schema
config), it'll silently pick up Airflow's conventions instead. Is the global
override intentional, or could this be scoped more tightly?
##########
airflow-core/src/airflow/utils/db.py:
##########
@@ -1148,7 +1155,9 @@ def _revisions_above_min_for_offline(config, revisions)
-> None:
)
Review Comment:
`run_post_migration_steps` is added here but never passed as `False`
anywhere in this PR. Is this intended for a follow-up? If so, might be better
to leave it out until it's needed to avoid dead code.
##########
providers/fab/src/airflow/providers/fab/auth_manager/models/__init__.py:
##########
@@ -199,9 +210,13 @@ class Permission(Model):
Sequence("ab_permission_view_id_seq", start=1, increment=1,
minvalue=1, cycle=False),
primary_key=True,
)
- action_id: Mapped[int] = mapped_column("permission_id", Integer,
ForeignKey("ab_permission.id"))
+ action_id: Mapped[int] = mapped_column(
Review Comment:
Changing `action_id` and `resource_id` to `nullable=True` while keeping
`Mapped[int]` (not `Mapped[int | None]`) means the Python type says it's always
an int, but the DB allows NULL. If a row has NULL here, accessing `.action_id`
returns `None` despite the type hint. Should the annotation be `Mapped[int |
None]` to match?
--
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]
