Alon Bar-Lev has posted comments on this change. Change subject: aaa: Introducing format to id of User and Group API entities ......................................................................
Patch Set 4: (4 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 use case in which the external id should be fetched from the da I got no answer why you parse both and not have: String internalId = DirectoryEntryIdUtils.getInternalId(id); if (internalId != null) { entity.setId(GuidUtils.asGuid(internalId)); } else { entity.setId(Guid.newGuid()); entity.setExternalId(DirectoryEntryIdUtils.getProviderId(id)); } maybe I miss something. But if entity is to be prepared in order to be stored in database, then it should not use DirectoryEntryIdUtils anyway, and should have to explicit fields: directory, id with directory and not single "external 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/utils/DirectoryEntryIdUtils.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/utils/DirectoryEntryIdUtils.java: 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)); > not sure i would introduce a new class here, i can introduce a new helper f anything that will avoid parsing 3 times same string. you can return struct (data class with public members). 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 disagree. how critical is it for you? no... but it did confused me. 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"); > 1. Should be hmmm........... bad java!!!! this function is not getting Charset as the other, so it is impossible, as it will do the conversion internally anyway. these inconsistencies within the java sdk are huge. 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
