From dcc213f60aa6b3c00a44ef896af35ede93700afb Mon Sep 17 00:00:00 2001 From: Beau Barker Date: Sat, 3 Jul 2021 19:41:12 +1000 Subject: [PATCH] Enforce returning a Result from methods --- jsonrpcserver/dispatcher.py | 9 +++++-- jsonrpcserver/response.py | 19 +++++++------- jsonrpcserver/result.py | 8 +++--- jsonrpcserver/status.py | 12 ++++----- tests/test_dispatcher.py | 51 ++++++++++++++++++++++++++++--------- 5 files changed, 66 insertions(+), 33 deletions(-) diff --git a/jsonrpcserver/dispatcher.py b/jsonrpcserver/dispatcher.py index 5300b99..dcdb7a8 100644 --- a/jsonrpcserver/dispatcher.py +++ b/jsonrpcserver/dispatcher.py @@ -25,7 +25,7 @@ from_result, to_serializable, ) -from .result import InvalidParams, InternalError, Result +from .result import InvalidParams, InternalError, Error, Result, Success default_deserializer = json.loads @@ -55,7 +55,12 @@ def call(method: Callable, args: list, kwargs: dict) -> Result: return InvalidParams(errors) try: - return method(*args, **kwargs) + result = method(*args, **kwargs) + return ( + InternalError("The method did not return a Result") + if not isinstance(result, (Success, Error)) + else result + ) except Exception as exc: # Other error inside method - server error logging.exception(exc) return InternalError(str(exc)) diff --git a/jsonrpcserver/response.py b/jsonrpcserver/response.py index 19b7a9c..f2e7004 100644 --- a/jsonrpcserver/response.py +++ b/jsonrpcserver/response.py @@ -16,9 +16,14 @@ """ from typing import Any, List, NamedTuple, Union -from . import status from .request import NOID from .result import Result, Success, UNSPECIFIED +from .status import ( + ERROR_INVALID_REQUEST, + ERROR_METHOD_NOT_FOUND, + ERROR_PARSE_ERROR, + ERROR_SERVER_ERROR, +) class SuccessResponse(NamedTuple): @@ -48,7 +53,7 @@ def ParseErrorResponse(data: Any) -> ErrorResponse: the id member in the Request Object. If there was an error in detecting the id in the Request object (e.g. Parse error/Invalid Request), it MUST be Null." """ - return ErrorResponse(status.JSONRPC_PARSE_ERROR_CODE, "Parse error", data, None) + return ErrorResponse(ERROR_PARSE_ERROR, "Parse error", data, None) def InvalidRequestResponse(data: Any) -> ErrorResponse: @@ -57,19 +62,15 @@ def InvalidRequestResponse(data: Any) -> ErrorResponse: the id member in the Request Object. If there was an error in detecting the id in the Request object (e.g. Parse error/Invalid Request), it MUST be Null." """ - return ErrorResponse( - status.JSONRPC_INVALID_REQUEST_CODE, "Invalid request", data, None - ) + return ErrorResponse(ERROR_INVALID_REQUEST, "Invalid request", data, None) def MethodNotFoundResponse(data: Any, id: Any) -> ErrorResponse: - return ErrorResponse( - status.JSONRPC_METHOD_NOT_FOUND_CODE, "Method not found", data, id - ) + return ErrorResponse(ERROR_METHOD_NOT_FOUND, "Method not found", data, id) def ServerErrorResponse(data: Any, id: Any) -> ErrorResponse: - return ErrorResponse(status.JSONRPC_SERVER_ERROR_CODE, "Server error", data, id) + return ErrorResponse(ERROR_SERVER_ERROR, "Server error", data, id) def from_result(result: Result, id: Any) -> Union[Response, None]: diff --git a/jsonrpcserver/result.py b/jsonrpcserver/result.py index d30ad04..4744dd8 100644 --- a/jsonrpcserver/result.py +++ b/jsonrpcserver/result.py @@ -25,7 +25,7 @@ def my_method(request: Request) -> Result: """ from typing import Any, NamedTuple, Optional, Union -from . import status +from .status import ERROR_INVALID_PARAMS, ERROR_METHOD_NOT_FOUND, ERROR_INTERNAL_ERROR # This is used to indicate when a value isn't present. We use this instead of # None, because None is a valid JSON-serializable type. @@ -47,12 +47,12 @@ class Error(NamedTuple): def InvalidParams(data: Any = UNSPECIFIED) -> Error: - return Error(status.JSONRPC_INVALID_PARAMS_CODE, "Invalid params", data) + return Error(ERROR_INVALID_PARAMS, "Invalid params", data) def MethodNotFound(data: Any) -> Error: - return Error(status.JSONRPC_METHOD_NOT_FOUND_CODE, "Method not found", data) + return Error(ERROR_METHOD_NOT_FOUND, "Method not found", data) def InternalError(data: Any) -> Error: - return Error(status.JSONRPC_INTERNAL_ERROR_CODE, "Internal error", data) + return Error(ERROR_INTERNAL_ERROR, "Internal error", data) diff --git a/jsonrpcserver/status.py b/jsonrpcserver/status.py index 7cb5421..e3139eb 100644 --- a/jsonrpcserver/status.py +++ b/jsonrpcserver/status.py @@ -67,9 +67,9 @@ HTTP_INTERNAL_ERROR = 500 # JSONRPC status codes from http://www.jsonrpc.org/specification#error_object -JSONRPC_PARSE_ERROR_CODE = -32700 -JSONRPC_INVALID_REQUEST_CODE = -32600 -JSONRPC_METHOD_NOT_FOUND_CODE = -32601 -JSONRPC_INVALID_PARAMS_CODE = -32602 -JSONRPC_INTERNAL_ERROR_CODE = -32603 -JSONRPC_SERVER_ERROR_CODE = -32000 +ERROR_PARSE_ERROR = -32700 +ERROR_INVALID_REQUEST = -32600 +ERROR_METHOD_NOT_FOUND = -32601 +ERROR_INVALID_PARAMS = -32602 +ERROR_INTERNAL_ERROR = -32603 +ERROR_SERVER_ERROR = -32000 diff --git a/tests/test_dispatcher.py b/tests/test_dispatcher.py index 5c8a865..38466dd 100644 --- a/tests/test_dispatcher.py +++ b/tests/test_dispatcher.py @@ -3,7 +3,6 @@ from typing import Any from unittest.mock import sentinel -from jsonrpcserver import status from jsonrpcserver.dispatcher import ( create_requests, default_deserializer, @@ -13,9 +12,15 @@ dispatch_to_response_pure, ) from jsonrpcserver.methods import Methods, global_methods -from jsonrpcserver.result import Result, Success from jsonrpcserver.request import Request, NOID from jsonrpcserver.response import ErrorResponse, SuccessResponse +from jsonrpcserver.result import Result, Success, InvalidParams +from jsonrpcserver.status import ( + ERROR_INTERNAL_ERROR, + ERROR_INVALID_PARAMS, + ERROR_INVALID_REQUEST, + ERROR_PARSE_ERROR, +) # def test_dispatch_to_response_pure_invalid_params_notification(): @@ -139,27 +144,29 @@ def test_dispatch_to_response_pure_notification(): assert response is None -def test_dispatch_to_response_pure_notification_invalid_jsonrpc(): +def test_dispatch_to_response_pure_invalid_json(): + """Unable to parse, must return an error""" response = dispatch_to_response_pure( methods=Methods(ping), context=None, schema_validator=default_schema_validator, deserializer=default_deserializer, - request='{"jsonrpc": "0", "method": "notify"}', + request="{", ) assert isinstance(response, ErrorResponse) + assert response.code == ERROR_PARSE_ERROR -def test_dispatch_to_response_pure_invalid_json(): - """Unable to parse, must return an error""" +def test_dispatch_to_response_pure_notification_invalid_jsonrpc(): response = dispatch_to_response_pure( methods=Methods(ping), context=None, schema_validator=default_schema_validator, deserializer=default_deserializer, - request="{", + request='{"jsonrpc": "0", "method": "notify"}', ) assert isinstance(response, ErrorResponse) + assert response.code == ERROR_INVALID_REQUEST def test_dispatch_to_response_pure_invalid_jsonrpc(): @@ -172,6 +179,7 @@ def test_dispatch_to_response_pure_invalid_jsonrpc(): request="{}", ) assert isinstance(response, ErrorResponse) + assert response.code == ERROR_INVALID_REQUEST def test_dispatch_to_response_pure_invalid_params(): @@ -187,6 +195,7 @@ def foo(colour: str) -> Result: request='{"jsonrpc": "2.0", "method": "foo", "params": ["blue"], "id": 1}', ) assert isinstance(response, ErrorResponse) + assert response.code == ERROR_INVALID_PARAMS def test_dispatch_to_response_pure_invalid_params_count(): @@ -201,7 +210,25 @@ def foo(colour: str, size: str): request='{"jsonrpc": "2.0", "method": "foo", "params": {"colour":"blue"}, "id": 1}', ) assert isinstance(response, ErrorResponse) - assert response.data == "missing a required argument: 'size'" + assert response.code == ERROR_INVALID_PARAMS + + +def test_dispatch_to_response_pure_enforcing_result(): + """Methods should return a Result, otherwise we get an Internal Error response.""" + + def not_a_result(): + return None + + response = dispatch_to_response_pure( + methods=Methods(not_a_result), + context=None, + schema_validator=default_schema_validator, + deserializer=default_deserializer, + request='{"jsonrpc": "2.0", "method": "not_a_result", "id": 1}', + ) + assert isinstance(response, ErrorResponse) + assert response.code == ERROR_INTERNAL_ERROR + assert response.data == "The method did not return a Result" # dispatch_to_response @@ -306,7 +333,7 @@ def test_examples_invalid_json(): request='[{"jsonrpc": "2.0", "method": "sum", "params": [1,2,4], "id": "1"}, {"jsonrpc": "2.0", "method"]', ) assert isinstance(response, ErrorResponse) - assert response.code == status.JSONRPC_PARSE_ERROR_CODE + assert response.code == ERROR_PARSE_ERROR def test_examples_empty_array(): @@ -319,7 +346,7 @@ def test_examples_empty_array(): deserializer=default_deserializer, ) assert isinstance(response, ErrorResponse) - assert response.code == status.JSONRPC_INVALID_REQUEST_CODE + assert response.code == ERROR_INVALID_REQUEST def test_examples_invalid_jsonrpc_batch(): @@ -335,7 +362,7 @@ def test_examples_invalid_jsonrpc_batch(): request="[1]", ) assert isinstance(response, ErrorResponse) - assert response.code == status.JSONRPC_INVALID_REQUEST_CODE + assert response.code == ERROR_INVALID_REQUEST def test_examples_multiple_invalid_jsonrpc(): @@ -351,7 +378,7 @@ def test_examples_multiple_invalid_jsonrpc(): request="[1, 2, 3]", ) assert isinstance(response, ErrorResponse) - assert response.code == status.JSONRPC_INVALID_REQUEST_CODE + assert response.code == ERROR_INVALID_REQUEST def test_examples_mixed_requests_and_notifications():