Yair Zaslavsky has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 5: (6 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: not just authz, i have to provide namespace as well. 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, a flat groups should be done before setting the group ids. 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 pe what's the 2nd function for? Line 49: dbUser.setGroupIds(DirectoryUtils.getGroupIdsFromUser(directoryUser)); Line 50: activeUsers.put(new DirectoryEntryKey(directoryUser.getDirectoryName(), directoryUser.getId()), dbUser); Line 51: } Line 52: } 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 tha Done Line 55: authz)); Line 56: } Line 57: } Line 58: 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 what if extension was removed and re-added ? (for example, the jboss module was removed and then re-placed). when the extension was removed - all the relevant users will become inactive. when returned - all relevant users will become active, no? Line 79: dbUser.getLoginName(), Line 80: dbUser.getDomain())); Line 81: } Line 82: if (!dbUser.equals(activeUser)) { http://gerrit.ovirt.org/#/c/29706/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbUser.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbUser.java: Line 250: Line 251: @Override Line 252: public int hashCode() { Line 253: final int prime = 31; Line 254: int result = 1; This proved to be wrong due to the following reasons: a. in the code I'm not using any HashSet or HashMap that has DbUser as a key. b. i should compare all fields when checking equality of db user at sync. Line 255: result = prime * result + ((externalId == null) ? 0 : externalId.hashCode()); Line 256: result = prime * result + ((domain == null) ? 0 : domain.hashCode()); Line 257: return result; Line 258: } -- 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: 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
