mikebridge commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510302530
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -191,19 +193,34 @@ def create_virtual_datasets(self):
@pytest.fixture
def create_datasets(self):
with self.create_app().app_context():
+ # Purge any soft-deleted rows that occupy the unique constraint
+ stale = self.get_fixture_datasets()
+ for ds in stale:
+ db.session.delete(ds)
+ if stale:
+ db.session.commit()
+
datasets = []
admin = self.get_user("admin")
main_db = get_main_database()
for tables_name in self.fixture_tables_names:
datasets.append(self.insert_dataset(tables_name, [admin.id],
main_db))
+ # Capture IDs eagerly — dataset objects may be detached after yield
+ dataset_ids = [ds.id for ds in datasets]
+
yield datasets
- # rollback changes
- for dataset in datasets:
- state = inspect(dataset)
- if not state.was_deleted:
- db.session.delete(dataset)
+ # rollback changes (including soft-deleted rows)
+ for dataset_id in dataset_ids:
+ row = (
+ db.session.query(SqlaTable)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES:
{SqlaTable}})
+ .filter(SqlaTable.id == dataset_id)
+ .one_or_none()
+ )
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
##########
tests/unit_tests/commands/databases/sync_permissions_test.py:
##########
@@ -409,3 +409,49 @@ def test_sync_permissions_command_rename_db_in_perms(
assert (
mock_chart.schema_perm ==
f"[{database_with_catalog.name}].[catalog1].[schema1]"
)
+
+
+def test_sync_permissions_command_rename_bypasses_visibility_filter(
+ mocker: MockerFixture, database_with_catalog: MagicMock
+):
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
##########
tests/unit_tests/commands/databases/sync_permissions_test.py:
##########
@@ -409,3 +409,49 @@ def test_sync_permissions_command_rename_db_in_perms(
assert (
mock_chart.schema_perm ==
f"[{database_with_catalog.name}].[catalog1].[schema1]"
)
+
+
+def test_sync_permissions_command_rename_bypasses_visibility_filter(
+ mocker: MockerFixture, database_with_catalog: MagicMock
+):
+ """The dataset/chart perm rewrite must run with the soft-delete
+ visibility filter bypassed for ``SqlaTable`` and ``Slice``.
+
+ A soft-deleted dataset's ``schema_perm``/``catalog_perm`` (and its
+ charts') must still be rewritten on a database rename; without the
+ bypass, the listener hides those rows from ``get_datasets`` and a
+ later restore resurrects stale perm strings referencing the old
+ database name — matchable by a ``schema_access`` grant on a new
+ database reusing that name.
+ """
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
+ from superset.models.slice import Slice
+
+ mocker.patch(
+ "superset.commands.database.sync_permissions."
+ "security_manager.find_permission_view_menu",
+ return_value=None,
+ )
+ observed_bypass: list[set[type]] = []
+
+ def capture_bypass(*args, **kwargs):
+ observed_bypass.append(
+ set(db.session.info.get(SKIP_VISIBILITY_FILTER_CLASSES, set()))
+ )
+ return []
+
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
