-
Notifications
You must be signed in to change notification settings - Fork 546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve applications list performance #4348
base: master
Are you sure you want to change the base?
Conversation
This PR is initiated as a fix for the original issue of executing the |
} | ||
return appInfo; | ||
|
||
return ApplicationMgtUtil.filterApplicationsForUser(applicationBasicInfos, userName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ApplicationMgtUtil.filterApplicationsForUser(applicationBasicInfos, userName); | |
return ApplicationMgtUtil.filterUserAuthorizedApplications(applicationBasicInfos, userName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling another method from this method without any additional steps doesn't seem fine. Either the new method could have been call from where the getAuthorizedApplicationBasicInfo
method was called.
if (userStoreManager instanceof AbstractUserStoreManager) { | ||
try { | ||
|
||
String[] userRolesAbstractUSM = (((AbstractUserStoreManager) userStoreManager) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it is required to cast and invoke the getRoleListOfUser
method. The results would be same as Line 173.
String[] userRolesAbstractUSM = (((AbstractUserStoreManager) userStoreManager) | ||
.getRoleListOfUser(username)); | ||
|
||
// Merge the lists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the above comment is valid, there is no point of merging. Also these operations will be costly when there large number of roles assigned for the user.
+ " by retrieving role list of " + "user : " + username); | ||
} | ||
|
||
for (String userRole : userRoles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can improve the code using Java streams APIs
Proposed changes in this pull request
Fix for
getRoleListOfUser(userName)
for each application for the same user product-is#4678getRoleListOfUser(userName)
for each application for the same user #2114Instead of performing a call to the User store for each Application role, it is performed once.
On our installation it reduced the loading time of the Service Providers list by a large factor (x1000), as the user store we are using had large amount of application roles configured.
When should this PR be merged
ASAP
Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation