Permission Performance Optimizations #2341
Conversation
Moved PermissionsDTO to PermissionManager Removed GSON to use the app-wide ObjectMapper from Jackson
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.fasterxml.jackson.databind.node.ArrayNode; | ||
| import com.google.gson.JsonArray; | ||
| import com.google.gson.JsonObject; |
There was a problem hiding this comment.
com.google.gson was removed from pom.xml
There was a problem hiding this comment.
This was partially addressed: there is a gson import related to google http client, somehow related to GoogleBigQuery. The confusion is: there's a install-file goal to include httpclient-gson, but then also a maven dependency to the same library, but different version. Probably redundant or something was missed, but we should minimize install-file goals as they complicate the build process. Will not address in this PR.
| @@ -305,7 +328,51 @@ public Set<PermissionEntity> getUserPermissions(UserEntity user) { | |||
|
|
|||
| return permissions; | |||
| } | |||
|
|
|||
| public PermissionsDTO queryUserPermissions(final String login) { | |||
There was a problem hiding this comment.
@chrisknoll should getUserPermissions be deprecated in favor of this method?
There was a problem hiding this comment.
Depends:
the getuserPermissions is returning PermissionEntity...which is the JPA Entity Bean that represents the row of data in the DB, and fetchign permissions this way involves getting the roles from the user, iterating through each role and adding each permission from those roles into a final Set of PermissionEntity. The use of Set de-dupes permissions across roles.
queryUserPermissions provides more of a 'view' of distinct permissions associated with a user bypassing the JPA interface of sweeping through roles. It also structures the result to be 'indexed' such that you can quickly navigate to specific permissions that begin with a certain term.
The inputs to each of these are a bit different too: the former wants a ID or UserEntity, the latter works off of a login, and this is just based on the use-case of when you'd use one or the other. When we get to the point that we can clean up the API/object model in the 3.x release we can normalize and simplify a lot of this to a more simple, straight forward implementation.
| @@ -114,6 +116,8 @@ public User getCurrentUser() throws Exception { | |||
| user.login = currentUser.getLogin(); | |||
| user.name = currentUser.getName(); | |||
| user.permissions = convertPermissions(permissions); | |||
| user.permissionIdx = authorizer.queryUserPermissions(currentUser.getLogin()).permissions; | |||
There was a problem hiding this comment.
does this mean user.permissions will no longer be needed?
There was a problem hiding this comment.
It's unclear. When I added the code, I was concerned with not breaking backwards compatibility. While I did do a search on where the user.permissions field was referenced, it doesn't find those uses by downstream apps like Atlas (ie: the value is populated, returned to Atlas via webAPI call, so removing it will break it there).
When we move into the 3.0 WebAPI changes, ( #2354) we can be more aggressive about removing obsolete/unused code.
Implemented permission lookup to bypass hibernate.
Moved PermissionsDTO to PermissionManager
Removed GSON to use the app-wide ObjectMapper from Jackson
Required for OHDSI/Atlas#2912.