Juan Hernandez has posted comments on this change. Change subject: core,restapi: added reported differences element to api.xsd + returning reported differences from GetNetworkAttachment queries ......................................................................
Patch Set 3: (8 comments) Is there any chance that we may want to add/update/delete/search reported configurations? If so then it should be implemented as a sub-collection, not as a inline collection. http://gerrit.ovirt.org/#/c/37407/3/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3871: <xs:element ref="host_nic" minOccurs="0" maxOccurs="1"/> Line 3872: <xs:element ref="ip_configuration" minOccurs="0" maxOccurs="1"/> Line 3873: <xs:element ref="properties" minOccurs="0" maxOccurs="1"/> Line 3874: <xs:element name="override_configuration" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 3875: <xs:element name="reported_configurations" type="ReportedConfigurations" minOccurs="0"/> Define a top level element for this, and use it: <xs:element ref="reported_configurations" .../> Line 3876: <xs:element ref="host" minOccurs="0" maxOccurs="1"/> Line 3877: </xs:sequence> Line 3878: </xs:extension> Line 3879: </xs:complexContent> Line 3896: </xs:extension> Line 3897: </xs:complexContent> Line 3898: </xs:complexType> Line 3899: Line 3900: <xs:complexType name="ReportedConfigurations"> Define a top level element for this complex type: <xs:element name="reported_configurations" type="ReportedConfigurations"/> This is required by the generators of the Python and Java SDKs, even if it is never used. Line 3901: <xs:sequence> Line 3902: <xs:element name="in_sync" type="xs:boolean"/> Line 3903: <xs:element name="reportedConfiguration" type="ReportedConfiguration" minOccurs="0" maxOccurs="unbounded"/> Line 3904: </xs:sequence> Line 3898: </xs:complexType> Line 3899: Line 3900: <xs:complexType name="ReportedConfigurations"> Line 3901: <xs:sequence> Line 3902: <xs:element name="in_sync" type="xs:boolean"/> Is this "in_sync" really a property of the collection, or rather a property of the network attachment entity? If it is a property of the entity please move it to the corresponding complex type. Note that we usually don't add properties to collections, and the generators of the Python and Java SDKs don't work well with them. Line 3903: <xs:element name="reportedConfiguration" type="ReportedConfiguration" minOccurs="0" maxOccurs="unbounded"/> Line 3904: </xs:sequence> Line 3905: </xs:complexType> Line 3906: Line 3899: Line 3900: <xs:complexType name="ReportedConfigurations"> Line 3901: <xs:sequence> Line 3902: <xs:element name="in_sync" type="xs:boolean"/> Line 3903: <xs:element name="reportedConfiguration" type="ReportedConfiguration" minOccurs="0" maxOccurs="unbounded"/> Define a top level element for this: <xs:element name="reported_configuration" type="ReportedConfiguration"/> And use it: <xs:element ref="reported_configuration" minOccurs="0" .../> In any case the name of the element should not use camel case, but underscores to separate words. Also add an annotation, so that the Java name will be plural. Line 3904: </xs:sequence> Line 3905: </xs:complexType> Line 3906: Line 3907: <xs:complexType name="ReportedConfiguration"> http://gerrit.ovirt.org/#/c/37407/3/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/NetworkAttachmentMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/NetworkAttachmentMapper.java: Line 104: Line 105: model.setOverrideConfiguration(entity.isOverrideConfiguration()); Line 106: Line 107: if (entity.getReportedConfigurations() != null) { Line 108: model.setReportedConfigurations(ReportedConfigurationsMapper.map(entity.getReportedConfigurations(), null)); The loop iterating the reported configurations should be here, as the mapper should map entities, not collections. Line 109: } Line 110: Line 111: IpConfiguration ipConfiguration = entity.getIpConfiguration(); Line 112: if (ipConfiguration != null) { http://gerrit.ovirt.org/#/c/37407/3/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ReportedConfigurationsMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ReportedConfigurationsMapper.java: Line 2: Line 3: import java.util.List; Line 4: Line 5: import org.ovirt.engine.api.model.ReportedConfiguration; Line 6: import org.ovirt.engine.core.common.businessentities.network.ReportedConfigurations; When the names of the backend entities and the names of the RESTAPI entities are the same we tend to import the RESTAPI entity and use the fully qualified name of the backend entity. Here you are doing the opposite. Not a big deal, just consider doing the other way around, for consistency with the rest of the code. Line 7: Line 8: public class ReportedConfigurationsMapper { Line 9: Line 10: @Mapping(from = ReportedConfigurations.class, to = org.ovirt.engine.api.model.ReportedConfigurations.class) Line 4: Line 5: import org.ovirt.engine.api.model.ReportedConfiguration; Line 6: import org.ovirt.engine.core.common.businessentities.network.ReportedConfigurations; Line 7: Line 8: public class ReportedConfigurationsMapper { We don't usually create mapper classes for collections, but for entities. So this should be named "ReportedConfigurationMapper", in singular, and the "map" method should translate from backend entity to RESTAPI entity, not collection. The iteration of the collection should be moved to the class that uses this "map" method, in this case "NetworkAttahmentMapper". Line 9: Line 10: @Mapping(from = ReportedConfigurations.class, to = org.ovirt.engine.api.model.ReportedConfigurations.class) Line 11: public static org.ovirt.engine.api.model.ReportedConfigurations map(ReportedConfigurations entity, Line 12: org.ovirt.engine.api.model.ReportedConfigurations template) { Line 19: for (ReportedConfigurations.ReportedConfiguration reportedConfiguration : entity.getReportedConfigurationList()) { Line 20: Line 21: ReportedConfiguration conf = new ReportedConfiguration(); Line 22: conf.setInSync(reportedConfiguration.isInSync()); Line 23: conf.setName(reportedConfiguration.getType().getName()); What if "reportedConfiguration.getType()" returns null? Line 24: conf.setValue(reportedConfiguration.getValue()); Line 25: reportedConfigurationList.add(conf); Line 26: } Line 27: -- To view, visit http://gerrit.ovirt.org/37407 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia7f83d268e250c24c4eeee6bff832426699ad95d Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
