refactor: Issue #25040; Refactored sync_role_definition function in order to reduce number of query. #25340
Conversation
sandeep-patel11
changed the title
justinpark
requested review from
john-bodley,
villebro and
michael-s-molina
justinpark
added
the
review:checkpoint
superset/security/manager.py
Outdated
| pvms = self.get_session.query(PermissionView).options( | ||
| eagerload(PermissionView.permission), | ||
| eagerload(PermissionView.view_menu)).all() | ||
| pvms = [p for p in pvms if p.permission and p.view_menu] |
There was a problem hiding this comment.
Does this check do anything? These are required relations, aren't they?
There was a problem hiding this comment.
Yes this is required relation but as this condition was written in previous query I thought of not removing it.
There was a problem hiding this comment.
Ah yeah, I see you haven't changed this. Fair enough.
superset/security/manager.py
Outdated
| @@ -864,7 +872,7 @@ def copy_role( | |||
| self.get_session.commit() | |||
|
|
|||
| def set_role( | |||
| self, role_name: str, pvm_check: Callable[[PermissionView], bool] | |||
| self, role_name: str, pvm_check: Callable[[PermissionView], bool], pvms:[] | |||
There was a problem hiding this comment.
Type should be list[PermissionView] here?
There was a problem hiding this comment.
Correct. Changes Done.
justinpark
removed
the
review:checkpoint
superset/security/manager.py
Outdated
| eagerload(self.permissionview_model.view_menu), | ||
| ) | ||
| .all() | ||
| ): |
There was a problem hiding this comment.
One small thing, I see this is effectively same as the change in the other location, except here you're using self.permissionview_model and there you're using the name directly. I think using self.permissionview_model is the right way to go based on patterns I've seen elsewhere, but seems we probably need to stay DRY if we've already done it two different ways, and especially since the eagerload stuff is a bit fiddly.
Maybe just separate it into a def _get_all_pvms or similar? Gives us an opportunity to mention that we're doing it eagerly to avoid too many queries that way too.
superset/security/manager.py
Outdated
| ) | ||
| .all() | ||
| ) | ||
| pvms = [p for p in pvms if p.permission and p.view_menu] |
There was a problem hiding this comment.
pedantic comment but you could just return this line instead of updating the var and returning the var.
There was a problem hiding this comment.
Done, Thanks for the the reviews @giftig .
…n in order to reduce number of query. (apache#25340)
mistercrunch
added
🏷️ bot
…n in order to reduce number of query. (apache#25340)
…n in order to reduce number of query. (apache#25340)
SUMMARY
Earlier we were fetching PVMS one by one which made too many queries to database. So to avoid it I have added eagerload so that we can get all the data with single query.
Also for each role we were doing the same call so to avoid that I fetched all the data in main function and then passed it as parameter to set_role function.
TESTING INSTRUCTIONS
Inorder to test the functionality we need to add SQLALCHEMY_ECHO = True in config.py and then run superset init. Before this changes we will able to see too many queries getting logged. After refactoring number of queries have decreased.
ADDITIONAL INFORMATION