Repository navigation
fix(mcp): support custom user models during dashboard and chart operations - #45036
FrancescoCastaldi wants to merge 14 commits into
Conversation
… function signature
…urity_manager.user_model
…p session handling
Safely detect mock models in ExcludeUsersFilter and fall back to datamodel or FAB User to prevent SQLAlchemy ArgumentError in unit tests and mock contexts.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| viewers=get_default_viewers_for_groups( | ||
| list(getattr(new_user, "groups", []) or []) | ||
| ), |
There was a problem hiding this comment.
Suggestion: When a user is inserted with groups assigned in memory, this reloaded user has no persisted group links yet, so the copied dashboard omits its group viewers.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incorrect variable usage
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/dashboard.py
**Line:** 110:112
**Comment:**
*Incorrect Variable Usage: When a user is inserted with groups assigned in memory, this reloaded user has no persisted group links yet, so the copied dashboard omits its group viewers.
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 fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 1c9308e.
The dashboard copy now derives group viewers from target.groups directly instead of reloading the user and relying on persisted group links.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of a857ad3.
Group viewers are now derived from the inserted target’s in-memory groups and rebound into the active session, rather than relying on reloaded group links.
If that's not right, unresolve this thread and CodeAnt will leave it open.
CodeAnt PR Risk: Low Risk
Assessed commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45036 +/- ##
==========================================
+ Coverage 82.47% 82.66% +0.18%
==========================================
Files 3002 3011 +9
Lines 186172 191723 +5551
Branches 43128 44399 +1271
==========================================
+ Hits 153550 158492 +4942
- Misses 29867 30200 +333
- Partials 2755 3031 +276
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:
|
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hey @FrancescoCastaldi, thanks for picking this one up, and good instinct on the isolated session. I can confirm the pre-existing session.commit() inside after_insert is broken on master for everyone, not just for custom user models: on a5ffac5 with DASHBOARD_TEMPLATE_ID set, sm.add_user(...) logs Error adding new user to database. This transaction is closed, returns False and writes no user at all, while on your branch the user is created and the copy lands. That is a real fix and it should go in.
I am holding it on three things in the implementation, all inline: two production branches that detect unittest.mock objects, and the isolated session not being able to hold the Subject objects that editors and viewers are built from, which only stays quiet right now because both lists come out empty.
One thing outside the diff that I would value your read on. #45023's traceback comes from superset/mcp_service/auth.py handing a base FAB User to Slice.last_saved_by, and on master that file no longer queries User itself: it delegates to SupersetSecurityManager.find_user_with_relationships (superset/mcp_service/auth.py:595), which queries self.user_model (superset/security/manager.py:5575). I wired up a custom security manager with a ProbeUser(User) model on ab_user and that call returned a ProbeUser, whereas on the 6.1.0 tag the same file did db.session.query(User) directly at line 163. So I suspect the reported FlushError was already fixed by the API key refactor in #39604, and that what you have found here is a separate (and real) bug in the template copy path. Might be worth checking with @lhpaoletti whether master still reproduces before sizing this PR around that issue. I could easily be missing a route that still yields a base User, so please push back if you see one.
All of the above was run locally against 95a5e0a in a throwaway venv. Full tests/unit_tests on your head: 3 failed, 21895 passed; on the merge base a5ffac5: 3 failed, 21889 passed, identical failed set, and all three are arm64 and babel environment failures unrelated to this PR. On top of that I copied the six new tests onto the base, and drove real inserts through sm.add_user and db.session.add on a sqlite app with ENABLE_VIEWERS and ASSIGN_CREATOR_GROUPS_AS_VIEWERS enabled. CI here is fully green (57 passing, 14 skipped), which is consistent with what I found: the test that would have caught the viewers regression is the one the sqla.inspect branch routes around.
Happy to dig into any of this with you, and if the session fix I sketched looks right I can share the exact patch I ran.
| _copy_dashboard_for_user(target_session, target, dashboard_id) | ||
| return | ||
|
|
||
| with Session(bind=_connection) as session: # pylint: disable=disallowed-name |
There was a problem hiding this comment.
This block worries me a bit. The Dashboard built just above goes into this new session, but its editors and viewers are Subject instances resolved on db.session: get_user_subject queries db.session (superset/subjects/utils.py:271) and subjects_from_groups does the same (superset/subjects/utils.py:410). A persistent object cannot be cascaded into a second session, so session.add(dashboard) plus session.flush() raises as soon as either list is non-empty.
It stays quiet on this branch only because both lists come back empty today (I measured editors=[] viewers=[] on a real insert). To check the path rather than the emptiness I left your code untouched and only made get_user_subject return a real db.session attached Subject, which is what it does whenever the user's subject row exists. sm.add_user(...) then fails:
ERROR:flask_appbuilder.security.sqla.manager:Error adding new user to database.
Object '<Subject at 0x...>' is already attached to session '3' (this is '5')
FAB swallows that and add_user returns False, so no user and no copy are written. The same thing happens if the viewers line is fixed to read the groups off the in-memory target, which is the fix for the bot's comment above, so closing that one uncovers this one.
Going back to the flushing session is not the way out either, I tried it and got InvalidRequestError: Session is already flushing swallowed the same way. What worked for me was keeping your isolated session and re-fetching the subjects inside it:
from superset.subjects.models import Subject
def _rebind(subjects: list[Subject | None]) -> list[Subject]:
"""The helpers resolve subjects on ``db.session``; a persistent object
cannot be cascaded into a second session."""
ids = [s.id for s in subjects if s is not None]
return session.query(Subject).filter(Subject.id.in_(ids)).all() if ids else []
editors = _rebind([get_user_subject(target.id)])
viewers = _rebind(
get_default_viewers_for_groups(list(getattr(target, "groups", []) or []))
)With that, a user inserted carrying one group that has a Subject row writes cleanly and the copy comes out with viewers=[3], which is the behaviour tests/unit_tests/subjects/test_creator_group_call_sites.py::test_copy_dashboard_attaches_viewers_from_the_users_in_memory_groups exists to pin.
For the regression test I think it has to use a real session: insert a user with one group that has a Subject row, DASHBOARD_TEMPLATE_ID pointing at a real dashboard, then assert the copy's viewers is that group's subject. A Mock session cannot fail this way, which is why the new tests here pass.
| return | ||
|
|
||
| # Check if sqla.inspect is mocked (for compatibility with legacy tests) | ||
| if hasattr(sqla.inspect, "mock_calls"): |
There was a problem hiding this comment.
This is the one I would most like to see go. It makes production behaviour depend on whether sqlalchemy.inspect happens to be a unittest.mock object, and what it keeps alive is tests/unit_tests/subjects/test_creator_group_call_sites.py::test_copy_dashboard_attaches_viewers_from_the_users_in_memory_groups. I checked by deleting just these seven lines: that test is the only one in the file that then fails.
The effect is that the one test asserting "viewers are resolved from the in-memory collection, never by querying membership" keeps running the old sqla.inspect(target).session path, while every real insert takes the Session(bind=_connection) path below, where new_user is a fresh DB load and its ab_user_group rows have not been written yet. So the invariant stays green in CI and stops holding at runtime. Measured on this branch with ENABLE_VIEWERS and ASSIGN_CREATOR_GROUPS_AS_VIEWERS on, a configured template, and a user inserted with one group that has a Subject row: the copy comes out viewers=[], where reading the groups off target gives viewers=[3].
Could we drop the branch and rewrite that test against the new structure? It patches dashboard_module.sqla.inspect and dashboard_module.Dashboard, neither of which the new path uses, so it needs updating either way.
| arg_name = "username" | ||
|
|
||
| @staticmethod | ||
| def _is_mock(obj: Any) -> bool: |
There was a problem hiding this comment.
Same family as the sqla.inspect branch, and I would rather unittest.mock not be imported into the security manager at all. _get_username_column runs on the user list request path through SupersetUserApi.base_filters, so this makes a request path behave differently for tests.
I checked what it is protecting and it is three of the four tests in tests/unit_tests/security/exclude_users_filter_test.py, which patch superset.security.manager.current_app with a bare MagicMock(). With _is_mock short-circuited to False they fail with ArgumentError: SQL expression for WHERE/HAVING role expected, got <MagicMock name='mock.appbuilder.sm.user_model.username.not_in()'>. Adding one line, mock_sm.user_model = User, to each of those three makes all four pass with _is_mock and _get_username_column deleted outright. I ran it: 4 passed.
Which raises whether the ExcludeUsersFilter change belongs in this PR at all. For the model shape in #45023 (a subclass with __tablename__ = "ab_user") I could not find a behaviour difference: User.username.not_in([...]) and CustomUser.username.not_in([...]) both compile to (ab_user.username NOT IN ('x')). Dropping it would take 45 lines out of manager.py and keep this PR on the template copy bug, which is the part I think is genuinely broken.
| return dash | ||
|
|
||
|
|
||
| def test_exclude_users_filter_with_custom_user_model( |
There was a problem hiding this comment.
Just so you know, this one passes unchanged on the merge base (a5ffac5), so it is not pinning the ExcludeUsersFilter change. CustomUserModel.username and User.username resolve to the same ab_user.username column here, so filter_arg.left matches either way.
I copied all six new tests onto the base out of curiosity: four fail there and all four fail on ImportError: cannot import name 'register_dashboard_copy_events' or AttributeError: module 'superset.models.dashboard' does not have the attribute 'Session', which is the new symbols not existing yet rather than the bug reproducing. None of them drives a real session or a real alternative user model, which is why the cross-session problem I flagged on superset/models/dashboard.py:146 is invisible to them.
|
Thanks @gabotorresruiz for the thorough review and spot-on analysis. Pushed commit 1c9308e addressing the feedback:
All 46 unit tests in |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @FrancescoCastaldi, this covers all three of the things I was holding on. The subjects are rebound inside the isolated session, both unittest.mock branches are gone, and superset/security/manager.py is out of the diff entirely. Approving so I am not in your way.
pre-commit is still red on two mypy errors, so it cannot merge as is. I left a suggestion for each, plus two non blocking notes.
Still curious whether #45023 itself reproduces on master after the API key refactor, but that is a question about the issue and not about this fix.
|
|
||
| session = sqla.inspect(target).session # pylint: disable=disallowed-name | ||
| new_user = session.query(User).filter_by(id=target.id).first() | ||
| def _rebind(subjects: list[Subject | None]) -> list[Subject]: |
There was a problem hiding this comment.
pre-commit fails here: list is invariant, so the viewers call below cannot pass its list[Subject] into a list[Subject | None] parameter. mypy's own note suggests Sequence:
| def _rebind(subjects: list[Subject | None]) -> list[Subject]: | |
| def _rebind(subjects: Sequence[Subject | None]) -> list[Subject]: |
with from collections.abc import Sequence added at the top of the module.
| if not template: | ||
| return | ||
|
|
||
| editors = _rebind([get_user_subject(target.id)]) |
There was a problem hiding this comment.
Not a blocker, and it predates this PR: on a real insert this comes out [], because the user's Subject row is written after the user itself, so get_user_subject(target.id) finds nothing at after_insert time. Did you ever get a non empty editors here? If not, maybe worth a follow up issue rather than keeping the lookup.
| MagicMock(), | ||
| MagicMock(), | ||
| SimpleNamespace(id=5), # type: ignore[arg-type] | ||
| user, # type: ignore[arg-type] |
There was a problem hiding this comment.
The other mypy error: this ignore is unused now that copy_dashboard takes target: Any.
| user, # type: ignore[arg-type] | |
| user, |
| assert extra_attributes.user_id == 55 | ||
|
|
||
|
|
||
| def test_copy_dashboard_rebinds_subjects_in_isolated_session( |
There was a problem hiding this comment.
This one does pin _rebind: drop it from _copy_dashboard_for_user and this is the test that fails, so thank you for adding it.
Non blocking, but all six new tests fail on master only because Session and register_dashboard_copy_events do not exist there yet, so none of them would catch the cross-session bug coming back: a Mock session cannot raise is already attached to session. The shape that can is a real one, a Group with a Subject row, a user inserted carrying that group, DASHBOARD_TEMPLATE_ID pointing at a real dashboard, then assert the copy's viewers is that group's subject. Happy to share the harness I used if it helps.
|
@gabotorresruiz I wanted to test it on master a couple of hours ago, but I might have run into migration issues regarding MCP-authentication, because it's not working right away with the OAuth configuration we have set up. I'll give it another go tomorrow, and if it works and is still relevant, I'll post it here. Anyone else is also welcome to test it! |
|
Hi @gabotorresruiz, Thanks for the suggestions! I have applied both fixes in commit a857ad3:
All 42 unit tests in Regarding Thanks again for the thorough review and guidance! Francesco |
This branch has not been deployed
3 participants
SUMMARY
Fixes #45023.
When using a custom User model extended from Flask-AppBuilder, MCP chart creation and dashboard copy operations failed due to static references to User in SQLAlchemy event listeners and query filters, causing FlushError and unmapped table errors.
This change:
TESTING INSTRUCTIONS
Run the dashboard unit tests:
pytest tests/unit_tests/models/dashboard_test.py
All tests pass, including unit tests verifying copy operations with custom extended user models.
ADDITIONAL INFORMATION