Juan Hernandez has posted comments on this change. Change subject: core: 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 to things other than VMs (templates, for example) and in the future they may apply to other things as well. 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? 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 and 1. 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: <xs:element ref="icon" ...> <xs:annotation> ... </xs:annotation> </xs:element> 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. 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 below has been performed. 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. 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 the fully qualified name of the backend type. 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 the data from the RESTAPI model to the backend entity. It is the task of other components (RESTAPI validators, or preferably the backend) to implement business rules. So, if the user is only sending only the large icon id, for example, then the mapper should just copy that to the backend entity. 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 overwrite it. In general you should check if things have a value, and create them only if they don't: VmIcon iconModel = model.getLargeIcon(); if (iconModel == null) { iconModel = new VmIcon(); model.setIconModel(iconModel); } iconModel.setId(...); 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. 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, but if it has then there should be two if's here, one for each individual component. Remember that mappers (and the RESTAPI in general) shouldn't contain business logic. 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 <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
