Juan Hernandez has posted comments on this change. Change subject: aaa: Introducing changes to API (at domain groups/ids) ......................................................................
Patch Set 10: (19 comments) Most of the inline comments are about explicitly specifying the character encoding when converting strings to arrays of bytes and the other way around. I think it was good idea to put the encode/decode methods in a separate class, that would reduce the verbosity and the places where you have to specify the encoding. http://gerrit.ovirt.org/#/c/26191/10//COMMIT_MSG Commit Message: Line 11: Primary keys (internal ids) are generated by engine code. Line 12: The forma of the domain group/users ids is hexadecimal Line 13: represenation of the id in the domain. Line 14: At db users/db groups presentation this information is Line 15: presented as "db_domain_user_id" and "db_domain_group_id" In this patch set it is actually "domain_entry_id". Line 16: Line 17: Topic: AAA Line 18: Change-Id: Iaecb5d43945769db82475edde1c7075c1a343c07 http://gerrit.ovirt.org/#/c/26191/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbGroup.java: Line 12: Line 13: /** Line 14: * This is the identifier assigned by the engine to this group for internal use only. Line 15: */ Line 16: private Guid id = Guid.newGuid(); Are you sure you want to do this? It means that any DbGroup instance will have an id, so it will look like it is already in the database, even if it is used just as a transport object, and that can be very confusing. I would suggest to delay this till the user is actually inserted in the database. Line 17: Line 18: /** Line 19: * This is the identifier assigned by the external directory to this group. Line 20: */ http://gerrit.ovirt.org/#/c/26191/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbUser.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DbUser.java: Line 13: /** Line 14: * This is the identifier assigned by the engine to this user for internal Line 15: * use only. Line 16: */ Line 17: private Guid id = Guid.newGuid(); Not sure this is good idea, see the comment in DbGroup. Line 18: Line 19: /** Line 20: * This is the identifier assigned by the external directory to this user. Line 21: */ http://gerrit.ovirt.org/#/c/26191/10/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 1665: <xs:complexContent> Line 1666: <xs:extension base="BaseResource"> Line 1667: <xs:sequence> Line 1668: <xs:element ref="domain" minOccurs="0"/> Line 1669: <xs:element name="domain_entry_id" type="xs:string" minOccurs="0" maxOccurs="1"/> Shouldn't this be "domain_user_id"? Line 1670: <xs:element name="department" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 1671: <xs:element name="logged_in" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 1672: <xs:element name="last_name" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 1673: <!-- generally name@domain --> Line 1735: <xs:complexContent> Line 1736: <xs:extension base="BaseResource"> Line 1737: <xs:sequence> Line 1738: <xs:element ref="domain" minOccurs="0"/> Line 1739: <xs:element name="domain_entry_id" type="xs:string" minOccurs="0" maxOccurs="1"/> Shouldn't this be "domain_group_id"? Line 1740: <!-- used only to represent the initial role assignments for a Line 1741: new group, therafter modification of role assignments are Line 1742: only supported via the rel="roles" sub-collection --> Line 1743: <xs:element name="roles" type="Roles" minOccurs="0" maxOccurs="1"/> http://gerrit.ovirt.org/#/c/26191/10/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDomainGroupResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDomainGroupResource.java: Line 21: Line 22: private BackendDomainGroupsResource parent; Line 23: Line 24: public BackendDomainGroupResource(String id, BackendDomainGroupsResource parent) { Line 25: super(new String(DatatypeConverter.parseHexBinary(id)), Group.class, DirectoryGroup.class); The constructor of the string should recive the encoding as the second parameter, otherwise the result depends on the environment settings. Line 26: this.parent = parent; Line 27: } Line 28: Line 29: public BackendDomainGroupsResource getParent() { http://gerrit.ovirt.org/#/c/26191/10/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDomainUserResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDomainUserResource.java: Line 23: Line 24: private BackendDomainUsersResource parent; Line 25: Line 26: public BackendDomainUserResource(String id, BackendDomainUsersResource parent) { Line 27: super(new String(DatatypeConverter.parseHexBinary(id)), User.class, DirectoryUser.class); Second parameter to the string constructor should be the encoding. Line 28: this.parent = parent; Line 29: } Line 30: Line 31: public BackendDomainUsersResource getParent() { http://gerrit.ovirt.org/#/c/26191/10/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/AbstractBackendBaseTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/AbstractBackendBaseTest.java: Line 66: */ Line 67: protected static final String[] EXTERNAL_IDS = { Line 68: DatatypeConverter.printHexBinary(GUIDS[0].toByteArray()), Line 69: DatatypeConverter.printHexBinary(GUIDS[1].toByteArray()), Line 70: DatatypeConverter.printHexBinary(GUIDS[2].toByteArray()), In order to make the tests more realistic I would suggest to use external ids that aren't exactly equal to the internal ids. For example: EXTERNAL_IDS = { "0", "1", "2", "3" }; That way if something is assuming that they are equal we have more changes of tests catching it. Line 71: }; Line 72: Line 73: /** Line 74: * External identifier of a non existing user or group. http://gerrit.ovirt.org/#/c/26191/10/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendDomainGroupResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendDomainGroupResourceTest.java: Line 67: setUpGetEntityExpectations( Line 68: VdcQueryType.GetDirectoryGroupById, Line 69: DirectoryIdQueryParameters.class, Line 70: new String[] { "Domain", "Id" }, Line 71: new Object[] { DOMAIN, new String(DatatypeConverter.parseHexBinary(EXTERNAL_IDS[index])) Remember to specify the encoding. Line 72: }, Line 73: notFound? null: getEntity(index) Line 74: ); Line 75: } Line 75: } Line 76: Line 77: @Override Line 78: protected DirectoryGroup getEntity(int index) { Line 79: return new DirectoryGroup(DOMAIN, new String(DatatypeConverter.parseHexBinary(EXTERNAL_IDS[index])), NAMES[index]); Same. Line 80: } Line 81: } http://gerrit.ovirt.org/#/c/26191/10/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendDomainUserResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendDomainUserResourceTest.java: Line 67: setUpGetEntityExpectations( Line 68: VdcQueryType.GetDirectoryUserById, Line 69: DirectoryIdQueryParameters.class, Line 70: new String[] { "Domain", "Id" }, Line 71: new Object[] { DOMAIN, new String(DatatypeConverter.parseHexBinary(EXTERNAL_IDS[index])) }, The encoding. Line 72: notFound? null: getEntity(index) Line 73: ); Line 74: } Line 75: Line 76: @Override Line 77: protected DirectoryUser getEntity(int index) { Line 78: return new DirectoryUser(DOMAIN, Line 79: new String(DatatypeConverter.parseHexBinary(EXTERNAL_IDS[index])), Line 80: NAMES[index]); Same. Line 81: } Line 82: } http://gerrit.ovirt.org/#/c/26191/10/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 22: Domain dom = new Domain(); Line 23: dom.setId(new Guid(entity.getDomain().getBytes(), true).toString()); Line 24: model.setDomain(dom); Line 25: } Line 26: model.setDomainEntryId(DatatypeConverter.printHexBinary(entity.getExternalId().getBytes())); As the external id is now a string you shouldn't use the getBytes method without specifying the encoding. Line 27: return model; Line 28: } Line 29: Line 30: @Mapping(from = DirectoryGroup.class, to = Group.class) Line 35: Domain dom = new Domain(); Line 36: dom.setId(new Guid(entity.getDirectoryName().getBytes(), true).toString()); Line 37: model.setDomain(dom); Line 38: } Line 39: model.setId(DatatypeConverter.printHexBinary(entity.getId().getBytes())); Same. Line 40: return model; Line 41: } Line 42: Line 43: @Mapping(from = Group.class, to = DbGroup.class) Line 52: entity.setId(GuidUtils.asGuid(id)); Line 53: } Line 54: catch (MalformedIdException exception) { Line 55: // The identifier won't be a UUID if the group comes from /domains/{domain:id}/groups. Line 56: } This try-catch is no longer necessary. See the comment in UserMapper. Line 57: } Line 58: if (model.isSetDomain()) { Line 59: Domain domain = model.getDomain(); Line 60: if (domain.isSetName()) { Line 61: entity.setDomain(domain.getName()); Line 62: } Line 63: } Line 64: if (model.isSetDomainEntryId()) { Line 65: entity.setExternalId(new String(DatatypeConverter.parseHexBinary(model.getDomainEntryId()))); Need to pass encoding as second parameter of the constructor of string. Line 66: } Line 67: return entity; Line 68: } Line 69: http://gerrit.ovirt.org/#/c/26191/10/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 24: model.setId(entity.getId().toString()); Line 25: model.setLastName(entity.getLastName()); Line 26: model.setEmail(entity.getEmail()); Line 27: model.setDepartment(entity.getDepartment()); Line 28: model.setDomainEntryId(DatatypeConverter.printHexBinary(entity.getExternalId().getBytes())); The getBytes method needs the encoding parameter. Line 29: if (entity.getGroupNames() != null && entity.getGroupNames().trim().length() > 0) { Line 30: model.setGroups(new Groups()); Line 31: for (String name : entity.getGroupNames().split(",")) { Line 32: Group group = new Group(); Line 46: public static User map(DirectoryUser entity, User template) { Line 47: User model = template != null ? template : new User(); Line 48: model.setName(entity.getFirstName()); Line 49: model.setUserName(entity.getName() + "@" + entity.getDirectoryName()); Line 50: model.setId(DatatypeConverter.printHexBinary(entity.getId().getBytes())); Same. Line 51: model.setLastName(entity.getLastName()); Line 52: model.setEmail(entity.getEmail()); Line 53: model.setDepartment(entity.getDepartment()); Line 54: if (entity.getGroups() != null) { Line 79: entity.setId(GuidUtils.asGuid(id)); Line 80: } Line 81: catch (MalformedIdException exception) { Line 82: // The identifier won't be a UUID if the user comes from /domains/{domain:id}/users. Line 83: } This try-catch is no longer necessary, as we don't longer accept an identifier from /domains/{domain:id}/users to add permissions, so anything that isn't a correct UUID should just propagate the exception. Line 84: } Line 85: if (model.isSetDomain()) { Line 86: Domain domain = model.getDomain(); Line 87: if (domain.isSetName()) { -- 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: 10 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
