villebro commented on a change in pull request #12865:
URL: https://github.com/apache/superset/pull/12865#discussion_r570035328
##########
File path: superset/commands/utils.py
##########
@@ -16,38 +16,55 @@
# under the License.
from typing import List, Optional
-from flask_appbuilder.security.sqla.models import User
+from flask_appbuilder.security.sqla.models import Role, User
from superset.commands.exceptions import (
DatasourceNotFoundValidationError,
OwnersNotFoundValidationError,
+ RolesNotFoundValidationError,
)
from superset.connectors.base.models import BaseDatasource
from superset.connectors.connector_registry import ConnectorRegistry
from superset.datasets.commands.exceptions import DatasetNotFoundError
from superset.extensions import db, security_manager
-def populate_owners(user: User, owners_ids: Optional[List[int]] = None) ->
List[User]:
+def populate_owners(user: User, owner_ids: Optional[List[int]] = None) ->
List[User]:
"""
Helper function for commands, will fetch all users from owners id's
Can raise ValidationError
:param user: The current user
- :param owners_ids: A List of owners by id's
+ :param owner_ids: A List of owners by id's
"""
owners = list()
- if not owners_ids:
+ if not owner_ids:
return [user]
- if user.id not in owners_ids:
+ if user.id not in owner_ids:
owners.append(user)
- for owner_id in owners_ids:
+ for owner_id in owner_ids:
owner = security_manager.get_user_by_id(owner_id)
if not owner:
raise OwnersNotFoundValidationError()
owners.append(owner)
return owners
+def populate_roles(role_ids: Optional[List[int]] = None) -> List[Role]:
+ """
+ Helper function for commands, will fetch all roles from roles id's
+ :raises RolesNotFoundValidationError: If a role in the input list is not
found
+ :param role_ids: A List of roles by id's
+ """
+ roles = list()
Review comment:
I'm surprised mypy didn't complain here (maybe it's smarter nowadays
than it was before). I'm also allergic to `list()` vs `[]`, but feel free to
ignore
```suggestion
roles: List[Role] = []
```
##########
File path: superset/security/manager.py
##########
@@ -656,6 +657,11 @@ def _get_pvms_from_builtin_role(self, role_name: str) ->
List[PermissionView]:
role_from_permissions.append(pvm)
return role_from_permissions
+ def find_role_by_id(self, role_id: int) -> Any:
+ return (
+
self.get_session.query(self.role_model).filter_by(id=role_id).one_or_none()
+ )
Review comment:
As we're actually doing a for-loop when retrieving these, to avoid
unnecessary query overhead it might be better to do `find_roles_by_id(self,
role_ids: List[int]) -> List[Role]`. Check
https://github.com/apache/superset/blob/77093a874e8c4c81c6e3b8557bca545c3fc0f3a2/superset/dao/base.py#L49-L77
for the DAO implementation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
