Juan Hernandez has posted comments on this change. Change subject: core, restapi: reporting & removal of unmanaged networks. ......................................................................
Patch Set 4: (7 comments) http://gerrit.ovirt.org/#/c/37525/4/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3805: <xs:complexType name="UnmanagedNetwork"> Line 3806: <xs:complexContent> Line 3807: <xs:extension base="BaseResource"> Line 3808: <xs:sequence> Line 3809: <xs:element name="unmanaged_network_name" type="xs:string" minOccurs="1" maxOccurs="1"/> The base "BaseResource" already has a "name" attribute, can we use it instead of defining a new one? Line 3810: <xs:element name="nic_name" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3811: <xs:element name="nic_id" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3812: </xs:sequence> Line 3813: </xs:extension> Line 3807: <xs:extension base="BaseResource"> Line 3808: <xs:sequence> Line 3809: <xs:element name="unmanaged_network_name" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3810: <xs:element name="nic_name" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3811: <xs:element name="nic_id" type="xs:string" minOccurs="0" maxOccurs="1"/> Does it make sense to re-use the already existing "HostNIC" complex type? It already has "name" and "id" attributes. Line 3812: </xs:sequence> Line 3813: </xs:extension> Line 3814: Line 3815: </xs:complexContent> Line 3813: </xs:extension> Line 3814: Line 3815: </xs:complexContent> Line 3816: Line 3817: </xs:complexType> Fix then indentation: two spaces per level. Line 3818: Line 3819: <xs:complexType name="HostNics"> Line 3820: <xs:complexContent> Line 3821: <xs:extension base="BaseResources"> http://gerrit.ovirt.org/#/c/37525/4/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostNetworkAttachmentsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostNetworkAttachmentsResource.java: Line 11: import org.ovirt.engine.core.compat.Guid; Line 12: Line 13: public class BackendHostNetworkAttachmentsResource extends AbstractBackendNetworkAttachmentsResource { Line 14: Line 15: public BackendHostNetworkAttachmentsResource(Guid hostId) { Remove the extra space. Line 16: super(hostId); Line 17: } Line 18: Line 19: @SingleEntityResource http://gerrit.ovirt.org/#/c/37525/4/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendUnmanagedNetworkResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendUnmanagedNetworkResource.java: Line 29: UnmanagedNetworkBusinessEntity entity = getEntity(UnmanagedNetworkBusinessEntity.class, Line 30: VdcQueryType.GetUnmanagedNetworkByVdsIdAndName, Line 31: new UnmanagedNetworkParameters(hostId, id), Line 32: id); Line 33: return map(entity, null); If possible this method should just be: return performGet(VdcQueryType.GetUnmanagedNetworkByVdsIdAndName, ...); That takes care of checking null and returning 404. Line 34: } Line 35: Line 36: @Override Line 37: protected Guid asGuidOr404(String id) { http://gerrit.ovirt.org/#/c/37525/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/UnmanagedNetworkMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/UnmanagedNetworkMapper.java: Line 13: Line 14: UnmanagedNetworkBusinessEntity entity = template == null ? new UnmanagedNetworkBusinessEntity() : template; Line 15: Line 16: if (model.isSetId()) { Line 17: entity.setId(model.getUnmanagedNetworkName()); The backend identifier can't be directly copied to the RESTAPI identifier, as it could be a string containing characters that aren't valid in a URL. For example, if the backend identifier happens to be "my/network" then the resulting URL would be: http://.../ovirt-engine/api/hosts/{host:id}/unmanagednetwors/my/network Later, if the user requests the URL RESTEasy would split it using the slash, and the result will be a 404 error, as there isn't an unregistered network named "my". To avoid this issue the names provided by the backend need to be encoded/decoded if they are strings. Use the HexUtils.string2hex and HexUtils.hex2string methods. Line 18: } Line 19: Line 20: if (model.isSetUnmanagedNetworkName()) { Line 21: entity.setNetworkName(model.getUnmanagedNetworkName()); Line 39: } Line 40: Line 41: UnmanagedNetwork model = template == null ? new UnmanagedNetwork() : template; Line 42: Line 43: model.setId(entity.getId()); Same issue with identifiers. Need to encode/decode them. Line 44: model.setNicName(entity.getNicName()); Line 45: model.setNicId(entity.getNicId().toString()); Line 46: model.setUnmanagedNetworkName(entity.getNetworkName()); Line 47: -- To view, visit http://gerrit.ovirt.org/37525 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idcf6e1292ce6f3a335051123be8c3389e8230bf0 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
