Juan Hernandez has posted comments on this change. Change subject: restapi: Phase 5: Add graphics console representation ......................................................................
Patch Set 8: (16 comments) https://gerrit.ovirt.org/#/c/39057/8/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/VmGraphicsConsoleResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/VmGraphicsConsoleResource.java: Line 8: Line 9: public interface VmGraphicsConsoleResource { Line 10: Line 11: @Path("{action: (generate_descriptor)}") Line 12: public ActionResource getActionSubresource(@PathParam("action") String action); There shouldn't be a "generate_descriptor" action, just an additional content type supported by a different GET method. Line 13: Line 14: @GET Line 15: @Produces({ ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, Line 16: ApiMediaType.APPLICATION_X_YAML}) Line 17: public GraphicsConsole get(); Line 18: Line 19: @GET Line 20: @Produces({ApiMediaType.APPLICATION_X_VIRT_VIEWER}) Line 21: @Path("generate_descriptor") This @Path annotation shouldn't be here, as this should be another @GET method, selected by JAX-RS according to the content type requested by the caller. Line 22: public String generateDescriptor(); Line 18: Line 19: @GET Line 20: @Produces({ApiMediaType.APPLICATION_X_VIRT_VIEWER}) Line 21: @Path("generate_descriptor") Line 22: public String generateDescriptor(); String is a poor implementation for potentially large data. Consider returning a "Response" object, and populate it inside the method implementation with an array of bytes. https://gerrit.ovirt.org/#/c/39057/8/backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml File backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml: Line 468: action.ticket.value: xs:string Line 469: description: Create a ticket for connecting to the VM using the display protocol. Line 470: - name: /vms/{vm:id}/graphicsconsoles|rel=get Line 471: description: Get active graphics consoles of a VM. Line 472: request: Remove this empty request description, it is automatically added to the RSDL document. Line 473: body: Line 474: parameterType: null Line 475: signatures: [] Line 476: headers: {} Line 476: headers: {} Line 477: - name: /vms/{vm:id}/graphicsconsoles/{graphicsconsole:id}|rel=get Line 478: description: Get active graphics console of a VM. Line 479: request: Line 480: body: Remove the body description, if it is empty. Line 481: parameterType: null Line 482: signatures: [] Line 483: headers: {} Line 484: - name: /vms/{vm:id}/graphicsconsoles/{graphicsconsole:id}|rel=generate_descriptor Line 479: request: Line 480: body: Line 481: parameterType: null Line 482: signatures: [] Line 483: headers: {} The "Content-Type" header indicating the "application/x-virt-viewer" type should go here, as it is the same GET method that can return XML, JSON or virt-viewer configuration: headers: Content-Type: value: application/xml|json|x-virt-viewer required: false Line 484: - name: /vms/{vm:id}/graphicsconsoles/{graphicsconsole:id}|rel=generate_descriptor Line 485: description: Generates text filled filled with console data. This file can be used to initiate console session using remote-viewer application. Line 486: request: Line 487: body: Line 480: body: Line 481: parameterType: null Line 482: signatures: [] Line 483: headers: {} Line 484: - name: /vms/{vm:id}/graphicsconsoles/{graphicsconsole:id}|rel=generate_descriptor There shouldn't be an additional link, just an additional content-type in the description of the previous link. Line 485: description: Generates text filled filled with console data. This file can be used to initiate console session using remote-viewer application. Line 486: request: Line 487: body: Line 488: parameterType: null https://gerrit.ovirt.org/#/c/39057/8/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsoleResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsoleResource.java: Line 52: Line 53: Fault fault = new Fault(); Line 54: fault.setReason("Unsupported console ID"); Line 55: fault.setDetail("Supported console IDs are: " + StringUtils.join(supportedIds, ", ")); Line 56: throw new WebApplicationException(Response.status(Response.Status.BAD_REQUEST).entity(fault).build()); This should just return 404 HTTP error code (not found). Line 57: } Line 58: Line 59: @Override Line 60: public String generateDescriptor() { Line 56: throw new WebApplicationException(Response.status(Response.Status.BAD_REQUEST).entity(fault).build()); Line 57: } Line 58: Line 59: @Override Line 60: public String generateDescriptor() { Consider changing this so that it returns byte[] instead of String, it is more efficient and it represents better the fact that it is opaque data for the RESTAPI. Line 61: String consoleString = HexUtils.hex2string(consoleId); Line 62: Line 63: GraphicsConsole console = new GraphicsConsole(); Line 64: console.setProtocol(consoleString); Line 73: ConsoleOptions configuredOptions = runQuery(VdcQueryType.ConfigureConsoleOptions, Line 74: new ConfigureConsoleOptionsParams(consoleOptions, true)).getReturnValue(); Line 75: Line 76: return runQuery(VdcQueryType.GetConsoleDescriptorFile, new ConsoleOptionsParams(configuredOptions)) Line 77: .getReturnValue(); The backend should also return byte[], in my opinion. Line 78: } https://gerrit.ovirt.org/#/c/39057/8/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsolesResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsolesResource.java: Line 7: import org.ovirt.engine.api.resource.VmGraphicsConsolesResource; Line 8: import org.ovirt.engine.api.restapi.types.VmMapper; Line 9: import org.ovirt.engine.core.common.businessentities.GraphicsInfo; Line 10: import org.ovirt.engine.core.common.businessentities.GraphicsType; Line 11: import org.ovirt.engine.core.common.businessentities.VM; When there are conflicts with the names of the classes try to import the restapi.model class and use fully qualified names of the backend classes. Line 12: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 13: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 14: import org.ovirt.engine.core.compat.Guid; Line 15: Line 51: protected Response performRemove(String id) { Line 52: Fault fault = new Fault(); Line 53: fault.setReason("Unsupported Operation"); Line 54: fault.setDetail("Remove is not supported"); Line 55: throw new WebApplicationException(Response.status(Response.Status.BAD_REQUEST).entity(fault).build()); The "remove" method is not part of the interface, so the client will never be able to invoke it, so there is no need to prepare a nice response for the user. Just throw an exception: throw new UnsupportedOperationException(); Line 56: } Line 57: Line 58: @Override Line 59: protected GraphicsConsole doPopulate(GraphicsConsole model, VM entity) { https://gerrit.ovirt.org/#/c/39057/8/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/vv/VirtViewerMessageBodyWriter.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/vv/VirtViewerMessageBodyWriter.java: Line 15: import java.lang.reflect.Type; Line 16: Line 17: @Provider Line 18: @Produces(ApiMediaType.APPLICATION_X_VIRT_VIEWER) Line 19: public class VirtViewerMessageBodyWriter extends DefaultTextPlain { I think this isn't needed, as JAX-RS already has a message body writer for strings (and for byte[]). Line 20: Line 21: @Override Line 22: public void writeTo(Object o, Class type, Type genericType, Annotation[] annotations, MediaType mediaType, MultivaluedMap httpHeaders, OutputStream entityStream) throws IOException, WebApplicationException { Line 23: if (o instanceof Fault) { https://gerrit.ovirt.org/#/c/39057/8/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java: Line 565: Line 566: return params; Line 567: } Line 568: Line 569: @Mapping(from = org.ovirt.engine.core.common.businessentities.VM.class, to = GraphicsConsole.class) The "from" part should match type of the first parameter, thus it should be Map.Entry.class. That or change the first parameter to be the backend VM. Line 570: public static GraphicsConsole map(Map.Entry<GraphicsType, GraphicsInfo> graphicsInfo, GraphicsConsole template) { Line 571: GraphicsConsole model = template != null ? template : new GraphicsConsole(); Line 572: Line 573: String graphicsTypeString = map(graphicsInfo.getKey(), null); Line 582: Line 583: return model; Line 584: } Line 585: Line 586: @Mapping(from = org.ovirt.engine.api.model.GraphicsType.class, to = GraphicsType.class) When there are conflicts try to import the model class and use the fully qualified name of the backend class. Line 587: public static GraphicsType map(org.ovirt.engine.api.model.GraphicsType graphicsType, GraphicsType template) { Line 588: if (graphicsType != null) { Line 589: switch (graphicsType) { Line 590: case SPICE: Line 597: return null; Line 598: } Line 599: Line 600: @Mapping(from = GraphicsType.class, to = String.class) Line 601: public static String map(GraphicsType graphicsType, String template) { This mapper should return instances of the RESTAPI GraphicsType enum, not strings. Conversion to string should then use GraphicsType.toString(). Line 602: if (graphicsType != null) { Line 603: switch (graphicsType) { Line 604: case SPICE: Line 605: return SPICE_STRING; -- To view, visit https://gerrit.ovirt.org/39057 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7dbc63ac49bfba5637c38b84b373d78f348c7872 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
