korbit-ai[bot] commented on code in PR #33025:
URL: https://github.com/apache/superset/pull/33025#discussion_r2032024366
##########
superset/utils/log.py:
##########
@@ -193,19 +193,20 @@ def log_with_context( # pylint:
disable=too-many-locals,too-many-arguments
# Whenever a user is not bounded to a session we
# need to add them back before logging to capture user_id
- if user_id is None:
+ if user_id is None and has_request_context():
try:
- db.session.add(g.user)
- user_id = get_user_id()
- except Exception as ex: # pylint: disable=broad-except
- logging.warning(ex)
+ actual_user = g.get("user", None)
+ if actual_user is not None:
+ db.session.add(actual_user)
+ user_id = get_user_id()
+ except Exception as ex:
+ logging.warning("Failed to add user to db session: %s", ex)
user_id = None
-
- payload = collect_request_payload()
- if object_ref:
- payload["object_ref"] = object_ref
- if payload_override:
- payload.update(payload_override)
+ payload = collect_request_payload()
+ if object_ref:
+ payload["object_ref"] = object_ref
+ if payload_override:
+ payload.update(payload_override)
Review Comment:
### Mixed concerns in exception handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code handling user session and payload collection is mixed together
inside an exception block, violating separation of concerns.
###### Why this matters
Mixing user session handling with payload collection in an error handler
makes the code harder to maintain and understand. It's unclear why payload
collection only happens in the error case.
###### Suggested change ∙ *Feature Preview*
Separate the user session handling and payload collection into distinct
methods:
```python
def ensure_user_in_session(self) -> int | None:
if not has_request_context():
return None
try:
actual_user = g.get("user", None)
if actual_user is not None:
db.session.add(actual_user)
return get_user_id()
except Exception as ex:
logging.warning("Failed to add user to db session: %s", ex)
return None
def get_payload(self, object_ref: str | None = None, payload_override: dict
| None = None) -> dict:
payload = collect_request_payload()
if object_ref:
payload["object_ref"] = object_ref
if payload_override:
payload.update(payload_override)
return payload
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc3e04dd-cff9-480b-8aac-8fb2ddbadd73/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc3e04dd-cff9-480b-8aac-8fb2ddbadd73?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc3e04dd-cff9-480b-8aac-8fb2ddbadd73?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc3e04dd-cff9-480b-8aac-8fb2ddbadd73?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc3e04dd-cff9-480b-8aac-8fb2ddbadd73)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:022f795b-cd40-4282-947d-e171d1b4d308 -->
[](022f795b-cd40-4282-947d-e171d1b4d308)
--
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]
