korbit-ai[bot] commented on code in PR #32770:
URL: https://github.com/apache/superset/pull/32770#discussion_r2005238411
##########
superset/utils/core.py:
##########
@@ -1290,6 +1290,18 @@
return g.user.email
except Exception: # pylint: disable=broad-except
return None
+
+def get_user_roles() -> list[str] | None:
+ """
+ Get the roles (if defined) associated with the current user.
+
+ :returns: The roles
+ """
Review Comment:
### Incomplete return value documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The docstring does not adequately explain the return value format and
failure case.
###### Why this matters
The function can return None or a list of role names, but this behavior is
not clearly documented making it harder for developers to handle all cases
correctly.
###### Suggested change ∙ *Feature Preview*
def get_user_roles() -> list[str] | None:
"""
Get the roles (if defined) associated with the current user.
:returns: List of role names if user is found and has roles, None if
user not found or no roles
"""
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/beb85d31-bb97-4086-ad35-ce47443c1c82/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/beb85d31-bb97-4086-ad35-ce47443c1c82?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/beb85d31-bb97-4086-ad35-ce47443c1c82?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/beb85d31-bb97-4086-ad35-ce47443c1c82?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/beb85d31-bb97-4086-ad35-ce47443c1c82)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:536e4058-9b98-4cf9-8da3-d3c74dbfb296 -->
[](536e4058-9b98-4cf9-8da3-d3c74dbfb296)
##########
superset/utils/core.py:
##########
@@ -1290,6 +1290,18 @@
return g.user.email
except Exception: # pylint: disable=broad-except
return None
+
+def get_user_roles() -> list[str] | None:
+ """
+ Get the roles (if defined) associated with the current user.
+
+ :returns: The roles
+ """
+
+ try:
+ return [role.name for role in g.user.roles]
+ except Exception: # pylint: disable=broad-except
+ return None
Review Comment:
### Missing Cache for User Roles <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The user roles are being fetched without any caching mechanism, potentially
causing unnecessary database queries on every template render.
###### Why this matters
Without caching, each call to get_user_roles() will trigger a database
query, creating performance overhead especially in templates where this could
be called multiple times.
###### Suggested change ∙ *Feature Preview*
Implement caching for user roles using Flask-Caching or a similar mechanism.
Store the roles in cache with a reasonable TTL. Example:
```python
@cache.memoize(timeout=300) # Cache for 5 minutes
def get_user_roles() -> list[str] | None:
try:
return [role.name for role in g.user.roles]
except Exception:
return None
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f813366e-1650-4800-832e-9352346ddee3/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f813366e-1650-4800-832e-9352346ddee3?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f813366e-1650-4800-832e-9352346ddee3?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f813366e-1650-4800-832e-9352346ddee3?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f813366e-1650-4800-832e-9352346ddee3)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fb643427-5e90-4842-9474-a8786fcb1c20 -->
[](fb643427-5e90-4842-9474-a8786fcb1c20)
##########
superset/utils/core.py:
##########
@@ -1290,6 +1290,18 @@ def get_user_email() -> str | None:
return g.user.email
except Exception: # pylint: disable=broad-except
return None
+
+def get_user_roles() -> list[str] | None:
+ """
+ Get the roles (if defined) associated with the current user.
+
+ :returns: The roles
+ """
+
+ try:
+ return [role.name for role in g.user.roles]
+ except Exception: # pylint: disable=broad-except
+ return None
Review Comment:
### Silent Exception Handling in get_user_roles <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The get_user_roles() function silently catches all exceptions and returns
None without any logging, which could hide important errors.
###### Why this matters
If there are errors accessing user roles (e.g., database connection issues),
they will be hidden which makes debugging difficult in production environments.
###### Suggested change ∙ *Feature Preview*
Add error logging to help with debugging:
```python
def get_user_roles() -> list[str] | None:
try:
return [role.name for role in g.user.roles]
except Exception as ex: # pylint: disable=broad-except
logger.exception("Failed to get user roles: %s", str(ex))
return None
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0456ac0d-a402-48d2-9f32-e0ac344711a7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0456ac0d-a402-48d2-9f32-e0ac344711a7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0456ac0d-a402-48d2-9f32-e0ac344711a7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0456ac0d-a402-48d2-9f32-e0ac344711a7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0456ac0d-a402-48d2-9f32-e0ac344711a7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:55b7aa12-705e-4b2d-bcf1-161a9fd9dc81 -->
[](55b7aa12-705e-4b2d-bcf1-161a9fd9dc81)
--
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]
