From bea58b762d5f6d9604561c24a0a3f94a15271538 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 6 Dec 2024 15:18:45 -0800 Subject: [PATCH 1/6] add indexes to catalog perms --- superset/migrations/shared/security_converge.py | 6 +++++- ...-52_58d051681a3b_add_catalog_perm_to_tables.py | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/superset/migrations/shared/security_converge.py b/superset/migrations/shared/security_converge.py index 42a68acb24369..19da899cdd0b4 100644 --- a/superset/migrations/shared/security_converge.py +++ b/superset/migrations/shared/security_converge.py @@ -17,6 +17,7 @@ import logging from dataclasses import dataclass +import sqlalchemy as sqla from sqlalchemy import ( Column, ForeignKey, @@ -94,7 +95,10 @@ def __repr__(self) -> str: class PermissionView(Base): # type: ignore __tablename__ = "ab_permission_view" - __table_args__ = (UniqueConstraint("permission_id", "view_menu_id"),) + __table_args__ = ( + sqla.Index("idx_permission_view_menu_id", "view_menu_id"), + sqla.Index("idx_permission_permission_id", "permission_id"), + ) id = Column(Integer, Sequence("ab_permission_view_id_seq"), primary_key=True) permission_id = Column(Integer, ForeignKey("ab_permission.id")) permission = relationship("Permission") diff --git a/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py b/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py index 6dfc2845bcb7d..dcd6bb1a071dc 100644 --- a/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py +++ b/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py @@ -29,12 +29,16 @@ downgrade_catalog_perms, upgrade_catalog_perms, ) -from superset.migrations.shared.utils import add_column_if_not_exists +from superset.migrations.shared.utils import add_column_if_not_exists, table_has_index # revision identifiers, used by Alembic. revision = "58d051681a3b" down_revision = "4a33124c18ad" +perm_table = "ab_permission_view" +perm_index_view_menu = "idx_permission_view_menu_id" +perm_index_permission_id = "idx_permission_permission_id" + def upgrade(): add_column_if_not_exists( @@ -45,10 +49,19 @@ def upgrade(): "slices", sa.Column("catalog_perm", sa.String(length=1000), nullable=True), ) + + if not table_has_index(perm_table, perm_index_view_menu): + op.create_index(op.f(perm_index_view_menu), perm_table, ["view_menu_id"]) + + if not table_has_index(perm_table, perm_index_permission_id): + op.create_index(op.f(perm_index_permission_id), perm_table, ["permission_id"]) + upgrade_catalog_perms(engines={"postgresql"}) def downgrade(): downgrade_catalog_perms(engines={"postgresql"}) + op.drop_index(op.f(perm_index_permission_id), table_name=perm_table) + op.drop_index(op.f(perm_index_view_menu), table_name=perm_table) op.drop_column("slices", "catalog_perm") op.drop_column("tables", "catalog_perm") From 7fbaf9987fc4767fbf46a943e2650577a30435a4 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 6 Dec 2024 15:21:40 -0800 Subject: [PATCH 2/6] add index to new migration --- ...23aa3bdd84_add_index_to_permission_view.py | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py diff --git a/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py b/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py new file mode 100644 index 0000000000000..27756295c0bc7 --- /dev/null +++ b/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py @@ -0,0 +1,48 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Add index to permission view + +Revision ID: 8a23aa3bdd84 +Revises: 48cbb571fa3a +Create Date: 2024-12-06 15:19:00.030588 + +""" + +from alembic import op + +from superset.migrations.shared.utils import table_has_index + +# revision identifiers, used by Alembic. +revision = "8a23aa3bdd84" +down_revision = "48cbb571fa3a" + +perm_table = "ab_permission_view" +perm_index_view_menu = "idx_permission_view_menu_id" +perm_index_permission_id = "idx_permission_permission_id" + + +def upgrade(): + if not table_has_index(perm_table, perm_index_view_menu): + op.create_index(op.f(perm_index_view_menu), perm_table, ["view_menu_id"]) + + if not table_has_index(perm_table, perm_index_permission_id): + op.create_index(op.f(perm_index_permission_id), perm_table, ["permission_id"]) + + +def downgrade(): + op.drop_index(op.f(perm_index_permission_id), table_name=perm_table) + op.drop_index(op.f(perm_index_view_menu), table_name=perm_table) From ea13294d8d2f3f0a04b9aa0399a0df9089f0e92c Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 6 Dec 2024 15:25:16 -0800 Subject: [PATCH 3/6] add checks before dropping --- ...-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py | 6 ++++-- ...2-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py b/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py index dcd6bb1a071dc..1cd23d2509868 100644 --- a/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py +++ b/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py @@ -61,7 +61,9 @@ def upgrade(): def downgrade(): downgrade_catalog_perms(engines={"postgresql"}) - op.drop_index(op.f(perm_index_permission_id), table_name=perm_table) - op.drop_index(op.f(perm_index_view_menu), table_name=perm_table) + if table_has_index(perm_table, perm_index_permission_id): + op.drop_index(op.f(perm_index_permission_id), table_name=perm_table) + if table_has_index(perm_table, perm_index_view_menu): + op.drop_index(op.f(perm_index_view_menu), table_name=perm_table) op.drop_column("slices", "catalog_perm") op.drop_column("tables", "catalog_perm") diff --git a/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py b/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py index 27756295c0bc7..28706ce1fd20f 100644 --- a/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py +++ b/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py @@ -44,5 +44,8 @@ def upgrade(): def downgrade(): - op.drop_index(op.f(perm_index_permission_id), table_name=perm_table) - op.drop_index(op.f(perm_index_view_menu), table_name=perm_table) + if table_has_index(perm_table, perm_index_permission_id): + op.drop_index(op.f(perm_index_permission_id), table_name=perm_table) + + if table_has_index(perm_table, perm_index_view_menu): + op.drop_index(op.f(perm_index_view_menu), table_name=perm_table) From fc451931149c25954d5bdfb5fd0db65acd44c53f Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 6 Dec 2024 16:16:11 -0800 Subject: [PATCH 4/6] add duplicate view name entry error handling --- superset/migrations/shared/catalogs.py | 20 +++++++++++++++++-- .../migrations/shared/security_converge.py | 5 +++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/superset/migrations/shared/catalogs.py b/superset/migrations/shared/catalogs.py index 3f14598eb6e82..a9176afdf892f 100644 --- a/superset/migrations/shared/catalogs.py +++ b/superset/migrations/shared/catalogs.py @@ -505,7 +505,15 @@ def upgrade_schema_perms( .filter_by(name=current_perm) .one_or_none() ): - existing_pvm.name = new_perm + # check that new_perm does not exist + if session.query(ViewMenu).filter_by(name=new_perm).one_or_none(): + logger.warning( + "New permission %s already exists. Removing old one", + new_perm, + ) + session.delete(existing_pvm) + else: + existing_pvm.name = new_perm elif new_perm: # new schema discovered, need to create a new permission perms[new_perm] = ("schema_access",) @@ -683,7 +691,15 @@ def downgrade_schema_perms( None, schema, ) - pvms_to_rename.append((pvm, new_name)) + # check to see if the new name already exists + if session.query(ViewMenu).filter_by(name=new_name).one_or_none(): + logger.warning( + "New permission %s already exists. Deleting old one", + new_name, + ) + pvms_to_delete.append(pvm) + else: + pvms_to_rename.append((pvm, new_name)) else: # non-default catalog, delete schema perm pvms_to_delete.append(pvm) diff --git a/superset/migrations/shared/security_converge.py b/superset/migrations/shared/security_converge.py index 19da899cdd0b4..0c5866ece0389 100644 --- a/superset/migrations/shared/security_converge.py +++ b/superset/migrations/shared/security_converge.py @@ -113,6 +113,11 @@ def _add_view_menu(session: Session, view_name: str) -> ViewMenu: """ Check and add the new view menu """ + # Check if the object is already in the session + for obj in session.identity_map.values(): + if isinstance(obj, ViewMenu) and obj.name == view_name: + return obj + new_view = session.query(ViewMenu).filter(ViewMenu.name == view_name).one_or_none() if not new_view: new_view = ViewMenu(name=view_name) From 8ba02ec47660e286c52ef74cf2216d481b5e6110 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Mon, 9 Dec 2024 16:16:35 -0800 Subject: [PATCH 5/6] do not delete existing PVMs --- superset/migrations/shared/catalogs.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/superset/migrations/shared/catalogs.py b/superset/migrations/shared/catalogs.py index a9176afdf892f..45d84e30365e3 100644 --- a/superset/migrations/shared/catalogs.py +++ b/superset/migrations/shared/catalogs.py @@ -506,13 +506,7 @@ def upgrade_schema_perms( .one_or_none() ): # check that new_perm does not exist - if session.query(ViewMenu).filter_by(name=new_perm).one_or_none(): - logger.warning( - "New permission %s already exists. Removing old one", - new_perm, - ) - session.delete(existing_pvm) - else: + if not session.query(ViewMenu).filter_by(name=new_perm).one_or_none(): existing_pvm.name = new_perm elif new_perm: # new schema discovered, need to create a new permission @@ -692,13 +686,7 @@ def downgrade_schema_perms( schema, ) # check to see if the new name already exists - if session.query(ViewMenu).filter_by(name=new_name).one_or_none(): - logger.warning( - "New permission %s already exists. Deleting old one", - new_name, - ) - pvms_to_delete.append(pvm) - else: + if not session.query(ViewMenu).filter_by(name=new_name).one_or_none(): pvms_to_rename.append((pvm, new_name)) else: # non-default catalog, delete schema perm From 5e2ef613eb0b8feba1c23d2251b3d28ccc0d57e9 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Mon, 9 Dec 2024 16:24:28 -0800 Subject: [PATCH 6/6] revert migration changes --- .../migrations/shared/security_converge.py | 11 +--- ...58d051681a3b_add_catalog_perm_to_tables.py | 17 +------ ...23aa3bdd84_add_index_to_permission_view.py | 51 ------------------- 3 files changed, 2 insertions(+), 77 deletions(-) delete mode 100644 superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py diff --git a/superset/migrations/shared/security_converge.py b/superset/migrations/shared/security_converge.py index 0c5866ece0389..42a68acb24369 100644 --- a/superset/migrations/shared/security_converge.py +++ b/superset/migrations/shared/security_converge.py @@ -17,7 +17,6 @@ import logging from dataclasses import dataclass -import sqlalchemy as sqla from sqlalchemy import ( Column, ForeignKey, @@ -95,10 +94,7 @@ def __repr__(self) -> str: class PermissionView(Base): # type: ignore __tablename__ = "ab_permission_view" - __table_args__ = ( - sqla.Index("idx_permission_view_menu_id", "view_menu_id"), - sqla.Index("idx_permission_permission_id", "permission_id"), - ) + __table_args__ = (UniqueConstraint("permission_id", "view_menu_id"),) id = Column(Integer, Sequence("ab_permission_view_id_seq"), primary_key=True) permission_id = Column(Integer, ForeignKey("ab_permission.id")) permission = relationship("Permission") @@ -113,11 +109,6 @@ def _add_view_menu(session: Session, view_name: str) -> ViewMenu: """ Check and add the new view menu """ - # Check if the object is already in the session - for obj in session.identity_map.values(): - if isinstance(obj, ViewMenu) and obj.name == view_name: - return obj - new_view = session.query(ViewMenu).filter(ViewMenu.name == view_name).one_or_none() if not new_view: new_view = ViewMenu(name=view_name) diff --git a/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py b/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py index 1cd23d2509868..6dfc2845bcb7d 100644 --- a/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py +++ b/superset/migrations/versions/2024-05-01_10-52_58d051681a3b_add_catalog_perm_to_tables.py @@ -29,16 +29,12 @@ downgrade_catalog_perms, upgrade_catalog_perms, ) -from superset.migrations.shared.utils import add_column_if_not_exists, table_has_index +from superset.migrations.shared.utils import add_column_if_not_exists # revision identifiers, used by Alembic. revision = "58d051681a3b" down_revision = "4a33124c18ad" -perm_table = "ab_permission_view" -perm_index_view_menu = "idx_permission_view_menu_id" -perm_index_permission_id = "idx_permission_permission_id" - def upgrade(): add_column_if_not_exists( @@ -49,21 +45,10 @@ def upgrade(): "slices", sa.Column("catalog_perm", sa.String(length=1000), nullable=True), ) - - if not table_has_index(perm_table, perm_index_view_menu): - op.create_index(op.f(perm_index_view_menu), perm_table, ["view_menu_id"]) - - if not table_has_index(perm_table, perm_index_permission_id): - op.create_index(op.f(perm_index_permission_id), perm_table, ["permission_id"]) - upgrade_catalog_perms(engines={"postgresql"}) def downgrade(): downgrade_catalog_perms(engines={"postgresql"}) - if table_has_index(perm_table, perm_index_permission_id): - op.drop_index(op.f(perm_index_permission_id), table_name=perm_table) - if table_has_index(perm_table, perm_index_view_menu): - op.drop_index(op.f(perm_index_view_menu), table_name=perm_table) op.drop_column("slices", "catalog_perm") op.drop_column("tables", "catalog_perm") diff --git a/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py b/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py deleted file mode 100644 index 28706ce1fd20f..0000000000000 --- a/superset/migrations/versions/2024-12-06_15-19_8a23aa3bdd84_add_index_to_permission_view.py +++ /dev/null @@ -1,51 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. -"""Add index to permission view - -Revision ID: 8a23aa3bdd84 -Revises: 48cbb571fa3a -Create Date: 2024-12-06 15:19:00.030588 - -""" - -from alembic import op - -from superset.migrations.shared.utils import table_has_index - -# revision identifiers, used by Alembic. -revision = "8a23aa3bdd84" -down_revision = "48cbb571fa3a" - -perm_table = "ab_permission_view" -perm_index_view_menu = "idx_permission_view_menu_id" -perm_index_permission_id = "idx_permission_permission_id" - - -def upgrade(): - if not table_has_index(perm_table, perm_index_view_menu): - op.create_index(op.f(perm_index_view_menu), perm_table, ["view_menu_id"]) - - if not table_has_index(perm_table, perm_index_permission_id): - op.create_index(op.f(perm_index_permission_id), perm_table, ["permission_id"]) - - -def downgrade(): - if table_has_index(perm_table, perm_index_permission_id): - op.drop_index(op.f(perm_index_permission_id), table_name=perm_table) - - if table_has_index(perm_table, perm_index_view_menu): - op.drop_index(op.f(perm_index_view_menu), table_name=perm_table)