fix(models): guard three more backref cascades for SQLAlchemy 2.0 - #42787
Conversation
SSHTunnel.database, ReportRecipients.report_schedule, and ReportExecutionLog.report_schedule were missed by the earlier sweep that added cascade_backrefs=False to Query, SavedQuery, SqlaTable, SqlMetric, TableColumn, TaggedObject, and User (discussion #40273, step 2) -- same reverse-cascade warning, same fix. Locks each in via a pytest.ini filterwarnings error line, matching the existing entries for the other seven models.
Code Review Agent Run #b39056Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42787 +/- ##
=======================================
Coverage 65.79% 65.79%
=======================================
Files 2842 2842
Lines 162106 162106
Branches 37148 37148
=======================================
Hits 106653 106653
Misses 53388 53388
Partials 2065 2065
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CC @hy144328 |
There was a problem hiding this comment.
Pull request overview
This PR continues the SQLAlchemy 2.0 migration work by preventing deprecated “backref cascade” session merges on three additional ORM relationships, aligning runtime behavior with SQLAlchemy 2.0 while locking regressions via pytest.ini warning-as-error rules.
Changes:
- Disable backref cascade merging for
ReportRecipients.report_scheduleandReportExecutionLog.report_scheduleby settingcascade_backrefs=Falseon theReportSchedulebackrefs. - Disable backref cascade merging for
SSHTunnel.databaseby settingcascade_backrefs=Falseon theDatabase.ssh_tunnelbackref. - Add
pytest.inifilterwarningserror entries for the three models to ensure these warnings cannot regress silently.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
superset/reports/models.py |
Adds cascade_backrefs=False to ReportSchedule.recipients and ReportSchedule.logs backrefs to prevent SQLAlchemy 2.0 backref-cascade session merge behavior. |
superset/databases/ssh_tunnel/models.py |
Adds cascade_backrefs=False to the Database.ssh_tunnel backref to prevent backref-cascade session merge behavior. |
pytest.ini |
Adds warning-as-error filters for the three newly covered “merged into a Session” RemovedIn20Warning messages. |
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
Approving with some food for thought.
| # SQLAlchemy 2.0 behavior: assigning `ssh_tunnel.database` no | ||
| # longer cascades the SSHTunnel into the Database's session; | ||
| # callers must add objects to a session explicitly. |
There was a problem hiding this comment.
Can we just make sure the code works on 1.4 and 2.0 and then remove these comments? I assume it just requires adding the child object into the session explicitly wherever that's being done.
There was a problem hiding this comment.
Already checked that part, ran the suite with SQLALCHEMY_WARN_20=1 and the new pytest.ini lines as hard errors, all 426 pass. So none of the three actually lean on the old implicit cascade, they already either call session.add() explicitly or go through the forward collection assignment, which has its own real cascade untouched by this flag.
On the comments themselves, I'd lean toward keeping them just to match the other seven models that got the identical treatment earlier in this series. Not a hill I need to die on though, curious what you think. @hy144328 might have an opinion too since he's driven most of this convention.
|
The comments were added to document the transition to SQLAlchemy 2.0 behavior, where superset/databases/ssh_tunnel/models.py |
SUMMARY
Discussion #40273 step 2 (SQLAlchemy 2.0 migration roadmap) added
cascade_backrefs=Falseto seven models (Query,SavedQuery,SqlaTable,SqlMetric,TableColumn,TaggedObject,User) whosebackref()-defined reverse relationships were triggering SQLAlchemy's "object is being merged into a Session along the backref cascade path" deprecation warning, locked in via apytest.inifilterwarningserror line per model to prevent regression.A follow-up sweep for step 6 prep found three more models with the exact same pattern that the original sweep missed:
SSHTunnel.database,ReportRecipients.report_schedule, andReportExecutionLog.report_schedule. Same fix, same reasoning: in SQLAlchemy 2.0 this reverse cascade no longer happens automatically, so callers that relied on it implicitly would silently stop persisting these objects. Verified each caller already reaches the objects via an explicitdb.session.add()or the forward collection assignment (which carries its owncascade="all, delete-orphan", unaffected bycascade_backrefs), so nothing here depended on the removed behavior.TESTING INSTRUCTIONS
426 tests pass with the three new
pytest.inierror lines active, confirming none of the exercised code paths still rely on the removed implicit cascade.ADDITIONAL INFORMATION