fix(semantic-layers): remove child view permissions when a layer is deleted

[mikebridge]

SUMMARY

Deleting a semantic layer left its views' datasource_access permissions and role grants behind when the views were not loaded in the session, because the cleanup depended on per-view ORM delete events.

Cleanup now runs in a layer before_delete hook on the connection, so it does not depend on whether the views are loaded, and it happens in the same transaction as the delete. A permission that is still in use is kept. That includes a permission a view in another layer uses, and, when a dataset is deleted, a permission a live semantic view still uses.

For the unloaded layer deletion hook used by DeleteSemanticLayerCommand, ownership checks are batched into three fixed-size SELECTs; a 30-child test guards this count. A loaded ORM collection still takes two ownership checks per child.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A (backend cleanup on delete).

TESTING INSTRUCTIONS

• pytest tests/unit_tests/semantic_layers/models_test.py
• New tests delete a layer with its views loaded and unloaded and assert the view permissions and role grants are gone; they fail on master in the unloaded case. Further tests assert that a permission still in use survives both a view delete and a layer delete. Regressions cover a key shared with a view in another layer, and a dataset delete whose permission a semantic view still uses.
• Not covered: large-catalog query plans and concurrent deletes. The permission columns are unindexed; adding indexes to tables.perm and semantic_views.perm is deliberately left to a separate migration change.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
• Introduces new feature or API
• Removes existing feature or API

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.26%. Comparing base (e606cad) to head (837a741).
⚠️ Report is 95 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/security/manager.py	93.54%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44905      +/-   ##
==========================================
+ Coverage   82.14%   82.26%   +0.11%     
==========================================
  Files        2995     2996       +1     
  Lines      184988   185580     +592     
  Branches    42814    42928     +114     
==========================================
+ Hits       151960   152663     +703     
+ Misses      30269    30162     -107     
+ Partials     2759     2755       -4     

Flag	Coverage Δ
hive	36.26% <17.14%> (-0.05%)	⬇️
mysql	55.22% <48.57%> (-0.12%)	⬇️
postgres	55.23% <48.57%> (-0.13%)	⬇️
presto	38.13% <17.14%> (-0.05%)	⬇️
python	86.32% <91.42%> (+0.19%)	⬆️
sqlite	54.96% <48.57%> (-0.12%)	⬇️
unit	79.38% <88.57%> (+0.28%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codeant-ai-for-open-source]

Suggestion: Deleting a layer now performs multiple permission queries per child view, causing a large layer deletion to issue O(n) database round trips and become unnecessarily slow.

Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Performance

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset/semantic_layers/models.py
**Line:** 842:842
**Comment:**
	*Performance: Deleting a layer now performs multiple permission queries per child view, causing a large layer deletion to issue O(n) database round trips and become unnecessarily slow.

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

👍 | 👎

[bito-code-review]

The flagged performance issue is correct. The current implementation of semantic_layer_before_delete performs a database query for every child view to check if its permission is owned elsewhere, leading to O(n) round trips.

To resolve this, you can optimize the permission ownership check by batching the queries. Instead of querying for each view individually, you can fetch all relevant permissions in a single query or use a more efficient set-based approach to identify which permissions are still in use by other resources.

superset/security/manager.py

def _semantic_view_perm_owned_elsewhere(
        self,
        connection: Connection,
        perm: str,
        deleted_view_ids: set[int],
    ) -> bool:
        # ... (existing logic)
        # Optimization: Consider fetching all used permissions in one query
        # and checking against that set instead of querying per-permission.
        return (
            connection.execute(
                sv_table.select()
                .where(sv_table.c.perm == perm, sv_table.c.id.not_in(deleted_view_ids))
                .limit(1)
            ).first()
            is not None
        )

[bito-code-review]

Code Review Agent Run #089298

Actionable Suggestions - 2

• superset/security/manager.py - 2

  • N+1 queries in delete hook · Line 4363-4366
  • Unindexed perm equality scans · Line 4392-4402

Review Details

• Files reviewed - 3 · Commit Range: d010870..bbb22ad

  • superset/security/manager.py
  • superset/semantic_layers/models.py
  • tests/unit_tests/semantic_layers/models_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[bito-code-review]

N+1 queries in delete hook

Each iteration of the views loop calls _semantic_view_perm_owned_elsewhere, which issues two queries (datasets.perm and semantic_views.perm lookups). Deleting a layer with N child views runs 2N+1 queries inside the flush. Consider batching: collect distinct perms, resolve ownership with one query per table, then delete the PVMs.

Code Review Run #089298

---

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

• Yes, avoid them

[bito-code-review]

Unindexed perm equality scans

SqlaTable.perm and SemanticView.perm are unindexed String(1000) columns; the equality lookups here full-scan datasets and semantic_views, once per deleted view (see the loop in semantic_layer_before_delete). An index on perm would bound this cleanup cost on large deployments.

Code Review Run #089298

---

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

• Yes, avoid them

[aminghadersohi]

