Yair Zaslavsky 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 ^^^^^^? Not sure if we can fix that without hurting backward compatibility - historically, this is the format of group names. 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 Group name format is historically different than user name format, and this is why the code is based on that format. If you change this, you might break legacy provide behavior. 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 f I disagree. IMHO, the is always in format of Hex.encode(string). There is no guessing magic here but a deterministic code that always performs decoding, under the assumption the provided format is the above. See the mapping between principals and groups to users and groups in UserMapper and GroupMapper. For example - UserMapper line 49 (more or less). 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 Done 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? Done 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? Done 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 f See my referral to UserMapper. The user is added based on a directory user . the format of the "id" (and both "domain entry id" ) in case of directory user is Hex.encode(string). 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
