experiment: test model_class= fix for FSA 3.x break (DO NOT MERGE) - #42789
Closed
rusackas wants to merge 1 commit into
Closed
experiment: test model_class= fix for FSA 3.x break (DO NOT MERGE)#42789rusackas wants to merge 1 commit into
rusackas wants to merge 1 commit into
Conversation
Investigation-only commit for discussion #40273 / PR #42542's revert. Passes model_class=flask_appbuilder.models.sqla.Model explicitly when constructing SQLA() in superset/extensions/__init__.py, testing the theory that FAB's FSA-3.x shim (base.py) leaves Superset's models and db.session looking at two disconnected declarative registries, since unlike the FSA-2.x shim (base_legacy.py) it never wires model_class through. Local pytest runs against sqlite reproduce neither the break nor a fix signal (matches PR #42542's original finding that this only surfaced under real CI). This commit exists to get a real CI signal on test-sqlite/test-mysql before deciding whether to pursue this as an actual fix (and whether it needs an upstream flask-appbuilder change too, or is fixable entirely on Superset's side as this attempts).
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #42789 +/- ##
==========================================
- Coverage 65.79% 56.84% -8.95%
==========================================
Files 2842 2842
Lines 162106 162107 +1
Branches 37148 37148
==========================================
- Hits 106653 92152 -14501
- Misses 53388 69116 +15728
+ Partials 2065 839 -1226
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:
|
6 tasks
Member
Author
|
Theory disproven, see #39278 for the corrected finding. Closing, not merging. |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Investigation only, do not merge. Testing a theory for the flask-sqlalchemy 3.x break reverted in #42542.
Root cause theory: Flask-AppBuilder's FSA-3.x compatibility shim (
flask_appbuilder/models/sqla/base.py) doesn't wiremodel_class=through when it constructsSQLA(), unlike its FSA-2.x shim (base_legacy.py), which explicitly rebinds Superset's models onto FAB's owndeclarative_base()viacreate_session()/get_bind()overrides. Under FSA 3.x, Superset's models (all subclassingflask_appbuilder.Model) anddb.session's bind resolution end up looking at two disconnected declarative registries.This branch tests whether the fix is entirely Superset-side: passing
model_class=flask_appbuilder.models.sqla.Modelexplicitly when constructingdb = get_sqla_class()()insuperset/extensions/__init__.py. Confirmed locally that this unifiesdb.Modelwithflask_appbuilder.Model(db.Model is FABModelisTrue). Local pytest against sqlite doesn't reproduce the original break either way (matches #42542's own finding that this only ever surfaced under real CI), so this PR exists purely to get a CI signal ontest-sqlite/test-mysql.Full context: discussion #40273, step 6.
TESTING INSTRUCTIONS
N/A, investigation only.
ADDITIONAL INFORMATION