No blockers: the cascade premise holds (migration declares ON DELETE CASCADE, SQLite FK pragma is on) and perms are unique per view via the (id:N) suffix. Mutants forcing the ownership check True/False, or dropping the SqlaTable lookup, the not_in clause or the listener, all fail the new tests.

[aminghadersohi]

Non-blocking: the guard is one-way. dataset_after_delete (superset/security/manager.py L3958, outside this diff) still drops the PVM unconditionally, so deleting the colliding dataset revokes grants on the semantic view this PR now protects. Same check there would close it; fine as a follow-up.

[mikebridge]

Thanks for pointing out the reverse deletion path. Fixed in 89dec7f: dataset deletion now checks whether a live semantic view still owns the permission before removing its grants. A red-first test reproduces the previous revocation and verifies the grant survives.

[aminghadersohi]

Nit: internal tracker IDs carry no meaning in apache/superset; suggest dropping it here and on L2447.

Suggested change

	"""sc-123444: deleting a layer removes each child view's datasource_access
	"""Deleting a layer removes each child view's datasource_access

[mikebridge]

Agreed. 89dec7f removes the internal tracker reference from the test docstring and gives it a complete first-line summary.

[aminghadersohi]

Same here.

Suggested change

	# Two children: while ``semantic_views`` is mapped as a scalar (SC-123445),
	# Two children: while ``semantic_views`` is mapped as a scalar,

[mikebridge]

Agreed. 89dec7f removes the internal tracker reference and describes the loaded ORM and unloaded database-cascade cases directly.

[gabotorresruiz]

Hi @mikebridge, LGTM. The shape is right: the hook runs on the connection inside the same flush as the layer DELETE, the loaded children are deleted by the ORM first and clean up in semantic_view_after_delete, and the ones left to ON DELETE CASCADE are read while their rows still exist, so no path leaves ab_permission_view_role or ab_permission_view rows behind. I also confirmed the scalar mapping the test comment mentions (semantic_views reports uselist=False at runtime), so until that is fixed the new hook is what carries every child past the first; good that the fix does not depend on it. Ran tests/unit_tests/semantic_layers/models_test.py here (115 passing) and the five new cases on the merge base, where four fail (the loaded-children case too, for the same scalar-mapping reason). Agree with Amin on dropping the tracker ids from the test and on the dataset_after_delete follow-up. CI is green. One small nit inline.

[gabotorresruiz]

Just a small NIT: this pulls every column of every child view, including configuration, when the loop only reads id and perm. select(sv_table.c.id, sv_table.c.perm) keeps the hook cheap on a layer with many or large views. semantic_layer_before_update does the same today, so feel free to leave it for consistency. Not a blocker.

[mikebridge]

Thanks, good catch. The later batch-cleanup commit already changed the layer-deletion hook to select only perm, so it no longer loads each view's configuration.

I added a regression assertion in 837a741 that keeps all of the ownership SELECTs narrow; the focused models and security tests pass. The separate update hook is unchanged, since it is outside this deletion-path change.

[codeant-ai-for-open-source]

CodeAnt PR Risk: Low Risk

• The PR appears safe to merge; permission cleanup preserves grants shared with datasets or other semantic views.
• Layer deletion uses a bounded set of ownership queries, and tests cover loaded and database-cascaded child views.

Assessed commit: 837a7410437e

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	80a405d
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ac45060c6dc0d000895078e
😎 Deploy Preview	https://deploy-preview-44905--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[bito-code-review]

Code Review Agent Run #0eea76

Actionable Suggestions - 0

Additional Suggestions - 3

• tests/unit_tests/semantic_layers/models_test.py - 2

  • Inconsistent fixture type annotation · Line 2591-2591
    The new test annotates the fixture as `session: Session` while every sibling test in this file (`test_layer_delete_batches_permission_ownership_queries` line 2486, `test_view_delete_keeps_permission_another_resource_owns` line 2540, `test_dataset_delete_keeps_permission_a_semantic_view_owns` line 2649) uses `session: Any`. The mixed annotations in one file are inconsistent; align them in one direction.
  • CWE-476: Unguarded Lookup Dereference · Line 2644-2644
    `session.get(SemanticView, retained_view_id)` can return `None`, so `.perm` on line 2644 would raise `AttributeError` instead of a clean assertion failure if the layer delete cascaded the retained view. The test already guards the equivalent lookup with `assert pvm is not None` (line 2625); add the same guard here. ([CWE-476](https://cwe.mitre.org/data/definitions/476.html))
• superset/security/manager.py - 1

  • Duplicated perm-ownership rule · Line 4385-4406
    The rule 'a datasource_access PVM survives while a dataset or another semantic view owns the perm' is now encoded in three places: the guard in `dataset_after_delete`, the batch queries here, and `_semantic_view_perm_owned_elsewhere`. The batch form rightly avoids the old N+1 loop, but parallel encodings can diverge silently (e.g. when a third owner type is added). Consider extracting one set-based helper shared by both paths.

Review Details

• Files reviewed - 2 · Commit Range: bbb22ad..80a405d

  • superset/security/manager.py
  • tests/unit_tests/semantic_layers/models_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by