Alon Bar-Lev has posted comments on this change. Change subject: aaa: Fix sync ......................................................................
Patch Set 13: (2 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 46: authzExtension = EngineExtensionsManager.getInstance().getExtensionByName(authz); Line 47: } catch (ConfigurationException ex) { Line 48: log.warn(String.format("The extension %1$s could not be found. Users associated with this authz extension will not be refreshed.", Line 49: authz)); Line 50: continue; > the "logic" part you mentioned is quite long IMHO. the "logic" part is the same size, the usage pattern is different. using continue in a code is just like using goto, you use it only if you have limitation of language, and when it clear, for example: for (x : y) { if (!x.active()) { continue; } } this is per java did not think of that case of conditional during iteration which was always part of loops: for (x = y.begin(); x != null && x.active(); x = x.next()) { } but ok, do not waste time in this any more, not that important, just wanted to show why/when. Line 51: } Line 52: Line 53: boolean supportRecursiveGroupResolution = Line 54: (authzExtension.getContext().<Long> get(Authz.ContextKeys.CAPABILITIES, 0L) & Authz.Capabilities.RECURSIVE_GROUP_RESOLUTION) != 0L; Line 53: boolean supportRecursiveGroupResolution = Line 54: (authzExtension.getContext().<Long> get(Authz.ContextKeys.CAPABILITIES, 0L) & Authz.Capabilities.RECURSIVE_GROUP_RESOLUTION) != 0L; Line 55: Line 56: Map<String, Set<String>> groupIdsToFetch = Collections.emptyMap(); Line 57: Set<DirectoryGroup> groupsToFetch = new HashSet<>(); > these are not db entries, but the AAA module related entries, I still think I did not write not to use AuthzUtils... just use utils that do not perform conversion... return PrincipalRecord/GroupRecord.... you can then have a util that convert a list of these into whatever entities you like, these utils will not be used here. For example findGroupsByIds() should return List<ExtMap> and you can add modify populateGroups() to receive List<ExtMap>. This will also make the AuthzUtils more consistent, as now at low level it uses the DirectoryXXX which is IVdcQueryable and is part of your bll legacy and have no benefit in aaa processing. Think of this as if aaa module is not requiring anything of common, moving the translation routings into AuthzUtils that is at bll. What we do need as "generic utils"? 1. getName 2. fetchPrincipalRecord 3. findPrincipalsByQuery 4. findPrincipalsByIds 5. findGroupsByQuery These should use only aaa entities (PrincipalRecord, GroupRecord), without any mapping. Then all you require is mapping that takes the aaa entity (or collection of such) and convert it into db entities, this can be in either common or bll, has no real place in aaa. As a result the entire aaa module processing will be consistent, and detached from the other logic, with clear cut. Line 58: Map<String, Set<String>> usersToFetchPerNamespace = userIdsToFetch.get(authz); Line 59: for (String namespace : usersToFetchPerNamespace.keySet()) { Line 60: List<DirectoryUser> fetchedUsers = AuthzUtils.findPrincipalsByIds( Line 61: authzExtension, -- 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
