Merge pull request #25 from kbase/better-errors

Add better error classes, codes, with tests

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
+
 ### <url>/rpc
 
 Uses [JSON RPC 2.0 format](https://www.jsonrpc.org/specification).

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):

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)

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
@@ -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']
@@ -150,8 +151,7 @@ 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 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 +192,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', {})

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:

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={
@@ -30,8 +31,7 @@ def show_indexes(params, meta):
         headers={'Content-Type': 'application/json'},
     )
     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

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({

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)

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]

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]

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})

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},

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, {})
 
 

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')

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