Skip to content

refactor: set cascade_backrefs=False for Query#42210

Open
rusackas wants to merge 1 commit into
masterfrom
refactor/sqla2-cascade-query
Open

refactor: set cascade_backrefs=False for Query#42210
rusackas wants to merge 1 commit into
masterfrom
refactor/sqla2-cascade-query

Conversation

@rusackas

Copy link
Copy Markdown
Member

See #40273

SUMMARY

Enable errors for the "Query" object is being merged into a Session along the backref cascade path RemovedIn20Warning, and fix the code that relied on the reverse cascade.

  • Database.queries backref now sets cascade_backrefs=False, which is the SQLAlchemy 2.0 behavior: constructing Query(database=...) no longer implicitly merges the new object into the Database's session. All code paths that persist queries already call session.add() explicitly.
  • SupersetSecurityManager.raise_for_access builds an ephemeral Query for access checks and unconditionally expunged it from the session. Since the object is no longer implicitly merged, the expunge is now guarded, matching the existing pattern in Database.resolve_default_schema.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

$ SQLALCHEMY_WARN_20=1 TZ=UTC python3 -m pytest tests/unit_tests/

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

🤖 Generated with Claude Code

Enable the pytest error filter for the "'Query' object is being merged
into a Session along the backref cascade path" RemovedIn20Warning and
adopt the SQLAlchemy 2.0 behavior for the Database.queries backref.

Constructing Query(database=...) no longer implicitly merges the new
object into the Database's session. All persisting code paths already
add queries to the session explicitly; the ephemeral Query built in
SupersetSecurityManager.raise_for_access no longer needs an
unconditional expunge, so it is now guarded the same way as
Database.resolve_default_schema.

See discussion #40273.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dosubot dosubot Bot added the change:backend Requires changing the backend label Jul 18, 2026
@bito-code-review

bito-code-review Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #1a7d11

Actionable Suggestions - 0
Additional Suggestions - 2
  • superset/models/sql_lab.py - 1
    • Consistency: SavedQuery backrefs missing cascade_backrefs · Line 193-200
      The SavedQuery class (lines 503, 509) uses plain backref() without cascade_backrefs=False, unlike Query.database which now adopts the 2.0 behavior. For consistency and to avoid divergent session-management behavior between Query and SavedQuery, consider applying the same setting to SavedQuery backrefs as well.
  • pytest.ini - 1
    • Inconsistent warning enforcement · Line 32-32
      This change enables error-level enforcement for 'Query' object session-merge warnings, but the same pattern is commented out for 'SavedQuery', 'SqlaTable', 'SqlMetric', 'TableColumn', 'TaggedObject', and 'User'. These share the same deprecated behavior and should be handled consistently to ensure complete SQLAlchemy 2.0 migration coverage.
Review Details
  • Files reviewed - 3 · Commit Range: f512d37..f512d37
    • pytest.ini
    • superset/models/sql_lab.py
    • superset/security/manager.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 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

AI Code Review powered by Bito Logo

@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.18%. Comparing base (751f5eb) to head (f512d37).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
superset/security/manager.py 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42210      +/-   ##
==========================================
- Coverage   65.19%   65.18%   -0.01%     
==========================================
  Files        2767     2768       +1     
  Lines      156062   156082      +20     
  Branches    35717    35720       +3     
==========================================
+ Hits       101741   101749       +8     
- Misses      52361    52369       +8     
- Partials     1960     1964       +4     
Flag Coverage Δ
hive 38.61% <0.00%> (+<0.01%) ⬆️
mysql 57.86% <0.00%> (-0.01%) ⬇️
postgres 57.92% <0.00%> (-0.01%) ⬇️
presto 40.55% <0.00%> (+<0.01%) ⬆️
python 59.32% <0.00%> (-0.01%) ⬇️
sqlite 57.53% <0.00%> (-0.01%) ⬇️
unit 100.00% <ø> (ø)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:backend Requires changing the backend size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant