Yair Zaslavsky has posted comments on this change. Change subject: aaa: Introducing format to id of User and Group API entities ......................................................................
Patch Set 4: (6 comments) http://gerrit.ovirt.org/#/c/26191/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GroupMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GroupMapper.java: Line 47: if (model.isSetId()) { Line 48: String id = model.getId(); Line 49: try { Line 50: String internalId = DirectoryEntryIdUtils.getInternalId(id); Line 51: String providerId = DirectoryEntryIdUtils.getProviderId(id); > there is no point in getting these both. There is no use case in which the external id should be fetched from the database. This mapping is used for either add or delete (there is no UpdateGroup command). for the case of add - you don't provide internal id, just provider id. In this case the entity will be set with a new internal id. for the case of delete - it should be performed by internal id. Line 52: if (StringUtils.isNotEmpty(internalId)) { Line 53: entity.setId(GuidUtils.asGuid(internalId)); Line 54: } else { Line 55: entity.setId(Guid.newGuid()); http://gerrit.ovirt.org/#/c/26191/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/UserMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/UserMapper.java: Line 74: if (model.isSetId()) { Line 75: String id = model.getId(); Line 76: try { Line 77: String internalId = DirectoryEntryIdUtils.getInternalId(id); Line 78: String providerId = DirectoryEntryIdUtils.getProviderId(id); > same here, fetch either, not both, as they cannot be both. see previous explanation. Line 79: if (StringUtils.isNotEmpty(internalId)) { Line 80: entity.setId(GuidUtils.asGuid(internalId)); Line 81: } else { Line 82: entity.setId(Guid.newGuid()); http://gerrit.ovirt.org/#/c/26191/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/utils/DirectoryEntryIdUtils.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/utils/DirectoryEntryIdUtils.java: Line 24: Matcher matcher = ID_PATTERN.matcher(decode(id)); Line 25: if (matcher.matches()) { Line 26: return matcher.group("id"); Line 27: } Line 28: throw new RuntimeException("Error in getting internal id from ID string " + id); > please use the exception pattern: Done Line 29: } Line 30: Line 31: /** Line 32: * Gets the provider id part from the id string Line 35: * the id string Line 36: * @return the provider id part Line 37: */ Line 38: public static String getProvider(String id) { Line 39: Matcher matcher = ID_PATTERN.matcher(decode(id)); > hmmm... you match 3 time for single string instead of once... not sure i would introduce a new class here, i can introduce a new helper function though. Line 40: if (matcher.matches()) { Line 41: return matcher.group("provider"); Line 42: } Line 43: throw new RuntimeException("Error in getting provider name from ID string " + id); Line 74: * name of directory Line 75: * @param idInDirectory Line 76: * id of entity in directory Line 77: */ Line 78: public static String generateId(Guid internalId) { > I think it is confusing to have same function name for two formats usages. i disagree. how critical is it for you? Line 79: return encode("id=" + internalId); Line 80: } Line 81: Line 82: private static String decode(String id) { Line 80: } Line 81: Line 82: private static String decode(String id) { Line 83: try { Line 84: return URLDecoder.decode(id, "UTF-8"); > please consider remove these wrapper in favour for: 1. Should be return URLDecoder.decode(id, Charset.forName("UTF-8").toString()); 2. What is the benefit in this? Line 85: } catch (UnsupportedEncodingException e) { Line 86: throw new RuntimeException(e); Line 87: } Line 88: } -- To view, visit http://gerrit.ovirt.org/26191 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaecb5d43945769db82475edde1c7075c1a343c07 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@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
