Alon Bar-Lev has posted comments on this change. Change subject: aaa: Fix add users ......................................................................
Patch Set 6: (7 comments) http://gerrit.ovirt.org/#/c/31499/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/aaa/BackendGroupsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/aaa/BackendGroupsResource.java: Line 195 Line 196 Line 197 Line 198 Line 199 what is that ^^^^^^? I would suggest it some kind of legacy, however if it does I expect the same at user... anyway, having lastIndex here is wrong.... if format is AUTHZ\user either fix and sync user or remove. Line 215 Line 216 Line 217 Line 218 Line 219 same here... cannot be that group and user do not behave the same, nor that one use indexof and the other lastindexof Line 197: return null; Line 198: } Line 199: Line 200: private DirectoryGroup getGroupById(String directoryName, String namespace, String groupId) { Line 201: groupId = DirectoryEntryIdUtils.decode(groupId); now that we have explicit id, can't we get rid of the guess magic between formats? you know if id is the internal id or the authz id at this point. Line 202: return getEntity( Line 203: DirectoryGroup.class, Line 204: VdcQueryType.GetDirectoryGroupById, Line 205: new DirectoryIdQueryParameters(directoryName, namespace, groupId), http://gerrit.ovirt.org/#/c/31499/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/aaa/BackendUsersResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/aaa/BackendUsersResource.java: Line 194 Line 195 Line 196 Line 197 Line 198 lastindexof Line 91: } Line 92: throw new WebFaultException(null, "Domain: '" + user.getDomain().getId().toString() + "' does not exist.", Response.Status.BAD_REQUEST); Line 93: } Line 94: else if (user.isSetUserName() && user.getUserName().contains("@")) { Line 95: return user.getUserName().substring(user.getUserName().lastIndexOf("@") + 1); use isNameContainsDomain? or any single function to split authz/user all over? Line 96: } Line 97: return null; Line 98: } Line 99: Line 212: result = getEntity( Line 213: DirectoryUser.class, Line 214: SearchType.DirectoryUser, Line 215: getDirectoryUserSearchPattern( Line 216: user.getUserName().contains("@") ? user.getUserName().substring(0, user.getUserName().indexOf("@")) : user.getUserName(), last indexof? use isNameContainsDomain? Line 217: user.getNamespace(), Line 218: directoryName) Line 219: ); Line 220: } Line 224: } Line 225: Line 226: private DirectoryUser getUserById(String directoryName, String namespace, String userId) { Line 227: DirectoryUser result; Line 228: userId = DirectoryEntryIdUtils.decode(userId); now that we have explicit id, can't we get rid of the guess magic between formats? you know if id is the internal id or the authz id at this point. Line 229: result = getEntity( Line 230: DirectoryUser.class, Line 231: VdcQueryType.GetDirectoryUserById, Line 232: new DirectoryIdQueryParameters(directoryName, namespace, userId), -- To view, visit http://gerrit.ovirt.org/31499 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie0384bab5abb6b8e1b1c9d1582bc630ea012cd4f Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
