codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510304039
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Avoid flagging missing type annotations for local test-scaffolding
variables in test files when the type is already clear from context; adding
annotations there is considered noise.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing type annotations for local/test-scaffolding variables
in test files when the type is already unambiguous from context and the
annotation would add noise.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing type annotations for local helper functions or
test-scaffolding variables in test files when the types are already unambiguous
from context and adding annotations would only add noise.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
--
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]
