aminghadersohi commented on code in PR #42306:
URL: https://github.com/apache/superset/pull/42306#discussion_r3643420727
##########
superset/utils/log.py:
##########
@@ -193,11 +194,14 @@ def log_with_context( # pylint:
disable=too-many-locals,too-many-arguments
if user_id is None and has_request_context():
try:
actual_user = g.get("user", None)
- if actual_user is not None:
+ # Guest/anonymous users (e.g. embedded dashboards) are never
+ # DB-mapped, so adding them to the session always fails.
+ # This is expected and not worth logging.
+ if actual_user is not None and sa_inspect(actual_user,
raiseerr=False):
db.session.add(actual_user)
user_id = get_user_id()
Review Comment:
Thanks for flagging this — I want to make sure the `LocalProxy` handling is
actually correct, so I verified it empirically against this repo's exact pinned
dependencies (SQLAlchemy 1.4.54, Werkzeug 3.1.6, Flask-Login 0.6.3) rather than
relying on the general claim.
`sqlalchemy.inspect()` does **not** use a `type(obj)`-based mapper lookup
for arbitrary objects. Once `sqlalchemy.orm` is imported, it registers a
catch-all inspector for `object`:
```python
@inspection._inspects(object)
def _inspect_mapped_object(instance):
try:
return instance_state(instance)
except (exc.UnmappedClassError,) + exc.NO_STATE:
return None
```
`instance_state` is `operator.attrgetter("_sa_instance_state")` — an
**attribute-based** lookup, not a type-based one. `LocalProxy.__getattr__`
forwards arbitrary attribute access to the wrapped object, so
`instance._sa_instance_state` on a proxy resolves through to the underlying
object's `_sa_instance_state` just fine.
Reproduced directly against this repo's environment:
```python
from werkzeug.local import LocalProxy
from sqlalchemy import inspect as sa_inspect
from flask_appbuilder.security.sqla.models import User
from flask_login import AnonymousUserMixin
mapped_user = User()
sa_inspect(LocalProxy(lambda: mapped_user), raiseerr=False)
# -> <sqlalchemy.orm.state.InstanceState object at 0x...> (truthy — add()
proceeds)
sa_inspect(LocalProxy(lambda: AnonymousUserMixin()), raiseerr=False)
# -> None (falsy — add() correctly skipped for guest/anonymous users)
```
So `sa_inspect(actual_user, raiseerr=False)` already unwraps the proxy
correctly in both directions — mapped users behind the proxy are detected as
mapped, and unmapped guest/anonymous users are correctly detected as unmapped.
This is in fact the whole point of the fix: the *original* bug came from
`db.session.add()`'s internal `UnmappedInstanceError`, which builds its message
from `type(obj)` directly (bypassing proxy unwrapping) — that mismatch is
exactly why the warning fired for every guest/anonymous request. Using
`sqlalchemy.inspect()` (attribute-based) instead of relying on
`session.add()`'s exception path is what fixes it correctly.
Manually unwrapping with `_get_current_object()` first would be redundant
here (same outcome, no functional difference) and adds a dependency on a
private/internal werkzeug API for no benefit, so I'm not planning to apply it —
happy to revisit if you have a concrete counter-example where `sa_inspect`
misses a mapped instance behind a proxy in practice.
--
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]
