Alon Bar-Lev has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 5: (8 comments) http://gerrit.ovirt.org/#/c/29706/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SyncUsers.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/SyncUsers.java: Line 35: authzToNamespaceToUserIds.put(dbUser.getDomain(), new HashMap<String, Set<String>>()); Line 36: } Line 37: MultiValueMapUtils.addToMapOfSets(dbUser.getNamespace(), dbUser.getExternalId(), authzToNamespaceToUserIds.get(dbUser.getDomain())); Line 38: dbUsersMap.put(new DirectoryEntryKey(dbUser.getDomain(), dbUser.getExternalId()), dbUser); Line 39: } please split into another private function to ease reading: public static void sync(List<DbUser> dbUsers) { } private static List<DbUser> sync(String authz, List<String> ids) { } Line 40: Line 41: for (String authz : authzToNamespaceToUserIds.keySet()) { Line 42: try { Line 43: ExtensionProxy authzExtension = EngineExtensionsManager.getInstance().getExtensionByName(authz); Line 43: ExtensionProxy authzExtension = EngineExtensionsManager.getInstance().getExtensionByName(authz); Line 44: for (Entry<String, Set<String>> userIdsPerNamespace : authzToNamespaceToUserIds.get(authz).entrySet()) { Line 45: Set<String> userIds = userIdsPerNamespace.getValue(); Line 46: for (DirectoryUser directoryUser : DirectoryUtils.fetchUsersByIdsRecursively(authzExtension, userIdsPerNamespace.getKey(), new ArrayList<String>(userIds))) { Line 47: DirectoryUtils.flatGroups(directoryUser); why do you flat groups here? flat groups should be the last thing you do, and if you do this here, why do you do flatgroups at the other sources? Line 48: DbUser dbUser = new DbUser(directoryUser); Line 49: dbUser.setGroupIds(DirectoryUtils.getGroupIdsFromUser(directoryUser)); Line 50: activeUsers.put(new DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), dbUser); Line 51: } Line 44: for (Entry<String, Set<String>> userIdsPerNamespace : authzToNamespaceToUserIds.get(authz).entrySet()) { Line 45: Set<String> userIds = userIdsPerNamespace.getValue(); Line 46: for (DirectoryUser directoryUser : DirectoryUtils.fetchUsersByIdsRecursively(authzExtension, userIdsPerNamespace.getKey(), new ArrayList<String>(userIds))) { Line 47: DirectoryUtils.flatGroups(directoryUser); Line 48: DbUser dbUser = new DbUser(directoryUser); why do you convert to dbUser here? can't we gather the DirectoryUser and perform the conversion at end? so I suggest 3 functions: public static void sync(List<DbUser> dbUsers) { } public static List<DirectoryUser> sync(List<DirectoryUser> directoryUsers) { } private static List<DbUser> sync(String authz, List<String> ids) { } Line 49: dbUser.setGroupIds(DirectoryUtils.getGroupIdsFromUser(directoryUser)); Line 50: activeUsers.put(new DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), dbUser); Line 51: } Line 52: } Line 45: Set<String> userIds = userIdsPerNamespace.getValue(); Line 46: for (DirectoryUser directoryUser : DirectoryUtils.fetchUsersByIdsRecursively(authzExtension, userIdsPerNamespace.getKey(), new ArrayList<String>(userIds))) { Line 47: DirectoryUtils.flatGroups(directoryUser); Line 48: DbUser dbUser = new DbUser(directoryUser); Line 49: dbUser.setGroupIds(DirectoryUtils.getGroupIdsFromUser(directoryUser)); this should be in own function that map between DirectoryUser and DbUser Line 50: activeUsers.put(new DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), dbUser); Line 51: } Line 52: } Line 53: } catch (ConfigurationException ex) { Line 46: for (DirectoryUser directoryUser : DirectoryUtils.fetchUsersByIdsRecursively(authzExtension, userIdsPerNamespace.getKey(), new ArrayList<String>(userIds))) { Line 47: DirectoryUtils.flatGroups(directoryUser); Line 48: DbUser dbUser = new DbUser(directoryUser); Line 49: dbUser.setGroupIds(DirectoryUtils.getGroupIdsFromUser(directoryUser)); Line 50: activeUsers.put(new DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), dbUser); why isn't this a set? inactive users = set(dbUsers).removeAll(activeUsers) Line 51: } Line 52: } Line 53: } catch (ConfigurationException ex) { Line 54: log.error(String.format("The authz extension for %1%s has not been found. Users belonging to this extension will be marked as inactive", Line 50: activeUsers.put(new DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), dbUser); Line 51: } Line 52: } Line 53: } catch (ConfigurationException ex) { Line 54: log.error(String.format("The authz extension for %1%s has not been found. Users belonging to this extension will be marked as inactive", please print the ex.getMessage() as well, as the error can be different than not found. Line 55: authz)); Line 56: } Line 57: } Line 58: Line 55: authz)); Line 56: } Line 57: } Line 58: Line 59: for (DbUser dbUser: dbUsers) { please split this into two: Set<DbUser> inactiveUsers = new HashSet<>(); inactiveUsers.addAll(dbUsers); inactiveUsers.removeAll(activeUsers); for (DbUser dbUser : inactiveUsers) { dbUser.setActive(false); DbFacade.getInstance().getDbUserDao().update(dbUser); } for (DbUser dbUser : activeUsers) { DbUser original = dbUsers.get(dbUser); if (!original.equals(dbUser)) { DbFacade.getInstance().getDbUserDao().update(dbUser); } } what do I miss? Line 60: DbUser activeUser = activeUsers.get(new DirectoryEntryKey(dbUser)); Line 61: DbUser updatedUser = activeUser; Line 62: boolean updated = false; Line 63: if (activeUser == null) { Line 74: updatedUser.setId(dbUser.getId()); Line 75: if (!dbUser.isActive()) { Line 76: updated = true; Line 77: dbUser.setActive(true); Line 78: log.info(String.format("The inactive user %1$s from authz extension %2$s was found, and will be marked as active", how can it be? we do not search for inactive at all Line 79: dbUser.getLoginName(), Line 80: dbUser.getDomain())); Line 81: } Line 82: if (!dbUser.equals(activeUser)) { -- To view, visit http://gerrit.ovirt.org/29706 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3a66a7c235f16ff26cac3a779de688c73d67f4a2 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@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
