Alon Bar-Lev has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 11: (9 comments) ok... very hard for me to follow... few comments. what I miss is the integration between the sync and adduser and addgroup... perform same mechanism. http://gerrit.ovirt.org/#/c/28561/11/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthzUtils.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthzUtils.java: Line 316: } Line 317: return directoryGroup; Line 318: } Line 319: Line 320: private static int queryFlagValue(boolean resolveGroups) { so you really can remove this function... no? Line 321: int result = 0; Line 322: if (resolveGroups) { Line 323: result |= Authz.QueryFlags.RESOLVE_GROUPS; Line 324: } http://gerrit.ovirt.org/#/c/28561/11/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 26: } Line 27: return results; Line 28: } Line 29: Line 30: public static void flatGroups(Set<DirectoryGroup> accumulator, Collection<DirectoryGroup> groupsFrom) { I am unsure you are using this one any more.... you have dup Line 31: for (DirectoryGroup group : groupsFrom) { Line 32: flatGroups(accumulator, group.getGroups()); Line 33: accumulator.add(group); Line 34: } http://gerrit.ovirt.org/#/c/28561/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java: Line 64: } Line 65: } Line 66: Line 67: /** Line 68: * Load all the users from the database and refresh them. The refresh logic is as follows : 1. Fetch users from DB please have each as new line 1. xxx 2. xxx 3. xxx Line 69: * 2. Resolve the groups of users (get 1st level groups) 3. Fetch groups from DB 4. Resolve the fetched groups (for Line 70: * each group that was not already resolved) 5. Resolve recursively all groups 6. Compare the user from db and the Line 71: * fetched user from the directory + store in DB if there is a change Line 72: */ Line 91: }; Line 92: Line 93: List<DbUser> refreshedUsers = new ArrayList<>(); Line 94: for (String authzName : authzNames) { Line 95: refreshedUsers.addAll(refreshAllUsers(EngineExtensionsManager.getInstance().getExtensionByName(authzName), you need to deal with users added by extension that is removed. the name of AllUsers is probably misleading as you refresh group as well? why don't you have two phases? one for users and one for groups? Line 96: usersPerAuthz.get(authzName), Line 97: groupsPerAuthz.get(authzName))); Line 98: } Line 99: Line 98: } Line 99: Line 100: for (DbUser dbUser : refreshedUsers) { Line 101: DbFacade.getInstance().getDbUserDao().update(dbUser); Line 102: } what about updating the groups? Line 103: } Line 104: Line 105: private Collection<DbUser> refreshAllUsers(ExtensionProxy authz, List<DbUser> dbUsers, List<DbGroup> dbGroups) { Line 106: Line 140: while (!idsToFetch.isEmpty()) { Line 141: groupsToFetch = new HashSet<>(); Line 142: for (String namespace : idsToFetch.keySet()) { Line 143: List<DirectoryGroup> groups = Line 144: AuthzUtils.findGroupsByIds(authz, namespace, new ArrayList<>(idsToFetch.get(namespace)), true); why do you need to cast it to ArrayList? Line 145: idsToFetch = new HashMap<>(); Line 146: for (DirectoryGroup group : groups) { Line 147: cache.put(group.getId(), group); Line 148: for (DirectoryGroup memberOf: group.getGroups()) { Line 141: groupsToFetch = new HashSet<>(); Line 142: for (String namespace : idsToFetch.keySet()) { Line 143: List<DirectoryGroup> groups = Line 144: AuthzUtils.findGroupsByIds(authz, namespace, new ArrayList<>(idsToFetch.get(namespace)), true); Line 145: idsToFetch = new HashMap<>(); idsToFetch.removeAll(idsToFetch.get(namespace)) ? Line 146: for (DirectoryGroup group : groups) { Line 147: cache.put(group.getId(), group); Line 148: for (DirectoryGroup memberOf: group.getGroups()) { Line 149: groupsToFetch.add(memberOf); Line 145: idsToFetch = new HashMap<>(); Line 146: for (DirectoryGroup group : groups) { Line 147: cache.put(group.getId(), group); Line 148: for (DirectoryGroup memberOf: group.getGroups()) { Line 149: groupsToFetch.add(memberOf); only if not in cache? Line 150: } Line 151: Line 152: } Line 153: Line 152: } Line 153: Line 154: Line 155: } Line 156: idsToFetch = getIdsFromGroups(groupsToFetch, cache); why do you need the double conversion? you can drop the groupsToFetch in favor of idsToFetch... no? also probably rename idsToFetch to something that contains groups. Line 157: } Line 158: List<DbUser> refreshed = new ArrayList<>(); Line 159: for (DbUser dbUser : dbUsers) { Line 160: DirectoryUser directoryUser = directoryUsersByIds.get(dbUser.getExternalId()); -- 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: 11 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
