Jakub Niedermertl has posted comments on this change. Change subject: restapi: Vm Icons - REST part ......................................................................
Patch Set 4: (12 comments) https://gerrit.ovirt.org/#/c/40655/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 3370: Line 3371: <xs:element name="icon" type="VmIcon" /> Line 3372: <xs:element name="icons" type="VmIcons" /> Line 3373: Line 3374: <xs:complexType name="VmIcon"> > Consider renaming the type to "Icon" (and "Icons") as they currently apply Done Line 3375: <xs:annotation> Line 3376: <xs:appinfo> Line 3377: <jaxb:class name="VmIcon"/> Line 3378: </xs:appinfo> Line 3375: <xs:annotation> Line 3376: <xs:appinfo> Line 3377: <jaxb:class name="VmIcon"/> Line 3378: </xs:appinfo> Line 3379: </xs:annotation> > I think this annotation isn't needed. Is it? Done Line 3380: <xs:complexContent> Line 3381: <xs:extension base="BaseResource"> Line 3382: <xs:sequence> Line 3383: <xs:element name="media_type" type="xs:string" /> Line 3380: <xs:complexContent> Line 3381: <xs:extension base="BaseResource"> Line 3382: <xs:sequence> Line 3383: <xs:element name="media_type" type="xs:string" /> Line 3384: <xs:element name="data" type="xs:string" /> > Explicitly indicate the values of minOccurs and maxOccurs here, should be 0 Done Line 3385: </xs:sequence> Line 3386: </xs:extension> Line 3387: </xs:complexContent> Line 3388: </xs:complexType> Line 3395: </xs:annotation> Line 3396: <xs:complexContent> Line 3397: <xs:extension base="BaseResources"> Line 3398: <xs:sequence> Line 3399: <xs:element ref="icon" minOccurs="0" maxOccurs="unbounded"/> > The jaxb:property annotation should be inside this element: Done Line 3400: </xs:sequence> Line 3401: </xs:extension> Line 3402: </xs:complexContent> Line 3403: </xs:complexType> Line 3399: <xs:element ref="icon" minOccurs="0" maxOccurs="unbounded"/> Line 3400: </xs:sequence> Line 3401: </xs:extension> Line 3402: </xs:complexContent> Line 3403: </xs:complexType> > Remember to fix indentation before merging: 2 spaces per level. Done Line 3404: Line 3405: <xs:element name="reported_devices" type="ReportedDevices"/> Line 3406: Line 3407: <xs:complexType name="ReportedDevices"> https://gerrit.ovirt.org/#/c/40655/4/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendOperatingSystemResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendOperatingSystemResource.java: Line 41: Integer key = Integer.valueOf(id); Line 42: final VmIconDefault vmIconDefault = getEntity(VmIconDefault.class, Line 43: VdcQueryType.GetVmIconDefault, Line 44: GetVmIconDefaultParameters.create(key), Line 45: "VmIconDefault"); > Consider moving the execution of this query after the "uniqueName" check be Done Line 46: String uniqueName = repository.getUniqueOsNames().get(key); Line 47: if (uniqueName == null) { Line 48: return notFound(); Line 49: } Line 50: model.setName(uniqueName); Line 51: String name = repository.getOsNames().get(key); Line 52: if (name != null) { Line 53: model.setDescription(name); Line 54: } > Maybe here. Done Line 55: if (vmIconDefault != null) { Line 56: model.setSmallIcon(VmIconHelper.createIcon(vmIconDefault.getSmallIconId())); Line 57: model.setLargeIcon(VmIconHelper.createIcon(vmIconDefault.getLargeIconId())); Line 58: } https://gerrit.ovirt.org/#/c/40655/4/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/util/VmIconHelper.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/util/VmIconHelper.java: Line 1: package org.ovirt.engine.api.restapi.util; Line 2: Line 3: import org.ovirt.engine.api.model.VmBase; Line 4: import org.ovirt.engine.core.common.action.HasVmIcon; Line 5: import org.ovirt.engine.core.common.businessentities.VmIcon; > When there are conflicts with names try to import the RESTAPI type and use Done Line 6: import org.ovirt.engine.core.compat.Guid; Line 7: Line 8: public class VmIconHelper { Line 9: https://gerrit.ovirt.org/#/c/40655/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmBaseMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmBaseMapper.java: Line 175: entity.setLargeIconId(GuidUtils.asGuid(model.getLargeIcon().getId())); Line 176: } Line 177: if (model.isSetSmallIcon() && model.getSmallIcon().isSetId()) { Line 178: entity.setSmallIconId(GuidUtils.asGuid(model.getSmallIcon().getId())); Line 179: } > The mapper shouldn't implement business logic rules, it should just copy th Done Line 180: } Line 181: Line 182: protected static void mapVmBaseEntityToModel(VmBase model, org.ovirt.engine.core.common.businessentities.VmBase entity) { Line 183: model.setId(entity.getId().toString()); Line 270: } Line 271: if (entity.getLargeIconId() != null) { Line 272: final VmIcon iconModel = new VmIcon(); Line 273: iconModel.setId(entity.getLargeIconId().toString()); Line 274: model.setLargeIcon(iconModel); > The large icon may be assigned before calling this method, and this will ov Done Line 275: } Line 276: if (entity.getSmallIconId() != null) { Line 277: final VmIcon iconModel = new VmIcon(); Line 278: iconModel.setId(entity.getSmallIconId().toString()); Line 275: } Line 276: if (entity.getSmallIconId() != null) { Line 277: final VmIcon iconModel = new VmIcon(); Line 278: iconModel.setId(entity.getSmallIconId().toString()); Line 279: model.setSmallIcon(iconModel); > Same. Done Line 280: } Line 281: } Line 282: Line 283: @Mapping(from = OriginType.class, to = String.class) https://gerrit.ovirt.org/#/c/40655/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmIconMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmIconMapper.java: Line 15: if (model.isSetId()) { Line 16: entity.setId(GuidUtils.asGuid(model.getId())); Line 17: } Line 18: if (model.isSetMediaType() && model.isSetData()) { Line 19: entity.setTypeAndData(model.getMediaType(), model.getData()); > Not sure of the backend entity has separate "setType" and "setData" methods Backend entity VmIcon has property 'dataUrl' that combines media type and data. Icon properties 'media_type' and 'data' has to be changed together because it doesn't make sense to change type and keep data and in general even vice versa. Line 20: } Line 21: return entity; Line 22: } Line 23: -- To view, visit https://gerrit.ovirt.org/40655 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9599565257e1e9d3e6293931fa35a7ad6f5a5f52 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
