sha174n commented on PR #41404:
URL: https://github.com/apache/superset/pull/41404#issuecomment-4811486855
Thanks for this, good catch. `PermissionView` indeed has no `name` attribute
(it's composed from `permission.name` + `view_menu.name`), so the previous
`perm.name` access would raise on real roles. The `"<perm> on <view>"` format
also lines up with FAB's own repr, which is nice.
Two small things before it goes in:
**1. Simplify the iteration.** The manual `iter()` / `while True` / `next()`
with try/except at each level can be a plain `for` loop with one inner
try/except and an outer guard, which does the same job and is easier to follow:
```python
permissions: list[str] | None = None
try:
raw_permissions = role.permissions
except DetachedInstanceError:
raw_permissions = None
if raw_permissions is not None:
permissions = []
try:
for permission in raw_permissions:
try:
name = _serialize_permission_name(permission)
except (DetachedInstanceError, TypeError):
continue
if name is not None:
permissions.append(name)
except (DetachedInstanceError, TypeError):
pass # detached / non-iterable relationship
```
**2. Don't swallow the detached case silently.** When `permissions` comes
back `None` because the relationship was detached, a `logger.debug(...)` would
make an empty/missing-permissions response traceable instead of silently
dropping it.
Otherwise this looks solid, and the test coverage across the edge cases is
great. Thanks for contributing!
--
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]
