From a72b2c361057b53e00a2a1625c3aac9d2db30d51 Mon Sep 17 00:00:00 2001 From: Anna Bocharova Date: Sun, 22 Oct 2023 15:35:00 +0200 Subject: [PATCH] Feat: API v2 (#242) * Introducing version | v parameter. * Consistent arguments of API v2. * Fix linting issues. * Changed response for cancelTask command in v2. * getStatus command response remains. * Fixing mypy issues. * Changing response for update command of v2. * Changing the response for listAllStatus command of v2. * Fix linting issue. * Partial revert: keeping id naming. * Minor: readability. * Ref: readbility for getStatus command branching. * Update octoprint_octorelay/__init__.py * Ref: Ensure int for version * Ref: todo for subject parameter name for next major release * Type constraints for listing command. * Fix linting issue. * UI: Switching to API v2. --- octoprint_octorelay/__init__.py | 75 ++++++++++++++-------- octoprint_octorelay/listing.py | 14 +++++ setup.py | 2 +- tests/test_init.py | 107 +++++++++++++++++++++++++++----- ts/helpers/actions.spec.ts | 7 ++- ts/helpers/actions.ts | 3 +- 6 files changed, 164 insertions(+), 44 deletions(-) create mode 100644 octoprint_octorelay/listing.py diff --git a/octoprint_octorelay/__init__.py b/octoprint_octorelay/__init__.py index d7f45578..2b50ada5 100755 --- a/octoprint_octorelay/__init__.py +++ b/octoprint_octorelay/__init__.py @@ -19,6 +19,7 @@ ) from .driver import Relay from .task import Task +from .listing import Listing from .migrations import migrate from .model import Model, get_initial_model from .exceptions import HandlingException @@ -101,9 +102,9 @@ def has_switch_permission(self): self._logger.warn(f"Failed to check relay switching permission, {exception}") return False - def handle_list_all_command(self): + def handle_list_all_command(self) -> Listing: self._logger.debug("Collecting information on all the relay states") - active_relays = [] + active_relays: Listing = [] settings = self._settings.get([], merged=True) # expensive for index in RELAY_INDEXES: if bool(settings[index]["active"]): @@ -114,7 +115,7 @@ def handle_list_all_command(self): active_relays.append({ "id": index, "name": settings[index]["label_text"], - "active": relay.is_closed(), + "status": relay.is_closed(), }) return active_relays @@ -152,33 +153,55 @@ def handle_cancel_task_command(self, subject: str, target: bool, owner: str) -> def on_api_command(self, command, data): # pylint: disable=too-many-return-statements self._logger.info(f"Received the API command {command} with parameters: {data}") - index = data.get("pin") - if command in [GET_STATUS_COMMAND, UPDATE_COMMAND] and index is None: - return flask.abort(400, description="Parameter pin is missing") - if command == LIST_ALL_COMMAND: # API command to get relay statuses - response = self.handle_list_all_command() - self._logger.info(f"Responding to {LIST_ALL_COMMAND} command: {response}") + version = int( data.get("version") or data.get("v") or 1 ) + subject_param_name = "pin" if version == 1 else "subject" # todo remove pin when dropping v1 + subject = data.get(subject_param_name) + target = data.get("target") + if command in [GET_STATUS_COMMAND, UPDATE_COMMAND] and subject is None: + return flask.abort(400, description=f"Parameter {subject_param_name} is missing") + # API command to list all the relays with their names and statuses + if command == LIST_ALL_COMMAND: + relays = self.handle_list_all_command() + response = list(map(lambda item: { + "id": item["id"], + "name": item["name"], + "active": item["status"] + }, relays)) if version == 1 else relays # todo remove ternary branch when dropping v1 + self._logger.info(f"Responding {response} to {LIST_ALL_COMMAND} command") return flask.jsonify(response) - if command == GET_STATUS_COMMAND: # API command to get relay status + # API command to get relay status + if command == GET_STATUS_COMMAND: + is_closed = False # todo remove this when dropping v1 try: - is_closed = self.handle_get_status_command(index) - except HandlingException: - is_closed = False # todo should just abort in the next version - self._logger.info(f"Responding to {GET_STATUS_COMMAND} command: {is_closed}") - return flask.jsonify({"status": is_closed}) - if command == UPDATE_COMMAND: # API command to toggle the relay - target = data.get("target") + is_closed = self.handle_get_status_command(subject) + except HandlingException as exception: + if version != 1: # todo remove condition when dropping v1 + return flask.abort(exception.status) + self._logger.info(f"Responding {is_closed} to {GET_STATUS_COMMAND} command") + return flask.jsonify({ "status": is_closed }) + # API command to toggle the relay + if command == UPDATE_COMMAND: try: - state = self.handle_update_command(index, target if isinstance(target, bool) else None) - self._logger.debug(f"Responding to {UPDATE_COMMAND} command. Switched state to {state}") - return flask.jsonify({"status": "ok", "result": state}) + state = self.handle_update_command(subject, target if isinstance(target, bool) else None) + self._logger.debug(f"Responding {state} to {UPDATE_COMMAND} command") + if version == 1: + return flask.jsonify({ "status": "ok", "result": state }) # todo remove branch when dropping v1 + return flask.jsonify({ "status": state }) except HandlingException as exception: # todo: deprecate the behavior for 400, only abort in next version - return flask.jsonify({"status": "error"}) if exception.status == 400 else flask.abort(exception.status) - if command == CANCEL_TASK_COMMAND: # API command to cancel the postponed toggling task - self.handle_cancel_task_command(data["subject"], bool(data["target"]), data["owner"]) - self._logger.debug(f"Responding to {CANCEL_TASK_COMMAND} command") - return flask.jsonify({"status": "ok"}) - self._logger.warn(f"Unknown command {command}") + if version == 1 and exception.status == 400: + return flask.jsonify({ "status": "error" }) # todo remove this branch when dropping v1 + return flask.abort(exception.status) + # API command to cancel the postponed toggling task + if command == CANCEL_TASK_COMMAND: + cancelled = self.handle_cancel_task_command( + data.get("subject"), bool(target), data["owner"] # todo use subject after dropping v1 + ) + self._logger.debug(f"Responding {cancelled} to {CANCEL_TASK_COMMAND} command") + if version == 1: + return flask.jsonify({ "status": "ok" }) # todo remove this branch when dropping v1 + return flask.jsonify({ "cancelled": cancelled }) + # Unknown commands + self._logger.warn(f"Received unknown API command {command}") return flask.abort(400, description="Unknown command") def on_event(self, event, payload): diff --git a/octoprint_octorelay/listing.py b/octoprint_octorelay/listing.py new file mode 100644 index 00000000..e9a8ea30 --- /dev/null +++ b/octoprint_octorelay/listing.py @@ -0,0 +1,14 @@ +# -*- coding: utf-8 -*- +from typing import List +# todo after dropping 3.7 take TypedDict from typing instead +from typing_extensions import TypedDict + +# false positive, see https://github.com/pylint-dev/pylint/issues/4166 +# pylint: disable=too-many-ancestors +# pylint: disable=too-few-public-methods +class Element(TypedDict): + id: str + name: str + status: bool + +Listing = List[Element] diff --git a/setup.py b/setup.py index c4274e5f..d78ea1e5 100755 --- a/setup.py +++ b/setup.py @@ -50,7 +50,7 @@ def get_version_and_cmdclass(pkg_path): plugin_license = "AGPLv3" # Any additional requirements besides OctoPrint should be listed here -# todo after dropping 3.7 remove typing-extensions (used by model.py) +# todo after dropping 3.7 remove typing-extensions (used by model.py and listing.py) plugin_requires = ["RPi.GPIO", "typing-extensions"] # -------------------------------------------------------------------------------------------------------------------- diff --git a/tests/test_init.py b/tests/test_init.py index c188d655..e38e4757 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -827,14 +827,14 @@ def test_handle_list_all_command(self): "expectedJson": list(map(lambda index: { "id": index, "name": "TEST", - "active": False + "status": False }, RELAY_INDEXES)) }, { "closed": True, "expectedJson": list(map(lambda index: { "id": index, "name": "TEST", - "active": True + "status": True }, RELAY_INDEXES)) }] for case in cases: @@ -999,7 +999,9 @@ def test_handle_cancel_task_command(self): @patch("flask.jsonify") def test_on_api_command(self, jsonify_mock): # Should call a handler and respond with expected payload - self.plugin_instance.handle_list_all_command = Mock(return_value=[]) + self.plugin_instance.handle_list_all_command = Mock(return_value=[ + {"id": "r1", "name": "Test", "status": True} + ]) self.plugin_instance.handle_get_status_command = Mock(return_value=True) self.plugin_instance.handle_update_command = Mock(return_value=False) self.plugin_instance.handle_cancel_task_command = Mock(return_value=True) @@ -1010,7 +1012,15 @@ def test_on_api_command(self, jsonify_mock): "expectedMethod": self.plugin_instance.handle_list_all_command, "expectedArguments": [], "expectedOutcome": jsonify_mock, - "expectedPayload": [], + "expectedPayload": [{"id": "r1", "name": "Test", "active": True}], + }, + { + "command": "listAllStatus", + "data": {"v": 2}, + "expectedMethod": self.plugin_instance.handle_list_all_command, + "expectedArguments": [], + "expectedOutcome": jsonify_mock, + "expectedPayload": [{"id": "r1", "name": "Test", "status": True}], }, { "command": "getStatus", @@ -1020,6 +1030,14 @@ def test_on_api_command(self, jsonify_mock): "expectedOutcome": jsonify_mock, "expectedPayload": {"status": True}, }, + { + "command": "getStatus", + "data": { "version": 2, "subject": "r4" }, + "expectedMethod": self.plugin_instance.handle_get_status_command, + "expectedArguments": ["r4"], + "expectedOutcome": jsonify_mock, + "expectedPayload": {"status": True}, + }, { "command": "update", "data": { "pin": "r4", "target": True }, @@ -1028,6 +1046,14 @@ def test_on_api_command(self, jsonify_mock): "expectedOutcome": jsonify_mock, "expectedPayload": {"status": "ok", "result": False}, }, + { + "command": "update", + "data": { "v": 2, "subject": "r4", "target": True }, + "expectedMethod": self.plugin_instance.handle_update_command, + "expectedArguments": ["r4", True], + "expectedOutcome": jsonify_mock, + "expectedPayload": {"status": False}, + }, { "command": "cancelTask", "data": { "subject": "r4", "owner": "STARTUP", "target": True }, @@ -1035,9 +1061,19 @@ def test_on_api_command(self, jsonify_mock): "expectedArguments": [ "r4", True, "STARTUP" ], "expectedOutcome": jsonify_mock, "expectedPayload": {"status": "ok"}, + }, + { + "command": "cancelTask", + "data": { "v": 2, "subject": "r4", "owner": "STARTUP", "target": True }, + "expectedMethod": self.plugin_instance.handle_cancel_task_command, + "expectedArguments": [ "r4", True, "STARTUP" ], + "expectedOutcome": jsonify_mock, + "expectedPayload": {"cancelled": True}, } ] for case in cases: + case["expectedMethod"].reset_mock() + case["expectedOutcome"].reset_mock() self.plugin_instance.on_api_command(case["command"], case["data"]) case["expectedMethod"].assert_called_with(*case["expectedArguments"]) case["expectedOutcome"].assert_called_with(case["expectedPayload"]) @@ -1047,27 +1083,70 @@ def test_on_api_command(self, jsonify_mock): def test_on_api_command__update_exceptions(self, jsonify_mock, abort_mock): # Should respond with a faulty HTTP code or status error when handler raises cases = [ - { "status": 403, "expectedMethod": abort_mock, "expectedArgument": 403 }, - { "status": 400, "expectedMethod": jsonify_mock, "expectedArgument": {"status": "error"} } + { + "payload": { "pin": "r4" }, + "status": 403, + "expectedMethod": abort_mock, + "expectedArgument": 403 + }, + { + "payload": { "version": 2, "subject": "r4" }, + "status": 403, + "expectedMethod": abort_mock, + "expectedArgument": 403 + }, + { + "payload": { "pin": "r4" }, + "status": 400, + "expectedMethod": jsonify_mock, + "expectedArgument": { "status": "error"} + }, + { + "payload": { "v": 2, "subject": "r4" }, + "status": 400, + "expectedMethod": abort_mock, + "expectedArgument": 400 + } ] for case in cases: + case["expectedMethod"].reset_mock() self.plugin_instance.handle_update_command = Mock(side_effect=HandlingException(case["status"])) - self.plugin_instance.on_api_command("update", {"pin": "r4"}) + self.plugin_instance.on_api_command("update", case["payload"]) case["expectedMethod"].assert_called_with(case["expectedArgument"]) + @patch("flask.abort") @patch("flask.jsonify") - def test_om_api_command__get_status_exception(self, jsonify_mock): + def test_om_api_command__get_status_exception(self, jsonify_mock, abort_mock): # Should respond with status false when handler raises + cases = [ + { + "payload": { "pin": "r4" }, + "expectedMethod": jsonify_mock, + "expectedArgument": {"status": False} + }, + { + "payload": { "v": 2, "subject": "r4" }, + "expectedMethod": abort_mock, + "expectedArgument": 400 + } + ] self.plugin_instance.handle_get_status_command = Mock(side_effect=HandlingException(400)) - self.plugin_instance.on_api_command("getStatus", {"pin": "r4"}) - jsonify_mock.assert_called_with({"status": False}) + for case in cases: + case["expectedMethod"].reset_mock() + self.plugin_instance.on_api_command("getStatus", case["payload"]) + case["expectedMethod"].assert_called_with(case["expectedArgument"]) @patch("flask.abort") def test_on_api_command__missing_parameters(self, abort_mock): - commands = ["getStatus", "update"] - for command in commands: - self.plugin_instance.on_api_command(command, {}) - abort_mock.assert_called_with(400, description="Parameter pin is missing") + cases = [ + { "command": "getStatus", "version": 1, "missing": "pin" }, + { "command": "update", "version": 1, "missing": "pin" }, + { "command": "getStatus", "version": 2, "missing": "subject" }, + { "command": "update", "version": 2, "missing": "subject" } + ] + for case in cases: + self.plugin_instance.on_api_command(case["command"], { "version": case["version"] }) + abort_mock.assert_called_with(400, description=f"Parameter {case['missing']} is missing") @patch("flask.abort") def test_on_api_command__unknown(self, abort_mock): diff --git a/ts/helpers/actions.spec.ts b/ts/helpers/actions.spec.ts index 965cf1ae..836ab199 100644 --- a/ts/helpers/actions.spec.ts +++ b/ts/helpers/actions.spec.ts @@ -30,7 +30,8 @@ describe("Actions", () => { upcoming: null, }); expect(apiMock).toHaveBeenCalledWith("octorelay", "update", { - pin: "r1", + v: 2, + subject: "r1", }); }); @@ -56,7 +57,8 @@ describe("Actions", () => { elementMock.modal.mockClear(); elementMock.on.mock.calls[1][1](); // confirm expect(apiMock).toHaveBeenCalledWith("octorelay", "update", { - pin: "r2", + v: 2, + subject: "r2", }); expect(elementMock.modal).toHaveBeenCalledWith("hide"); }); @@ -70,6 +72,7 @@ describe("Actions", () => { deadline: 86400, }); expect(apiMock).toHaveBeenCalledWith("octorelay", "cancelTask", { + v: 2, owner: "PRINTING_STARTED", subject: "r2", target: true, diff --git a/ts/helpers/actions.ts b/ts/helpers/actions.ts index 949a5c9e..7a85d000 100644 --- a/ts/helpers/actions.ts +++ b/ts/helpers/actions.ts @@ -3,7 +3,7 @@ import { ownCode } from "./const"; export const toggleRelay = (key: string, relay: Relay) => { const command = () => - OctoPrint.simpleApiCommand(ownCode, "update", { pin: key }); + OctoPrint.simpleApiCommand(ownCode, "update", { v: 2, subject: key }); if (!relay.confirm_off) { return command(); } @@ -28,6 +28,7 @@ export const toggleRelay = (key: string, relay: Relay) => { export const cancelTask = (key: string, { owner, target }: Task) => OctoPrint.simpleApiCommand(ownCode, "cancelTask", { + v: 2, subject: key, owner, target,