Tomas Jelinek 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 conte Done 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 met Done 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 return Done 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 RSD Done 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. Done 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 s Done 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 t Done 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). Done 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 m Done 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. The frontend-backend is always based on String communication and not binary so for consistency reasons I would leave it like this. The .vv file will anyway not be big. 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 re Done 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 Done 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 It was needed when I was throwing WebApplicationException with content and it was needed to serialize it's content. But since now Im throwing them without content it is not needed - removed. 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 Done 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 qu Done 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 s Done 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
