Repository navigation
fix(logging): stop noisy LocalProxy-not-mapped warning for guest users - #42306
Conversation
Guest and anonymous users (e.g. embedded dashboards) are never DB-mapped SQLAlchemy instances, so log_with_context()'s attempt to add them to the DB session was always doomed to fail and log a WARNING. Check sqlalchemy.inspect(actual_user, raiseerr=False) before attempting the add, and skip both the add and the log line for this expected case. Any other genuinely unexpected exception is now logged at debug level via this module's logger instead of the raw logging module. Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42306 +/- ##
=======================================
Coverage 65.26% 65.26%
=======================================
Files 2794 2794
Lines 157525 157526 +1
Branches 36039 36039
=======================================
+ Hits 102806 102813 +7
+ Misses 52741 52735 -6
Partials 1978 1978
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:
|
dosubot
Bot
added
the
logging
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if actual_user is not None and sa_inspect(actual_user, raiseerr=False): | ||
| db.session.add(actual_user) | ||
| user_id = get_user_id() |
There was a problem hiding this comment.
Suggestion: This mapped-instance check is performed against g.user directly, but in Flask-AppBuilder g.user is often a LocalProxy; SQLAlchemy inspection on the proxy can return not-mapped even when the underlying authenticated user model is mapped. That causes the rebind path to be skipped, so detached real users can still end up with missing user_id in logs. Unwrap the proxy to the underlying user object before calling SQLAlchemy inspection and adding to the session. [logic error]
Severity Level: Major ⚠️
- WARNING Event logging loses user_id in detached-user scenarios.
- WARNING Audit trails missing user information for affected events.
- WARNING Harder to debug issues tied to specific users.Steps of Reproduction ✅
1. Start Superset with this PR code and authenticate a real user so that a request runs
with `has_request_context()` true and `g.user` populated (Flask-AppBuilder before-request
hook; log_with_context in `superset/utils/log.py` around lines 189–205).
2. Arrange for `get_user_id()` (in `superset/utils/core.py`, called at
`superset/utils/log.py:190`) to return `None` even though the underlying authenticated
user model is a valid, mapped SQLAlchemy instance (for example, by using a detached user
instance in `g.user`).
3. Trigger any path that calls `AbstractEventLogger.log_with_context()` so execution
reaches the block at `superset/utils/log.py:194-205` where `user_id is None and
has_request_context()` is true and `actual_user = g.get("user", None)` is a
`werkzeug.local.LocalProxy` wrapping the real user.
4. At `superset/utils/log.py:200`, `sa_inspect(actual_user, raiseerr=False)` is applied to
the `LocalProxy` instead of the underlying user; SQLAlchemy inspection treats the proxy
type as unmapped, the `if` condition is false, `db.session.add(actual_user)` is skipped,
and `user_id` remains `None`, so the resulting event log entry has no user ID even though
a real mapped user was present.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/log.py
**Line:** 200:202
**Comment:**
*Logic Error: This mapped-instance check is performed against `g.user` directly, but in Flask-AppBuilder `g.user` is often a `LocalProxy`; SQLAlchemy inspection on the proxy can return not-mapped even when the underlying authenticated user model is mapped. That causes the rebind path to be skipped, so detached real users can still end up with missing `user_id` in logs. Unwrap the proxy to the underlying user object before calling SQLAlchemy inspection and adding to the session.
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.
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:
@inspection._inspects(object)
def _inspect_mapped_object(instance):
try:
return instance_state(instance)
except (exc.UnmappedClassError,) + exc.NO_STATE:
return Noneinstance_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:
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.
|
The flagged issue is correct. In Flask-AppBuilder, Here is the corrected implementation for actual_user = g.get("user", None)
# Unwrap LocalProxy to get the underlying object
if hasattr(actual_user, "_get_current_object"):
actual_user = actual_user._get_current_object()
if actual_user is not None and sa_inspect(actual_user, raiseerr=False):
db.session.add(actual_user)
user_id = get_user_id()There are no other review comments in this PR to address. superset/utils/log.py |
|
@bito-code-review the underlying claim here is the same one raised in the inline review thread on Manually unwrapping with |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #dbdaca
Actionable Suggestions - 1
-
tests/integration_tests/event_logger_tests.py - 1
- Duplicate DummyEventLogger class definition · Line 239-239
Review Details
-
Files reviewed - 2 · Commit Range:
c48495a..ee37316- superset/utils/log.py
- tests/integration_tests/event_logger_tests.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| """Guest/anonymous users are never DB-mapped; adding them to the | ||
| session is expected to be a no-op, not a warning-worthy failure.""" | ||
|
|
||
| class DummyEventLogger(AbstractEventLogger): |
There was a problem hiding this comment.
This file contains syntactic duplication of the DummyEventLogger class definition, which appears at multiple locations: lines 113-134, 152-173, 202-223, 239-260, and 279-298. Consider extracting this common test utility to a shared fixtures module to reduce duplication and improve maintainability.
Code Review Run #dbdaca
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
SUMMARY
AbstractEventLogger.log_with_context()insuperset/utils/log.pylogsWARNING:root:Failed to add user to db session: Class 'werkzeug.local.LocalProxy' is not mappedwhenever it tries to bind the currentflask.g.userback into the DB session to resolve auser_id.flask_appbuilder'sbefore_requesthook setsg.user = current_user, which is awerkzeug.local.LocalProxywrapping the actual user object. For guest/anonymous users (e.g. embedded dashboards), the wrapped object is aGuestUser/AnonymousUserMixin, which is never a DB-mapped SQLAlchemy instance.db.session.add()on that proxy always raisesUnmappedInstanceError, which is caught and logged atWARNINGlevel — this is expected, permanent, and unrecoverable for guest/anonymous requests, not an actionable failure. In production this fires 1.3-1.8M times/day and accounts for ~63% of sampled WARN-level logs for the service.An earlier fix (#33025, closed #26020) added a
has_request_context()guard and a clearer message, but didn't address the root cause: guest/anonymous users can never be added to the session, so the warning was structurally guaranteed to keep firing.This PR:
sqlalchemy.inspect(actual_user, raiseerr=False)before attemptingdb.session.add(). If the user isn't a DB-mapped instance (the guest/anonymous case), the add and the log line are both skipped entirely — this is expected behavior, not an error.debuglevel via the module's ownlogger, instead ofWARNINGvia the rawloggingmodule.No other part of
log_with_contextor event-logging behavior is touched.Internal tracking: SC-114901
TESTING INSTRUCTIONS
tests/integration_tests/event_logger_tests.py::test_log_with_context_guest_user_skips_warning: setsflask.g.userto aGuestUserand asserts noWARNINGis logged.tests/integration_tests/event_logger_tests.py::test_log_with_context_unexpected_add_failure_logs_debug: forcesdb.session.add()to raise for a DB-mapped user and asserts the failure is logged atDEBUG(notWARNING).pytest tests/integration_tests/event_logger_tests.py tests/unit_tests/utils/log_tests.pypre-commit run --files superset/utils/log.py tests/integration_tests/event_logger_tests.pyADDITIONAL INFORMATION