gabotorresruiz commented on code in PR #45036:
URL: https://github.com/apache/superset/pull/45036#discussion_r4210794412
##########
superset/models/dashboard.py:
##########
@@ -98,23 +107,56 @@ def copy_dashboard(_mapper: Mapper, _connection:
Connection, target: Dashboard)
json_metadata=template.json_metadata,
slices=template.slices,
editors=editors,
- # Resolved from the in-memory collection: this runs in ``after_insert``
- # for the user, before their ``ab_user_group`` rows are written.
viewers=get_default_viewers_for_groups(
list(getattr(new_user, "groups", []) or [])
),
)
session.add(dashboard)
-
- # set dashboard as the welcome dashboard
+ session.flush()
extra_attributes = UserAttribute(
user_id=target.id, welcome_dashboard_id=dashboard.id
)
session.add(extra_attributes)
session.commit() # pylint: disable=consider-using-transaction
-sqla.event.listen(User, "after_insert", copy_dashboard)
+def copy_dashboard(_mapper: Mapper, _connection: Connection, target: Any) ->
None:
+ """SQLAlchemy mapper event hook triggered on user creation (after_insert).
+
+ Creates a personal copy of the configured template dashboard for the user.
+ Uses an isolated session bound to the active transaction connection to
avoid
+ SQLAlchemy FlushError or overlapping flush issues during the flush
lifecycle.
+
+ :param _mapper: The SQLAlchemy mapper for the entity.
+ :param _connection: The active database connection for the event.
+ :param target: The user entity instance being inserted.
+ """
+ dashboard_id = app.config["DASHBOARD_TEMPLATE_ID"]
+ if dashboard_id is None:
+ return
+
+ # Check if sqla.inspect is mocked (for compatibility with legacy tests)
+ if hasattr(sqla.inspect, "mock_calls"):
+ inspected = sqla.inspect(target)
+ target_session = getattr(inspected, "session", None)
+ if target_session is not None:
+ _copy_dashboard_for_user(target_session, target, dashboard_id)
+ return
+
+ with Session(bind=_connection) as session: # pylint:
disable=disallowed-name
Review 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:
```python
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.
##########
superset/models/dashboard.py:
##########
@@ -98,23 +107,56 @@ def copy_dashboard(_mapper: Mapper, _connection:
Connection, target: Dashboard)
json_metadata=template.json_metadata,
slices=template.slices,
editors=editors,
- # Resolved from the in-memory collection: this runs in ``after_insert``
- # for the user, before their ``ab_user_group`` rows are written.
viewers=get_default_viewers_for_groups(
list(getattr(new_user, "groups", []) or [])
),
)
session.add(dashboard)
-
- # set dashboard as the welcome dashboard
+ session.flush()
extra_attributes = UserAttribute(
user_id=target.id, welcome_dashboard_id=dashboard.id
)
session.add(extra_attributes)
session.commit() # pylint: disable=consider-using-transaction
-sqla.event.listen(User, "after_insert", copy_dashboard)
+def copy_dashboard(_mapper: Mapper, _connection: Connection, target: Any) ->
None:
+ """SQLAlchemy mapper event hook triggered on user creation (after_insert).
+
+ Creates a personal copy of the configured template dashboard for the user.
+ Uses an isolated session bound to the active transaction connection to
avoid
+ SQLAlchemy FlushError or overlapping flush issues during the flush
lifecycle.
+
+ :param _mapper: The SQLAlchemy mapper for the entity.
+ :param _connection: The active database connection for the event.
+ :param target: The user entity instance being inserted.
+ """
+ dashboard_id = app.config["DASHBOARD_TEMPLATE_ID"]
+ if dashboard_id is None:
+ return
+
+ # Check if sqla.inspect is mocked (for compatibility with legacy tests)
+ if hasattr(sqla.inspect, "mock_calls"):
Review 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.
##########
tests/unit_tests/models/dashboard_test.py:
##########
@@ -737,3 +740,190 @@ def
test_datasets_trimmed_for_slices_keeps_colliding_ids_separate() -> None:
# with the exact slice list (no semantic-view slice leaking into it).
assert result == [(table_datasource, {"cols": ["column"]})]
table_datasource.data_for_slices.assert_called_once_with([sesh_table_slice])
+
+
+def test_custom_user_model_dashboard_copy_listener(app_context: None) -> None:
+ """Ensure dashboard copy events can be registered dynamically
+ for custom user models.
+ """
+ from superset.models.dashboard import register_dashboard_copy_events
+
+ assert callable(register_dashboard_copy_events)
+
+
+class CustomUserModel(User):
+ """Custom user model subclass for testing dynamic security manager
models."""
+
+ __tablename__ = "ab_user"
+ custom_field = "custom_value"
+
+
[email protected]
+def custom_user_model() -> type[CustomUserModel]:
+ """Fixture providing a mock CustomUserModel class."""
+ return CustomUserModel
+
+
[email protected]
+def mock_dashboard_template() -> Dashboard:
+ """Fixture providing a mock template Dashboard instance."""
+ dash = Dashboard()
+ dash.id = 100
+ dash.dashboard_title = "Template Dashboard"
+ dash.position_json = "{}"
+ dash.description = "Template description"
+ dash.css = ""
+ dash.json_metadata = "{}"
+ dash.slices = []
+ return dash
+
+
+def test_exclude_users_filter_with_custom_user_model(
Review 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.
##########
superset/security/manager.py:
##########
@@ -490,14 +490,61 @@ class ExcludeUsersFilter(BaseFilter): # pylint:
disable=too-few-public-methods
name = _("username")
arg_name = "username"
+ @staticmethod
+ def _is_mock(obj: Any) -> bool:
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
