Juan Hernandez has posted comments on this change. Change subject: restApi : RestApi to export volume profile to pdf ......................................................................
Patch Set 24: (16 comments) http://gerrit.ovirt.org/#/c/28340/24/backend/manager/dependencies/pom.xml File backend/manager/dependencies/pom.xml: Line 265: <groupId>commons-io</groupId> Line 266: <artifactId>commons-io</artifactId> Line 267: <version>${commons-io.version}</version> Line 268: </dependency> Line 269: Remember to fix this whitespace problem before merging. Line 270: <dependency> Line 271: <groupId>org.apache.xmlgraphics</groupId> Line 272: <artifactId>batik-util</artifactId> Line 273: <version>${batik.version}</version> http://gerrit.ovirt.org/#/c/28340/24/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/pdf/FOPMessageBodyWriter.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/pdf/FOPMessageBodyWriter.java: Line 58: return true; Line 59: } Line 60: Line 61: @Override Line 62: public void writeTo(final Object data, Class<?> dataClass, Type arg2, Annotation[] annotation, MediaType mediaType, MultivaluedMap<String, Object> map, OutputStream outputStream) throws IOException, WebApplicationException { Name the parameters as in the definition of the interface: writeTo(Object data, Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType, MultivaluedMap<String, Object> httpHeaders, OutputStream entityStream) Line 63: try { Line 64: new FOPMessageBodyWriter(); Line 65: Line 66: Source source = new JAXBSource(jaxbContext, data); Line 60: Line 61: @Override Line 62: public void writeTo(final Object data, Class<?> dataClass, Type arg2, Annotation[] annotation, MediaType mediaType, MultivaluedMap<String, Object> map, OutputStream outputStream) throws IOException, WebApplicationException { Line 63: try { Line 64: new FOPMessageBodyWriter(); Don't need to create an instance here. Line 65: Line 66: Source source = new JAXBSource(jaxbContext, data); Line 67: Line 68: String xslName = "/" + dataClass.getSimpleName() + "AsPdf.xsl"; Line 70: StreamSource transformSource = new StreamSource(templateStream); Line 71: Line 72: FopFactory fopFactory = FopFactory.newInstance(); Line 73: FOUserAgent foUserAgent = fopFactory.newFOUserAgent(); Line 74: TransformerFactory transfact = TransformerFactory.newInstance(); Is it possible to create these objects in the constructor and reuse them? Can they be used from multiple threads simultaneously? Line 75: Line 76: final ByteArrayOutputStream pdfoutStream = new ByteArrayOutputStream(); Line 77: Transformer xslfoTransformer = transfact.newTransformer(transformSource); Line 78: Fop fop = fopFactory.newFop(MimeConstants.MIME_PDF, foUserAgent, pdfoutStream); Line 74: TransformerFactory transfact = TransformerFactory.newInstance(); Line 75: Line 76: final ByteArrayOutputStream pdfoutStream = new ByteArrayOutputStream(); Line 77: Transformer xslfoTransformer = transfact.newTransformer(transformSource); Line 78: Fop fop = fopFactory.newFop(MimeConstants.MIME_PDF, foUserAgent, pdfoutStream); No need for the pdfoutStream, you can generate the PDF document directly to the outputStream passed as parameter to the method: Fop fop = fopFactory.newFop(MimeConstants.MIME_PDF, foUserAgent, outputStream); Line 79: Result res = new SAXResult(fop.getDefaultHandler()); Line 80: xslfoTransformer.transform(source, res); Line 81: Line 82: StreamingOutput stream = new StreamingOutput() { Line 82: StreamingOutput stream = new StreamingOutput() { Line 83: public void write(OutputStream output) throws IOException, WebApplicationException { Line 84: output.write(pdfoutStream.toByteArray()); Line 85: } Line 86: }; This stream isn't necessary. Line 87: Response.ok(stream).type("application/pdf").header("content-disposition", "attachment; filename=\"stats.pdf\"").build(); Line 88: } catch(Exception e) { Line 89: log.error(e.toString()); Line 90: } Line 83: public void write(OutputStream output) throws IOException, WebApplicationException { Line 84: output.write(pdfoutStream.toByteArray()); Line 85: } Line 86: }; Line 87: Response.ok(stream).type("application/pdf").header("content-disposition", "attachment; filename=\"stats.pdf\"").build(); This is completely ignored, as you aren't doing anything with the created Response object. To set the headers you need to use the "map" parameter (should be named httpHeaders): httpHeaders.putSingle("Content-Disposition", "attachment; filename=\"stats.pdf\""); And this should be done at the beginning of the method, before starting to send the response content, as you can't set headers after starting to send content. You don't need to set the content-type explicitly, the JAX-RS framework takes care of that. Line 88: } catch(Exception e) { Line 89: log.error(e.toString()); Line 90: } Line 91: } http://gerrit.ovirt.org/#/c/28340/24/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/gluster/GlusterVolumeResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/gluster/GlusterVolumeResource.java: Line 100: ApiMediaType.APPLICATION_XML, Line 101: ApiMediaType.APPLICATION_JSON, Line 102: ApiMediaType.APPLICATION_X_YAML, Line 103: ApiMediaType.APPLICATION_PDF Line 104: }) Fix the indentation of the }) , should start in the same column that @Produces. Line 105: public GlusterVolumeProfileDetails getProfileStatistics(); Line 106: Line 107: /** Line 108: * Sub-resource locator method, returns GlusterBricksResource on which the remainder of the URI is dispatched. http://gerrit.ovirt.org/#/c/28340/24/ovirt-engine.spec.in File ovirt-engine.spec.in: Line 192: Requires: %{name}-dbscripts = %{version}-%{release} Line 193: Requires: %{name}-restapi = %{version}-%{release} Line 194: Requires: %{name}-tools = %{version}-%{release} Line 195: Requires: %{name}-userportal = %{version}-%{release} Line 196: Requires: %{name}-webadmin-portal = %{version}-%{release} The "Requires: fop" should go here, as it is available in all the distributions that we support, not just Fedora. Line 197: Requires: %{name}-websocket-proxy >= %{version}-%{release} Line 198: Requires: java Line 199: Requires: java-1.7.0-openjdk >= 1:1.7.0.9-2.3.3.2 Line 200: Requires: jpackage-utils Line 223: Requires: postgresql-jdbc Line 224: Requires: quartz Line 225: Requires: snmp4j Line 226: Requires: spring-ldap Line 227: Requires: fop Use a tab to indent, no spaces, to be consistent with the rest of the "Requires". Line 228: Requires: springframework-aop Line 229: Requires: springframework-beans Line 230: Requires: springframework-context Line 231: Requires: springframework-expression Line 689: dst="%{engine_jboss_modules}/${dst}" Line 690: src="%{_javadir}/${src}" Line 691: rm -f "%{buildroot}${dst}" Line 692: ln -s "${src}" "%{buildroot}${dst}" Line 693: done << __EOF__ The line for fop should go here, as it is available in all the distributions that we support. And we should do the same for avalon, batik, xmlgraphics-commons and commons-io: org/apache/avalon/framework/main/avalon-framework-impl.jar org/apache/avalon/framework/main/avalon-framework-api.jar org/apache/commons/io/main/commons-io.jar org/apache/xmlgraphics/batik/main/batik-awt-util.jar batik/batik-awt-util.jar org/apache/xmlgraphics/batik/main/batik-bridge.jar batik/batik-bridge.jar org/apache/xmlgraphics/batik/main/batik-css.jar batik/batik-css.jar org/apache/xmlgraphics/batik/main/batik-extension.jar batik/batik-extension.jar org/apache/xmlgraphics/batik/main/batik-ext.jar batik/batik-ext.jar org/apache/xmlgraphics/batik/main/batik-gvt.jar batik/batik-gvt.jar org/apache/xmlgraphics/batik/main/batik-svg-dom.jar batik/batik-svg-dom.jar org/apache/xmlgraphics/batik/main/batik-transcoder.jar batik/batik-transcoder.jar org/apache/xmlgraphics/batik/main/batik-util.jar batik/batik-util.jar org/apache/xmlgraphics/commons/main/xmlgraphics-commons.jar org/apache/xmlgraphics/fop/main/fop.jar Not sure if this is completely correct. Please build the RPMs and check that there aren't broken symlinks. Also this needs to be checked in Fedora and CentOS. Line 694: org/apache/ws/commons/main/ws-commons-util.jar Line 695: org/ovirt/otopi/main/otopi.jar otopi/otopi.jar Line 696: org/ovirt/ovirt-host-deploy/main/ovirt-host-deploy.jar ovirt-host-deploy/ovirt-host-deploy.jar Line 697: org/ovirt/vdsm-jsonrpc-java/main/vdsm-jsonrpc-java-client.jar vdsm-jsonrpc-java/vdsm-jsonrpc-java-client.jar Line 705: com/woorea/openstack/sdk/main/openstack-client.jar openstack-java-sdk/openstack-client.jar Line 706: com/woorea/openstack/sdk/main/quantum-client.jar openstack-java-sdk/quantum-client.jar Line 707: com/woorea/openstack/sdk/main/quantum-model.jar openstack-java-sdk/quantum-model.jar Line 708: com/woorea/openstack/sdk/main/resteasy-connector.jar openstack-java-sdk/resteasy-connector.jar Line 709: org/apache/fop/main/fop.jar This is not specific to Fedora, should go in the previous section. Line 710: org/aopalliance/main/aopalliance.jar Line 711: org/apache/commons/compress/main/commons-compress.jar Line 712: org/apache/commons/configuration/main/commons-configuration.jar commons-configuration.jar Line 713: org/apache/commons/httpclient/main/commons-httpclient.jar http://gerrit.ovirt.org/#/c/28340/24/pom.xml File pom.xml: Line 95: <fop.version>1.0</fop.version> Line 96: <xmlgraphics-commons.version>1.4</xmlgraphics-commons.version> Line 97: <batik.version>1.7</batik.version> Line 98: <apache-avalon-api.version>4.3.1</apache-avalon-api.version> Line 99: <apache-avalon-impl.version>4.3.1</apache-avalon-impl.version> Use only one property for avalon: <avalon.version>4.3.1</avalon.version> Line 100: <apache-batick-all.version>1.7</apache-batick-all.version> Line 101: <xalan.version>2.6.0</xalan.version> Line 102: <servlet-api.version>2.5</servlet-api.version> Line 103: <commons-io.version>1.3</commons-io.version> Line 96: <xmlgraphics-commons.version>1.4</xmlgraphics-commons.version> Line 97: <batik.version>1.7</batik.version> Line 98: <apache-avalon-api.version>4.3.1</apache-avalon-api.version> Line 99: <apache-avalon-impl.version>4.3.1</apache-avalon-impl.version> Line 100: <apache-batick-all.version>1.7</apache-batick-all.version> I think this isn't needed, you already "have bakit.version". Line 101: <xalan.version>2.6.0</xalan.version> Line 102: <servlet-api.version>2.5</servlet-api.version> Line 103: <commons-io.version>1.3</commons-io.version> Line 104: <c3p0.version>0.9.1.1</c3p0.version> Line 97: <batik.version>1.7</batik.version> Line 98: <apache-avalon-api.version>4.3.1</apache-avalon-api.version> Line 99: <apache-avalon-impl.version>4.3.1</apache-avalon-impl.version> Line 100: <apache-batick-all.version>1.7</apache-batick-all.version> Line 101: <xalan.version>2.6.0</xalan.version> This isn't needed. Line 102: <servlet-api.version>2.5</servlet-api.version> Line 103: <commons-io.version>1.3</commons-io.version> Line 104: <c3p0.version>0.9.1.1</c3p0.version> Line 105: <aopalliance.version>1.0</aopalliance.version> Line 98: <apache-avalon-api.version>4.3.1</apache-avalon-api.version> Line 99: <apache-avalon-impl.version>4.3.1</apache-avalon-impl.version> Line 100: <apache-batick-all.version>1.7</apache-batick-all.version> Line 101: <xalan.version>2.6.0</xalan.version> Line 102: <servlet-api.version>2.5</servlet-api.version> This isn't needed. Line 103: <commons-io.version>1.3</commons-io.version> Line 104: <c3p0.version>0.9.1.1</c3p0.version> Line 105: <aopalliance.version>1.0</aopalliance.version> Line 106: <snmp4j.version>2.2.2</snmp4j.version> -- To view, visit http://gerrit.ovirt.org/28340 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie3ff28f5cf18bd7a2bcb53a169873fe6ae3541ed Gerrit-PatchSet: 24 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
