Juan Hernandez has posted comments on this change. Change subject: restapi: [WIP!] Phase 5: REST part for console descriptor generator ......................................................................
Patch Set 1: (8 comments) http://gerrit.ovirt.org/#/c/37976/1/backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/ConsoleOptionsHelper.java File backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/ConsoleOptionsHelper.java: Line 19: MultivaluedMap<String, String> consoleOptions = lastSegment.getMatrixParameters(); Line 20: for (Map.Entry<String, List<String>> entry : consoleOptions.entrySet()) { Line 21: updateOptions(options, new Pair<>(entry.getKey(), entry.getValue().get(0))); Line 22: } Line 23: } catch (Exception e) { } Remember to at least log this exception. Line 24: Line 25: return options; Line 26: } Line 27: Line 28: private static void updateOptions(ConsoleOptions options, Pair<String, String> keyVal) { Line 29: String val = keyVal.getSecond(); Line 30: switch (keyVal.getFirst()) { Line 31: case "fullscreen": Line 32: options.setFullScreen("1".equals(val)); I think the values of this should be "true" and "false". Line 33: break; Line 34: case "release-cursor": Line 35: options.setReleaseCursorHotKey(val); Line 36: break; Line 33: break; Line 34: case "release-cursor": Line 35: options.setReleaseCursorHotKey(val); Line 36: break; Line 37: // enough for the demo lol; todo switch (~16 cases) for filling actual console options If possible the RESTAPI should just pass all the parameters to the backend, without converting them to a Java type, that way the RESTAPI doesn't need to know what are the names of the parameters. In the backend, if you need or wish to use the parameters to populate a Java bean I'd suggest to use reflection, or maybe the BeanMap class from commons-beanutils. Line 38: } Line 39: } http://gerrit.ovirt.org/#/c/37976/1/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/ApiMediaType.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/ApiMediaType.java: Line 19: Line 20: import javax.ws.rs.core.MediaType; Line 21: Line 22: public class ApiMediaType extends javax.ws.rs.core.MediaType { Line 23: private final static String APPLICATION = "application"; // todo externalize all string or use old approach for VV These strings shouldn't be externalized, they aren't translatable. Line 24: private final static String X_VIRT_VIEWER = "x-virt-viewer"; Line 25: Line 26: private final static String SLASH = "/"; Line 27: Line 22: public class ApiMediaType extends javax.ws.rs.core.MediaType { Line 23: private final static String APPLICATION = "application"; // todo externalize all string or use old approach for VV Line 24: private final static String X_VIRT_VIEWER = "x-virt-viewer"; Line 25: Line 26: private final static String SLASH = "/"; This is going too far in the use of constants, in my opinion. Line 27: Line 28: public final static String APPLICATION_X_YAML = "application/x-yaml"; Line 29: public final static MediaType APPLICATION_X_YAML_TYPE = new MediaType("application", "x-yaml"); Line 30: public final static String APPLICATION_PDF = "application/pdf"; http://gerrit.ovirt.org/#/c/37976/1/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/VmResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/VmResource.java: Line 100: @GET Line 101: @Actionable Line 102: @Produces({ApiMediaType.APPLICATION_X_VIRT_VIEWER}) Line 103: @Path("vv") Line 104: public Response vv(); This shouldn't be an action, but just an alternative media type for the VM entity: @GET @Produces({ApiMediaType.APPLICATION_X_VIRT_VIEWER}) public Response vv(); Also the name isn't very descriptive, I'd suggest "getVirtViewerConfiguration" or something similar. If, for whatever the reason, you end up doing this an an action then the method needs to be @POST, and it should receive an "Action" object as parameter. If you don't do this then the SDKs won't support it correctly. Line 105: Line 106: @POST Line 107: @Consumes({ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML}) Line 108: @Actionable http://gerrit.ovirt.org/#/c/37976/1/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 543: signatures: [] Line 544: urlparams: {} Line 545: headers: Line 546: Content-Type: {value: application/x-virt-viewer, required: true} Line 547: Filter: {value: true|false, required: false} No new link should be used, as this should just be an alternative media type for /vms/{vm:id}|rel=get. Line 548: - name: /vms/{vm:id}/logon|rel=logon Line 549: description: Perform automatic logon on the VM using the guest agent. Line 550: request: Line 551: body: http://gerrit.ovirt.org/#/c/37976/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmResource.java: Line 458: ConsoleOptions consoleOptions = ConsoleOptionsHelper.getConsoleOptions(uriInfo); Line 459: ConsoleOptionsParams queryParams = new ConsoleOptionsParams(consoleOptions); Line 460: VdcQueryReturnValue result = runQuery(VdcQueryType.GetConsoleDescriptorFile, queryParams); Line 461: String descriptor = result.getReturnValue(); Line 462: Response.ResponseBuilder builder = Response.ok(descriptor, ApiMediaType.APPLICATION_X_VIRT_VIEWER_TYPE); If possible don't use Strings to carry the information between the backend and the RESTAPI. As this should be completely opaque for the RESTAPI it is more efficient if you use an array of bytes. Line 463: return builder.build(); Line 464: } Line 465: Line 466: private GraphicsType deriveGraphicsType() { -- To view, visit http://gerrit.ovirt.org/37976 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I08814006b413a7f213561e1884166af4d0d6c8ae Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
