Yair Zaslavsky has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 13: (3 comments) http://gerrit.ovirt.org/#/c/28561/13/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/sync/SyncAAAEntries.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/sync/SyncAAAEntries.java: Line 32: // 5. Resolve recursively all groups Line 33: // 6. Compare the user from db and the fetched user from the directory + store in DB if there is a change Line 34: public void sync( Line 35: Map<String, Map<String, Set<String>>> userIdsToFetch, Line 36: Set<String> activeGroupIds, > this is not required as it can be concluded from the output, no? see my comment on DirectoryUtils - yes, this can be calculated by the callers, but why not provide this info? the current callers are Login,DbUserCacheManager, AddUser , imho all will require to update the inactive list in db, no? Line 37: Map<String, DirectoryGroup> outDirectoryGroupsByIds, Line 38: Map<String, DirectoryUser> outDirectoryUserByIds, Line 39: List<String> outNonActiveUserIds, Line 40: List<String> outNonActiveGroupIds) { http://gerrit.ovirt.org/#/c/28561/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/aaa/DirectoryUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/aaa/DirectoryUtils.java: Line 80: Line 81: for (String id : outNonActiveGroupExternalIds) { Line 82: dbGroupsPerExternalId.get(id).setActive(false); Line 83: DbFacade.getInstance().getDbGroupDao().update(dbGroupsPerExternalId.get(id)); Line 84: } > if you have outDirectoryUsers and you have a collection of all database use well, since sync is used from several places in code (DbUserCacheManager, AddUser,Login...) why repeat the logic of understanding what is missing, and not have the calculation inside sync and have it set on "out" variable? I can of course add another method at DirectoryUtils to calculate the inactive elements, but not sure if it worth the hassle. Line 85: Line 86: for (DbUser dbUser : dbUsers) { Line 87: if (dbUser.isActive()) { Line 88: DirectoryUser user = outDirectoryUserByIds.get(dbUser.getExternalId()); Line 86: for (DbUser dbUser : dbUsers) { Line 87: if (dbUser.isActive()) { Line 88: DirectoryUser user = outDirectoryUserByIds.get(dbUser.getExternalId()); Line 89: Set<DirectoryGroup> accumulator = new HashSet<>(); Line 90: DirectoryUtils.flatGroups(accumulator, user.getGroups(), outDirectoryGroupsByIds); > once again... you are mixing the layers. ok. Line 91: user.setGroups(new ArrayList<DirectoryGroup>(accumulator)); Line 92: DbUser userFromDirectory = new DbUser(user); Line 93: userFromDirectory.setGroupIds(DirectoryUtils.getGroupIdsFromUser(user, outDirectoryGroupsByIds)); Line 94: if (!userFromDirectory.equals(dbUser)) { -- To view, visit http://gerrit.ovirt.org/28561 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id49b51517a967c7a83e8e73f52181673baa31700 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
