bito-code-review[bot] commented on code in PR #39604:
URL: https://github.com/apache/superset/pull/39604#discussion_r3291840190
##########
superset/security/manager.py:
##########
@@ -3164,6 +3169,62 @@ def get_user_by_username(self, username: str) ->
Optional[User]:
.one_or_none()
)
+ def find_user_with_relationships(
+ self,
+ username: Optional[str] = None,
+ email: Optional[str] = None,
+ ) -> Optional[User]:
+ """Find a user with roles and group roles eagerly loaded.
+
+ Mirrors FAB's ``SecurityManager.find_user``
+ (including ``auth_username_ci`` case-insensitive handling and
+ ``MultipleResultsFound`` guard) and additionally eager-loads
+ ``User.roles`` and ``User.groups.roles`` to prevent detached-instance
+ errors when the SQLAlchemy session is closed or rolled back after the
+ lookup — as happens in MCP tool-execution contexts.
+
+ FAB does not expose an eager-loading option on ``find_user``, so the
+ query logic is mirrored here with joinedload options added. Review this
+ method when upgrading FAB to ensure it stays in sync with upstream.
+ """
+ eager = [
+ joinedload(self.user_model.roles),
+
joinedload(self.user_model.groups).joinedload(self.group_model.roles),
+ ]
+ if username:
+ try:
+ if self.auth_username_ci:
+ return (
+ self.session.query(self.user_model)
+ .options(*eager)
+ .filter(
+ sa_func.lower(self.user_model.username)
+ == sa_func.lower(username)
+ )
+ .one_or_none()
+ )
+ return (
+ self.session.query(self.user_model)
+ .options(*eager)
+ .filter(self.user_model.username == username)
+ .one_or_none()
+ )
+ except MultipleResultsFound:
+ logger.error("Multiple results found for username lookup")
+ return None
+ if email:
+ try:
+ return (
+ self.session.query(self.user_model)
+ .options(*eager)
+ .filter_by(email=email)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Query API inconsistency: email vs username</b></div>
<div id="fix">
Email lookup at line 3220 uses `.filter_by(email=email)` while username
paths (lines 3209, 3201) use `.filter(self.user_model.username == ...)`. Mixing
query styles creates maintenance risk if the model attribute name changes or if
additional filters need to be added consistently.
</div>
</div>
<small><i>Code Review Run #3eb4cf</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
