codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3530161837
##########
superset/commands/database/sync_permissions.py:
##########
@@ -283,17 +286,29 @@ def _rename_database_in_permissions(
if existing_pvm:
existing_pvm.view_menu.name = new_schema_perm_name
- # rename permissions on datasets and charts
- for dataset in DatabaseDAO.get_datasets(
- self.db_connection_id,
- catalog=catalog,
- schema=schema,
- ):
- dataset.catalog_perm = new_catalog_perm_name
- dataset.schema_perm = new_schema_perm_name
- for chart in
DatasetDAO.get_related_objects(dataset.id)["charts"]:
- chart.catalog_perm = new_catalog_perm_name
- chart.schema_perm = new_schema_perm_name
+ # Rename permissions on datasets and charts. Bypass the
+ # soft-delete visibility filter: a soft-deleted dataset's
+ # ``schema_perm``/``catalog_perm`` (and its charts') must still be
+ # rewritten on a database rename — otherwise restoring the dataset
+ # later brings back stale perm strings referencing the old
+ # database name (fail-closed for legitimate users, and matchable
+ # by a schema_access grant on a new database reusing the old
+ # name). Mirrors the same bypass in
+ # ``security_manager._update_vm_datasources_access``, which
+ # maintains the dataset-level ``perm`` string. ``Slice`` is
+ # included so chart perm rewrites keep working once charts gain
+ # soft delete (a no-op until then).
+ with skip_visibility_filter(db.session, SqlaTable, Slice):
+ for dataset in DatabaseDAO.get_datasets(
+ self.db_connection_id,
+ catalog=catalog,
+ schema=schema,
+ ):
+ dataset.catalog_perm = new_catalog_perm_name
+ dataset.schema_perm = new_schema_perm_name
+ for chart in
DatasetDAO.get_related_objects(dataset.id)["charts"]:
+ chart.catalog_perm = new_catalog_perm_name
+ chart.schema_perm = new_schema_perm_name
Review Comment:
**Suggestion:** This introduces an N+1 query pattern during database rename:
for every dataset in a schema you call `get_related_objects`, which runs
multiple SQL queries (including an unnecessary dashboards query) even though
only charts are used. On databases with many datasets this can make permission
sync very slow and timeout-prone. Fetch charts in bulk once per schema (or add
a charts-only DAO helper) instead of querying per dataset. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Database rename sync issues two queries per dataset.
- ⚠️ Large schemas may cause slow or timing-out sync.
- ⚠️ Unnecessary dashboards queries increase database load.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger a database rename so permissions are synced:
- Call `PUT /api/v1/database/<id>` to change `database_name`, which
invokes
`UpdateDatabaseCommand.run` in
`superset/commands/database/update.py:54-118`.
- That method computes `original_database_name` and then calls
`SyncPermissionsCommand(...).run()` at `update.py:105-115`.
2. Follow the execution into `SyncPermissionsCommand`:
- `SyncPermissionsCommand.run` in
`superset/commands/database/sync_permissions.py:122-133` calls
`self.sync_database_permissions()` when
`SYNC_DB_PERMISSIONS_IN_ASYNC_MODE` is False
(or enqueues the Celery task which calls the same method).
- `sync_database_permissions` iterates catalogs and schemas and, when the
database name
changed, calls `_rename_database_in_permissions(catalog, schemas)` at
`sync_permissions.py:185-187`.
3. Observe dataset enumeration per schema:
- Inside `_rename_database_in_permissions`
(`sync_permissions.py:248-312`), for each
`schema` it enters the new block at `sync_permissions.py:289-301`.
- Within the `skip_visibility_filter` context, it calls
`DatabaseDAO.get_datasets(self.db_connection_id, catalog=catalog,
schema=schema)` and
iterates `for dataset in ...` at `sync_permissions.py:302-306`.
4. See the per-dataset relationship queries and redundant dashboards query:
- For each `dataset` returned, the code runs `for chart in
DatasetDAO.get_related_objects(dataset.id)["charts"]:` at
`sync_permissions.py:309-311`.
- `DatasetDAO.get_related_objects` in `superset/daos/dataset.py:124-144`
issues two
separate SQL queries per call: one to load charts
(`db.session.query(Slice)...all()`)
and another to load dashboards via a `Dashboard` join and
`Slice.id.in_(chart_ids)`.
- Because this call is made once per dataset, the permission sync
performs an N+1
pattern (2 queries per dataset) plus one query for the dataset list, and
it loads
dashboards even though `_rename_database_in_permissions` only uses the
`charts` list.
On databases with many datasets in a schema, this can significantly slow
or even time
out the `/<pk>/sync_permissions/` endpoint and the implicit sync invoked
from database
updates.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=16f8bbdeb5e445508f870ff558c33571&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=16f8bbdeb5e445508f870ff558c33571&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/database/sync_permissions.py
**Line:** 309:311
**Comment:**
*Performance: This introduces an N+1 query pattern during database
rename: for every dataset in a schema you call `get_related_objects`, which
runs multiple SQL queries (including an unnecessary dashboards query) even
though only charts are used. On databases with many datasets this can make
permission sync very slow and timeout-prone. Fetch charts in bulk once per
schema (or add a charts-only DAO helper) instead of querying per dataset.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=a7ed2082234c00e5336b69d92fa1f4e477d79fb1d5b18047b8652b8f59a05a6f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=a7ed2082234c00e5336b69d92fa1f4e477d79fb1d5b18047b8652b8f59a05a6f&reaction=dislike'>👎</a>
--
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]
