Reduce logs in webapp container (depends on blackduck-common release, do not merge) #1390
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zahidblackduck
changed the title
zahidblackduck
changed the title
zahidblackduck
changed the title
zahidblackduck
changed the title
| UserGroupService userGroupService = blackDuckServicesFactory.createUserGroupService(); | ||
| List<RoleAssignmentView> roles = userGroupService.getServerRolesForUser(userView); | ||
| boolean isAdmin = checkIsAdmin(roles); | ||
| version = blackDuckRegistrationService.getBlackDuckServerData(isAdmin).getVersion(); |
There was a problem hiding this comment.
As discussed on a call, the greater intention of passing in isAdmin here is not clear.
To someone working with this code in the future it may look like the only thing this code cares about here is the version.
That could lead a developer down a path of refactoring, not realizing the importance of the side effect, and all of a sudden we will start making extra calls in other places again.
I think Ideally we would:
- Determine user roles once
- Fetch BlackDuckServerData based on it once
- Pass around this data to wherever its needed, including phone home
This is more work but I believe is more reliable and would minimize the number of API calls needed,.
| version = blackDuckRegistrationService.getBlackDuckServerData().getVersion(); | ||
| UserView userView = userService.findCurrentUser(); | ||
| UserGroupService userGroupService = blackDuckServicesFactory.createUserGroupService(); | ||
| List<RoleAssignmentView> roles = userGroupService.getServerRolesForUser(userView); |
There was a problem hiding this comment.
One thing to be careful about here is pagination.
I'm not sure what the maximum number of roles is, but it may be greater than the default page size.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA Ticket
IDETECT-4209
Description
This update aims to reduce excessive logs in hub webapp container when a non-sysadmin user tries to access registration key calling the endpoint
/api/registrationduring detect startup and phone-homing.Detect can determine the current user’s roles during startup by communicating with
blackduck-common. By checking if the user has sysadmin privileges, we can pass this status toBlackDuckRegistrationService#getBlackDuckServerData(isAdmin), reducing unnecessary calls.Solution Approach
getBlackDuckServerData()to accept anisAdminflag and pass it fromdetermineConnectivity().getRegistrationId()ifisAdminisfalseto avoid redundant requests.This fix depends on the release of
blackduck-commonlibrary containing the following code updates.blackducksoftware/blackduck-common#453