From 090b6b04df99751f86edc4bfc161445747814634 Mon Sep 17 00:00:00 2001 From: Jay R Bolton Date: Tue, 18 Aug 2020 16:06:42 -0700 Subject: [PATCH 1/2] Add better error classes, codes, with tests --- README.md | 8 +++++ src/es_client/query.py | 8 ++--- src/exceptions.py | 35 +++++++++++++++---- src/search1_conversion/convert_params.py | 7 ++-- src/search2_conversion/convert_params.py | 5 +-- src/search2_rpc/service.py | 3 +- src/server/__main__.py | 3 +- src/utils/config.py | 4 ++- src/utils/user_profiles.py | 4 +-- src/utils/workspace.py | 17 ++++----- tests/unit/es_client/test_es_client.py | 5 +-- .../test_search1_convert_params.py | 5 +-- .../test_search2_convert_params.py | 3 +- tests/unit/utils/test_user_profiles.py | 3 +- tests/unit/utils/test_workspace.py | 19 ++++++++-- 15 files changed, 91 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index b15cb2c..a29bc65 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,14 @@ The [search configuration file](https://github.com/kbase/index_runner_spec/blob/ * `aliases` is a list of Elasticsearch index aliases to a list of index names. These are all searchable as index names. * `mappings` gives the type definitions for every specific index in the Elasticsearch database. Use these type definitions to find out what kind of data you will get back in the search results. +### Custom error codes + +* `-32001` - authorization failed +* `-32002` - unknown ES index name +* `-32003` - Elasticsearch response error +* `-32004` - User profile service response error +* `-32005` - Unknown workspace type + ### /rpc Uses [JSON RPC 2.0 format](https://www.jsonrpc.org/specification). diff --git a/src/es_client/query.py b/src/es_client/query.py index de5a52f..0bd4960 100644 --- a/src/es_client/query.py +++ b/src/es_client/query.py @@ -9,7 +9,7 @@ from src.utils.workspace import ws_auth from src.utils.config import config from src.utils.obj_utils import get_path -from src.exceptions import UnknownIndex +from src.exceptions import UnknownIndex, ElasticsearchError def search(params, meta): @@ -96,14 +96,14 @@ def _handle_es_err(resp): try: resp_json = resp.json() except Exception: - raise RuntimeError(resp.text) + raise ElasticsearchError(resp.text) err_type = get_path(resp_json, ['error', 'root_cause', 0, 'type']) err_reason = get_path(resp_json, ['error', 'reason']) if err_type is None: - raise RuntimeError(resp.text) + raise ElasticsearchError(resp.text) if err_type == 'index_not_found_exception': raise UnknownIndex(err_reason) - raise RuntimeError(err_reason) + raise ElasticsearchError(err_reason) def _handle_response(resp_json): diff --git a/src/exceptions.py b/src/exceptions.py index 7b8d3ce..93a9444 100644 --- a/src/exceptions.py +++ b/src/exceptions.py @@ -1,13 +1,34 @@ -class Search2Error(Exception): +class ResponseError(Exception): - def __init__(self, msg): - self.msg = msg + def __init__(self, code=-32000, message='Server error', status=400): + self.message = message + self.code = code + self.status = status - def __str__(self): - return self.msg +class UnknownType(ResponseError): -class UnknownIndex(Search2Error): - pass + def __init__(self, message): + super().__init__(code=-32005, message=message) + + +class ElasticsearchError(ResponseError): + + def __init__(self, message): + msg = f"Elasticsearch request error:\n{message}" + super().__init__(code=-32003, message=msg) + + +class UnknownIndex(ResponseError): + + def __init__(self, message): + super().__init__(code=-32002, message=message) + + +class UserProfileError(ResponseError): + + def __init__(self, url, resp_text): + msg = "User profile service error:\nResponse: {resp_text}\nURL: {url}" + super().__init__(code=-32004, message=msg) diff --git a/src/search1_conversion/convert_params.py b/src/search1_conversion/convert_params.py index 68df2f0..99623e0 100644 --- a/src/search1_conversion/convert_params.py +++ b/src/search1_conversion/convert_params.py @@ -35,6 +35,7 @@ """ from src.utils.config import config from src.utils.obj_utils import get_any +from src.exceptions import ResponseError # Unversioned feature index name/alias, (eg "genome_features") _FEATURES_UNVERSIONED = config['global']['genome_features_current_index_name'] @@ -151,7 +152,7 @@ def _get_search_params(params): query['bool']['must'].append({'range': {'timestamp': {'gte': min_ts, 'lte': max_ts}}}) else: # TODO proper error - raise RuntimeError("Invalid timestamp range in match_filter/timestamp.") + raise ResponseError(code=-32602, message="Invalid timestamp range in match_filter/timestamp") # Handle a search on tags, which corresponds to the generic `tags` field in all indexes. if match_filter.get('source_tags'): # If source_tags_blacklist is `1`, then we are **excluding** these tags. @@ -192,8 +193,8 @@ def _get_search_params(params): if prop in _SORT_PROP_MAPPING: prop = _SORT_PROP_MAPPING[sort_rule['property']] else: - # TODO proper error - raise RuntimeError(f"Invalid non-object sorting property '{prop}'") + msg = f"Invalid non-object sorting property '{prop}'" + raise ResponseError(code=-32602, message=msg) order = 'asc' if ascending else 'desc' sort.append({prop: {'order': order}}) pagination = params.get('pagination', {}) diff --git a/src/search2_conversion/convert_params.py b/src/search2_conversion/convert_params.py index 96010b4..b35249d 100644 --- a/src/search2_conversion/convert_params.py +++ b/src/search2_conversion/convert_params.py @@ -1,4 +1,5 @@ from src.utils.config import config +from src.exceptions import UnknownType def search_workspace(params, meta): @@ -19,9 +20,9 @@ def search_workspace(params, meta): mapping = config['global']['ws_type_to_indexes'] for typ in params['types']: if typ not in mapping: - # TODO proper error class with code available = list(mapping.keys()) - raise RuntimeError(f"Unknown type: {type}. Available types: {available}.") + msg = f"Unknown type: {type}. Available types: {available}." + raise UnknownType(msg) indexes.append(mapping[typ]) converted["indexes"] = indexes if "types" not in params or len(params["types"]) == 0: diff --git a/src/search2_rpc/service.py b/src/search2_rpc/service.py index 9e3187c..8275b65 100644 --- a/src/search2_rpc/service.py +++ b/src/search2_rpc/service.py @@ -10,6 +10,7 @@ from src.utils.config import config from src.utils.logger import logger from src.search2_conversion import convert_params, convert_result +from src.exceptions import ElasticsearchError service = jsonrpcbase.JSONRPCService( info={ @@ -31,7 +32,7 @@ def show_indexes(params, meta): ) if not resp.ok: # TODO better error class - raise RuntimeError(f'Elasticsearch error:\n{resp.text}') + raise ElasticsearchError(resp.text) resp_json = resp.json() result = [] # Drop the prefixes diff --git a/src/server/__main__.py b/src/server/__main__.py index c0fff71..539abaf 100644 --- a/src/server/__main__.py +++ b/src/server/__main__.py @@ -61,7 +61,8 @@ async def page_not_found(request, err): async def any_exception(request, err): """ Handle any unexpected server error. - Ideally, this should never be reached. JSONRPCBase will handle method error responses. + Theoretically, this should never be reached. JSONRPCBase will handle method + error responses. """ traceback.print_exc() return sanic.response.json({ diff --git a/src/utils/config.py b/src/utils/config.py index e813061..f1f8f6d 100644 --- a/src/utils/config.py +++ b/src/utils/config.py @@ -20,7 +20,9 @@ def init_config(): 'https://ci.kbase.us/services/user_profile/rpc' ).strip('/') # Load the global configuration release (non-environment specific, public config) - if not config_url.startswith('http'): + allowed_protocols = ('https://', 'http://', 'file://') + matches_protocol = (config_url.startswith(prot) for prot in allowed_protocols) + if not any(matches_protocol): raise RuntimeError(f"Invalid config url: {config_url}") with urllib.request.urlopen(config_url) as res: # nosec global_config = yaml.safe_load(res) diff --git a/src/utils/user_profiles.py b/src/utils/user_profiles.py index bbf3660..9ffe2e9 100644 --- a/src/utils/user_profiles.py +++ b/src/utils/user_profiles.py @@ -2,6 +2,7 @@ import requests from src.utils.config import config +from src.exceptions import UserProfileError def get_user_profiles(usernames: list, auth_token): @@ -26,6 +27,5 @@ def get_user_profiles(usernames: list, auth_token): headers=headers, ) if not resp.ok: - # TODO better error class - raise RuntimeError(url, resp.text) + raise UserProfileError(url, resp.text) return resp.json()['result'][0] diff --git a/src/utils/workspace.py b/src/utils/workspace.py index 015f9ea..2a3b57a 100644 --- a/src/utils/workspace.py +++ b/src/utils/workspace.py @@ -6,7 +6,7 @@ from typing import Optional from src.utils.config import config -# from src.exceptions import UnauthorizedAccess +from src.exceptions import ResponseError def ws_auth(auth_token): @@ -61,11 +61,12 @@ def _req(method: str, params: dict, token: Optional[str]): headers=headers, data=json.dumps(payload), ) - if not resp.ok: - # TODO better error class - raise RuntimeError(resp.text) - result = resp.json().get('result') - if not result or len(result) == 0: - # TODO better error class - raise RuntimeError(resp.text) + result = None + try: + result = resp.json().get('result') + except json.decoder.JSONDecodeError: + pass + if not resp.ok or not result or len(result) == 0: + msg = f"Authorization failed with response:\n{resp.text}" + raise ResponseError(code=-32001, message=msg, status=403) return result[0] diff --git a/tests/unit/es_client/test_es_client.py b/tests/unit/es_client/test_es_client.py index a4466b1..41cf4e1 100644 --- a/tests/unit/es_client/test_es_client.py +++ b/tests/unit/es_client/test_es_client.py @@ -10,6 +10,7 @@ from src.exceptions import UnknownIndex from src.es_client import search from src.utils.wait_for_service import wait_for_service +from src.exceptions import ElasticsearchError from tests.helpers import init_elasticsearch @@ -146,7 +147,7 @@ def test_es_response_error(): index_name_str = prefix + delim + "default_search" url = config['elasticsearch_url'] + '/' + index_name_str + '/_search' responses.add(responses.POST, url, json={}, status=500) - with pytest.raises(RuntimeError): + with pytest.raises(ElasticsearchError): search({}, {'auth': None}) @@ -158,5 +159,5 @@ def test_es_response_error_no_json(): index_name_str = prefix + delim + "default_search" url = config['elasticsearch_url'] + '/' + index_name_str + '/_search' responses.add(responses.POST, url, body="!", status=500) - with pytest.raises(RuntimeError): + with pytest.raises(ElasticsearchError): search({}, {'auth': None}) diff --git a/tests/unit/search1_conversion/test_search1_convert_params.py b/tests/unit/search1_conversion/test_search1_convert_params.py index ae07f01..e0433b1 100644 --- a/tests/unit/search1_conversion/test_search1_convert_params.py +++ b/tests/unit/search1_conversion/test_search1_convert_params.py @@ -1,6 +1,7 @@ import pytest from src.search1_conversion import convert_params +from src.exceptions import ResponseError def test_search_objects_valid(): @@ -80,7 +81,7 @@ def test_search_objects_timestamp(): def test_search_objects_timestamp_invalid(): - with pytest.raises(RuntimeError): + with pytest.raises(ResponseError): params = { 'match_filter': {'timestamp': {'min_date': 0, 'max_date': 0}} } @@ -155,7 +156,7 @@ def test_search_objects_sorting(): def test_search_objects_sorting_invalid_prop(): - with pytest.raises(RuntimeError): + with pytest.raises(ResponseError): params = { 'sorting_rules': [ {'property': 'x', 'is_object_property': False, 'ascending': False}, diff --git a/tests/unit/search2_conversion/test_search2_convert_params.py b/tests/unit/search2_conversion/test_search2_convert_params.py index d8a9cc5..fd4c81f 100644 --- a/tests/unit/search2_conversion/test_search2_convert_params.py +++ b/tests/unit/search2_conversion/test_search2_convert_params.py @@ -2,6 +2,7 @@ from src.utils.config import config from src.search2_conversion import convert_params +from src.exceptions import ResponseError # TODO Test invalid parameters @@ -91,7 +92,7 @@ def test_search_workspace_invalid_type(): params = { 'types': ['xyz'], } - with pytest.raises(RuntimeError): + with pytest.raises(ResponseError): convert_params.search_workspace(params, {}) diff --git a/tests/unit/utils/test_user_profiles.py b/tests/unit/utils/test_user_profiles.py index 6592a91..fe880ff 100644 --- a/tests/unit/utils/test_user_profiles.py +++ b/tests/unit/utils/test_user_profiles.py @@ -3,6 +3,7 @@ from src.utils.config import config from src.utils.user_profiles import get_user_profiles +from src.exceptions import UserProfileError mock_resp = { @@ -32,5 +33,5 @@ def test_get_user_profiles_noauth(): @responses.activate def test_get_user_profiles_invalid(): responses.add(responses.POST, config['user_profile_url'], status=400) - with pytest.raises(RuntimeError): + with pytest.raises(UserProfileError): get_user_profiles(['username'], 'x') diff --git a/tests/unit/utils/test_workspace.py b/tests/unit/utils/test_workspace.py index 2a4683f..06f5aa5 100644 --- a/tests/unit/utils/test_workspace.py +++ b/tests/unit/utils/test_workspace.py @@ -3,6 +3,7 @@ from src.utils.config import config from src.utils.workspace import ws_auth, get_workspace_info, get_object_info +from src.exceptions import ResponseError mock_ws_ids = { "version": "1.1", @@ -94,8 +95,12 @@ def test_ws_auth_blank(): def test_ws_auth_invalid(): # Mock the workspace call responses.add(responses.POST, config['workspace_url'], status=403) - with pytest.raises(RuntimeError): + with pytest.raises(ResponseError) as ctx: ws_auth('x') + err = ctx.value + assert err.status == 403 + assert err.code == -32001 + assert len(err.message) > 0 @responses.activate @@ -114,8 +119,12 @@ def test_get_workspace_info_blank(): @responses.activate def test_get_workspace_info_invalid(): responses.add(responses.POST, config['workspace_url'], status=500) - with pytest.raises(RuntimeError): + with pytest.raises(ResponseError) as ctx: get_workspace_info(1, 'token') + err = ctx.value + assert err.status == 403 + assert err.code == -32001 + assert len(err.message) > 0 @responses.activate @@ -125,8 +134,12 @@ def test_get_workspace_info_invalid2(): } responses.add(responses.POST, config['workspace_url'], json=resp, status=200) - with pytest.raises(RuntimeError): + with pytest.raises(ResponseError) as ctx: get_workspace_info(1, 'token') + err = ctx.value + assert err.status == 403 + assert err.code == -32001 + assert len(err.message) > 0 @responses.activate From 45df9558a042b13bd629638d28c414f253a78595 Mon Sep 17 00:00:00 2001 From: Jay R Bolton Date: Wed, 19 Aug 2020 12:56:33 -0700 Subject: [PATCH 2/2] Remove a few TODOs --- src/search1_conversion/convert_params.py | 3 +-- src/search2_rpc/service.py | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/search1_conversion/convert_params.py b/src/search1_conversion/convert_params.py index 99623e0..d6d48d5 100644 --- a/src/search1_conversion/convert_params.py +++ b/src/search1_conversion/convert_params.py @@ -26,7 +26,7 @@ used to create the object and their version and version control commit hash. Not all keys may be present; if not their values were not available in the search data. - highlight - dict of string to list of string - search result highlights from ES. TODO + highlight - dict of string to list of string - search result highlights from ES The keys are the field names and the list contains the sections in each field that matched the search query. Fields with no hits will not be available. Short fields that matched are shown in their @@ -151,7 +151,6 @@ def _get_search_params(params): query['bool']['must'] = query['bool'].get('must', []) query['bool']['must'].append({'range': {'timestamp': {'gte': min_ts, 'lte': max_ts}}}) else: - # TODO proper error raise ResponseError(code=-32602, message="Invalid timestamp range in match_filter/timestamp") # Handle a search on tags, which corresponds to the generic `tags` field in all indexes. if match_filter.get('source_tags'): diff --git a/src/search2_rpc/service.py b/src/search2_rpc/service.py index 8275b65..3a7dbfb 100644 --- a/src/search2_rpc/service.py +++ b/src/search2_rpc/service.py @@ -31,7 +31,6 @@ def show_indexes(params, meta): headers={'Content-Type': 'application/json'}, ) if not resp.ok: - # TODO better error class raise ElasticsearchError(resp.text) resp_json = resp.json() result = []