Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: pass if table is already removed on upgrade #30017

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions superset/migrations/shared/constraints.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@
from dataclasses import dataclass

from alembic import op
from sqlalchemy.dialects.sqlite.base import SQLiteDialect # noqa: E402

Check warning on line 22 in superset/migrations/shared/constraints.py

View check run for this annotation

Codecov / codecov/patch

superset/migrations/shared/constraints.py#L22

Added line #L22 was not covered by tests
from sqlalchemy.engine.reflection import Inspector

from superset.migrations.shared.utils import has_table

Check warning on line 25 in superset/migrations/shared/constraints.py

View check run for this annotation

Codecov / codecov/patch

superset/migrations/shared/constraints.py#L25

Added line #L25 was not covered by tests
from superset.utils.core import generic_find_fk_constraint_name


Expand Down Expand Up @@ -75,13 +77,19 @@

def drop_fks_for_table(table_name: str) -> None:
"""
Drop all foreign key constraints for a table.
Drop all foreign key constraints for a table if it exist and the database
is not sqlite.

:param table_name: The table name to drop foreign key constraints for
"""
connection = op.get_bind()
inspector = Inspector.from_engine(connection)
foreign_keys = inspector.get_foreign_keys(table_name)

for fk in foreign_keys:
op.drop_constraint(fk["name"], table_name, type_="foreignkey")
if isinstance(connection.dialect, SQLiteDialect):
return # sqlite doesn't like constraints

Check warning on line 89 in superset/migrations/shared/constraints.py

View check run for this annotation

Codecov / codecov/patch

superset/migrations/shared/constraints.py#L88-L89

Added lines #L88 - L89 were not covered by tests

if has_table(table_name):
foreign_keys = inspector.get_foreign_keys(table_name)

Check warning on line 92 in superset/migrations/shared/constraints.py

View check run for this annotation

Codecov / codecov/patch

superset/migrations/shared/constraints.py#L91-L92

Added lines #L91 - L92 were not covered by tests

for fk in foreign_keys:
op.drop_constraint(fk["name"], table_name, type_="foreignkey")

Check warning on line 95 in superset/migrations/shared/constraints.py

View check run for this annotation

Codecov / codecov/patch

superset/migrations/shared/constraints.py#L94-L95

Added lines #L94 - L95 were not covered by tests
15 changes: 15 additions & 0 deletions superset/migrations/shared/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,18 @@
except json.JSONDecodeError:
print(f"Failed to parse: {data}")
return {}


def has_table(table_name: str) -> bool:
"""
Check if a table exists in the database.

:param table_name: The table name
:returns: True if the table exists
"""

insp = inspect(op.get_context().bind)
table_exists = insp.has_table(table_name)

Check warning on line 182 in superset/migrations/shared/utils.py

View check run for this annotation

Codecov / codecov/patch

superset/migrations/shared/utils.py#L181-L182

Added lines #L181 - L182 were not covered by tests

print(f"Table {table_name} exist: {table_exists}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe delete this print? If not, then:

Suggested change
print(f"Table {table_name} exist: {table_exists}")
print(f"Table {table_name} exists: {table_exists}")

return table_exists

Check warning on line 185 in superset/migrations/shared/utils.py

View check run for this annotation

Codecov / codecov/patch

superset/migrations/shared/utils.py#L184-L185

Added lines #L184 - L185 were not covered by tests
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,19 @@
from alembic import op

from superset.migrations.shared.constraints import drop_fks_for_table
from superset.migrations.shared.utils import has_table

# revision identifiers, used by Alembic.
revision = "39549add7bfc"
down_revision = "02f4f7811799"


def upgrade():
connection = op.get_bind()
if connection.dialect.name != "sqlite":
drop_fks_for_table("sl_table_columns")
op.drop_table("sl_table_columns")
table_name = "sl_table_columns"
Copy link
Member

@michael-s-molina michael-s-molina Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this outside the upgrade method (below down_revision variable) and reuse it on downgrade?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do this for all changed migrations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


if has_table(table_name):
drop_fks_for_table(table_name)
op.drop_table(table_name)


def downgrade():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,19 @@
from alembic import op

from superset.migrations.shared.constraints import drop_fks_for_table
from superset.migrations.shared.utils import has_table

# revision identifiers, used by Alembic.
revision = "38f4144e8558"
down_revision = "39549add7bfc"


def upgrade():
connection = op.get_bind()
if connection.dialect.name != "sqlite":
drop_fks_for_table("sl_dataset_tables")
op.drop_table("sl_dataset_tables")
table_name = "sl_dataset_tables"

if has_table(table_name):
drop_fks_for_table(table_name)
op.drop_table(table_name)


def downgrade():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,19 @@
from alembic import op

from superset.migrations.shared.constraints import drop_fks_for_table
from superset.migrations.shared.utils import has_table

# revision identifiers, used by Alembic.
revision = "e53fd48cc078"
down_revision = "38f4144e8558"


def upgrade():
connection = op.get_bind()
if connection.dialect.name != "sqlite":
drop_fks_for_table("sl_dataset_users")
op.drop_table("sl_dataset_users")
table_name = "sl_dataset_users"

if has_table(table_name):
drop_fks_for_table(table_name)
op.drop_table(table_name)


def downgrade():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,19 @@
from alembic import op

from superset.migrations.shared.constraints import drop_fks_for_table
from superset.migrations.shared.utils import has_table

# revision identifiers, used by Alembic.
revision = "a6b32d2d07b1"
down_revision = "e53fd48cc078"


def upgrade():
connection = op.get_bind()
if connection.dialect.name != "sqlite":
drop_fks_for_table("sl_columns")
op.drop_table("sl_columns")
table_name = "sl_columns"

if has_table(table_name):
drop_fks_for_table(table_name)
op.drop_table(table_name)


def downgrade():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,19 @@
from alembic import op

from superset.migrations.shared.constraints import drop_fks_for_table
from superset.migrations.shared.utils import has_table

# revision identifiers, used by Alembic.
revision = "007a1abffe7e"
down_revision = "a6b32d2d07b1"


def upgrade():
connection = op.get_bind()
if connection.dialect.name != "sqlite":
drop_fks_for_table("sl_tables")
op.drop_table("sl_tables")
table_name = "sl_tables"

if has_table(table_name):
drop_fks_for_table(table_name)
op.drop_table(table_name)


def downgrade():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,19 @@
from alembic import op

from superset.migrations.shared.constraints import drop_fks_for_table
from superset.migrations.shared.utils import has_table

# revision identifiers, used by Alembic.
revision = "48cbb571fa3a"
down_revision = "007a1abffe7e"


def upgrade():
connection = op.get_bind()
if connection.dialect.name != "sqlite":
drop_fks_for_table("sl_datasets")
op.drop_table("sl_datasets")
table_name = "sl_datasets"

if has_table(table_name):
drop_fks_for_table(table_name)
op.drop_table(table_name)


def downgrade():
Expand Down
Loading