From 083e44d5f596369c0a2cdc2af4606308426d2dd0 Mon Sep 17 00:00:00 2001 From: Max Yu <18641481+maxyu1115@users.noreply.github.com> Date: Sun, 10 Sep 2023 22:29:26 -0700 Subject: [PATCH] Refactored exception handling, and added top level integration tests --- integration-tests/conftest.py | 5 ++ integration-tests/test_controlplane.py | 24 ++++++++ integration-tests/test_dataplane.py | 22 ++++++++ memas/app.py | 8 +++ memas/context_manager.py | 8 +-- memas/interface/exceptions.py | 61 ++++++++++++++++++--- memas/storage_driver/memas_metadata.py | 24 ++++---- tests/interface/test_exceptions.py | 15 +++++ tests/storage_driver/test_memas_metadata.py | 20 +++++++ 9 files changed, 162 insertions(+), 25 deletions(-) create mode 100644 integration-tests/test_controlplane.py create mode 100644 integration-tests/test_dataplane.py create mode 100644 tests/interface/test_exceptions.py diff --git a/integration-tests/conftest.py b/integration-tests/conftest.py index da0cd4a..1de8a18 100644 --- a/integration-tests/conftest.py +++ b/integration-tests/conftest.py @@ -64,6 +64,11 @@ def create_test_app(): app: Flask = create_test_app() +@pytest.fixture +def test_client(): + yield app.test_client() + + @pytest.fixture def es_client(): with app.app_context(): diff --git a/integration-tests/test_controlplane.py b/integration-tests/test_controlplane.py new file mode 100644 index 0000000..c68515c --- /dev/null +++ b/integration-tests/test_controlplane.py @@ -0,0 +1,24 @@ + + +def test_create_user(test_client): + resp = test_client.post("/cp/create_user", json={"namespace_pathname": "create_user_1"}) + assert resp.status_code == 200 + + +def test_create_user_existing(test_client): + test_client.post("/cp/create_user", json={"namespace_pathname": "create_user_2"}) + resp = test_client.post("/cp/create_user", json={"namespace_pathname": "create_user_2"}) + assert resp.status_code == 400 + assert resp.json["error_code"] == "namespace_exists" + + +def test_create_corpus(test_client): + resp = test_client.post("/cp/create_corpus", json={"corpus_pathname": "create_user_1:create_corpus_1"}) + assert resp.status_code == 200 + + +def test_create_corpus_existing(test_client): + test_client.post("/cp/create_corpus", json={"corpus_pathname": "create_user_2:create_corpus_2"}) + resp = test_client.post("/cp/create_corpus", json={"corpus_pathname": "create_user_2:create_corpus_2"}) + assert resp.status_code == 400 + assert resp.json["error_code"] == "namespace_exists" diff --git a/integration-tests/test_dataplane.py b/integration-tests/test_dataplane.py new file mode 100644 index 0000000..d77b819 --- /dev/null +++ b/integration-tests/test_dataplane.py @@ -0,0 +1,22 @@ +import time + + +def test_memorize_then_recall(test_client): + namespace_pathname = "memorize" + corpus_pathname = namespace_pathname + ":memorize_1" + resp1 = test_client.post("/cp/create_user", json={"namespace_pathname": namespace_pathname}) + print(resp1) + resp2 = test_client.post("/cp/create_corpus", json={"corpus_pathname": corpus_pathname}) + print(resp2) + + resp3 = test_client.post( + "/dp/memorize", json={"corpus_pathname": corpus_pathname, "document": "What's MeMaS", "citation": {}}) + assert resp3.status_code == 200 + assert resp3.json["success"] + + time.sleep(1) + + resp4 = test_client.post("/dp/recall", json={"namespace_pathname": namespace_pathname, "clue": "What's MeMaS"}) + assert resp4.status_code == 200 + assert len(resp4.json) == 1 + assert resp4.json[0]["document"] == "What's MeMaS" diff --git a/memas/app.py b/memas/app.py index 78545b8..0f03bdd 100644 --- a/memas/app.py +++ b/memas/app.py @@ -1,6 +1,8 @@ +import traceback import yaml from flask import Flask from memas.context_manager import ContextManager +from memas.interface.exceptions import MemasException def create_app(config_filename, *, first_init=False): @@ -15,6 +17,12 @@ def create_app(config_filename, *, first_init=False): app.ctx.init() + @app.errorhandler(MemasException) + def handle_memas_exception(e: MemasException): + app.logger.info(f"{e.__class__.__name__}: {e.return_obj()}") + # app.logger.info(traceback.format_exc()) + return e.return_obj(), e.status_code.value + from memas.dataplane import dataplane from memas.controlplane import controlplane app.register_blueprint(dataplane) diff --git a/memas/context_manager.py b/memas/context_manager.py index c8a6375..205ea00 100644 --- a/memas/context_manager.py +++ b/memas/context_manager.py @@ -7,7 +7,7 @@ from cassandra.cqlengine import connection as c_connection from elasticsearch import Elasticsearch from pymilvus import connections as milvus_connection -from memas.interface.exceptions import NotProperlyInitializedException +from memas.interface.exceptions import IllegalStateException from memas.interface.storage_driver import CorpusDocumentMetadataStore, CorpusDocumentStore, CorpusVectorStore, MemasMetadataStore from memas.storage_driver import corpus_doc_metadata, corpus_doc_store, corpus_vector_store, memas_metadata from memas.corpus.corpus_provider import CorpusProvider @@ -85,7 +85,7 @@ def setup_cassandra_keyspace(self): 'replication_factor': self.consts.cassandra_replication_factor } # NOTE: management.create_keyspace_simple doesn't work when we don't have a single keyspace... - create_keyspace_query = f"CREATE KEYSPACE IF NOT EXISTS {self.consts.cassandra_keyspace} WITH replication = {replication_options};" + create_keyspace_query = f"CREATE KEYSPACE {self.consts.cassandra_keyspace} WITH replication = {replication_options};" session.execute(create_keyspace_query) session.shutdown() @@ -103,7 +103,7 @@ def init_clients(self) -> None: def first_init_datastores(self) -> None: if self.es is None: - raise NotProperlyInitializedException("Attempted to initialize data stores before connectors/clients") + raise IllegalStateException("Attempted to initialize data stores before connectors/clients") self.corpus_doc = corpus_doc_store.ESDocumentStore(self.es) self.memas_metadata.first_init() @@ -113,7 +113,7 @@ def first_init_datastores(self) -> None: def init_datastores(self) -> None: if self.es is None: - raise NotProperlyInitializedException("Attempted to initialize data stores before connectors/clients") + raise IllegalStateException("Attempted to initialize data stores before connectors/clients") self.corpus_doc = corpus_doc_store.ESDocumentStore(self.es) self.memas_metadata.init() diff --git a/memas/interface/exceptions.py b/memas/interface/exceptions.py index e9c485e..a631cb0 100644 --- a/memas/interface/exceptions.py +++ b/memas/interface/exceptions.py @@ -1,25 +1,68 @@ +from enum import Enum -class BadArgumentException(Exception): +class MemasInternalException(Exception): + def __init__(self, *args: object) -> None: + super().__init__(*args) + + +class IllegalStateException(MemasInternalException): def __init__(self, msg: str) -> None: super().__init__(msg) -class IllegalNameException(BadArgumentException): - def __init__(self, pathname: str) -> None: - super().__init__(f"\"{pathname}\" is not a valid pathname") +class StatusCode(Enum): + # The request could not be understood by the server due to incorrect syntax. The client SHOULD NOT repeat the request without modifications. + BAD_REQUEST = 400 + # Indicates that the request requires user authentication information. The client MAY repeat the request with a suitable Authorization header field + UNAUTHORIZED = 401 + # Unauthorized request. The client does not have access rights to the content. Unlike 401, the client’s identity is known to the server. + FORBIDDEN = 403 + # The server can not find the requested resource. + NOT_FOUND = 404 + # The request HTTP method is known by the server but has been disabled and cannot be used for that resource. + METHOD_NOT_ALLOWED = 405 -class NamespaceExistsException(Exception): - def __init__(self, pathname: str) -> None: - super().__init__(f"\"{pathname}\" already exists") +# Each memas error needs to have an unique error code +class ErrorCode(Enum): + NamespaceExists = "namespace_exists" + NamespaceDoesNotExist = "namespace_does_not_exist" + NamespaceIllegalName = "namespace_illegal_name" -class NotProperlyInitializedException(Exception): - def __init__(self, msg: str) -> None: +class MemasException(Exception): + def __init__(self, error_code: ErrorCode, msg: str, additional_details: str = None) -> None: super().__init__(msg) + self.status_code: StatusCode = StatusCode.BAD_REQUEST + self.error_code: ErrorCode = error_code + self.msg: str = msg + self.additional_details: str = additional_details + + def return_obj(self): + resp = {"error_code": self.error_code.value, "msg": self.msg} + if self.additional_details: + resp["details"] = self.additional_details + return resp + + +class IllegalNameException(MemasException): + def __init__(self, pathname: str) -> None: + super().__init__(ErrorCode.NamespaceIllegalName, f"\"{pathname}\" is not a valid pathname") + + +class NamespaceExistsException(MemasException): + def __init__(self, pathname: str, additional_details: str = None) -> None: + super().__init__(ErrorCode.NamespaceExists, f"\"{pathname}\" already exists", additional_details) + + +class NamespaceDoesNotExistException(MemasException): + def __init__(self, pathname: str, additional_details: str = None) -> None: + super().__init__(ErrorCode.NamespaceDoesNotExist, + f"\"{pathname}\" does not exists, you need to create the resource first", additional_details) +# TODO: properly specify this exception type class SentenceLengthOverflowException(Exception): def __init__(self, sentence_len: int) -> None: super().__init__("Sentence length is {len} which exceededs limit".format(len=sentence_len)) diff --git a/memas/storage_driver/memas_metadata.py b/memas/storage_driver/memas_metadata.py index 3aca0d8..b74c2a8 100644 --- a/memas/storage_driver/memas_metadata.py +++ b/memas/storage_driver/memas_metadata.py @@ -4,10 +4,10 @@ import uuid from cassandra.cqlengine import columns, management from cassandra.cqlengine.models import Model -from cassandra.cqlengine.query import BatchQuery, LWTException +from cassandra.cqlengine.query import BatchQuery, DoesNotExist, LWTException from memas.interface import corpus from memas.interface.corpus import CorpusType -from memas.interface.exceptions import BadArgumentException, IllegalNameException, NamespaceExistsException +from memas.interface.exceptions import IllegalNameException, NamespaceDoesNotExistException, NamespaceExistsException from memas.interface.namespace import ROOT_ID, ROOT_NAME, NAMESPACE_SEPARATOR, CORPUS_SEPARATOR, is_pathname_format_valid, is_name_format_valid from memas.interface.storage_driver import MemasMetadataStore @@ -114,9 +114,14 @@ def first_init(self): self.init() def _get_id_by_name(self, fullname: str) -> uuid.UUID: + # the root user currently only exists logically if fullname == ROOT_NAME: return ROOT_ID - result = NamespaceNameToId.get(fullname=fullname) + + try: + result = NamespaceNameToId.get(fullname=fullname) + except DoesNotExist as e: + raise NamespaceDoesNotExistException(fullname) from e return result.id def _get_ids_by_name(self, fullname: str) -> tuple[uuid.UUID, uuid.UUID]: @@ -125,20 +130,15 @@ def _get_ids_by_name(self, fullname: str) -> tuple[uuid.UUID, uuid.UUID]: else: parent_pathname, child_name = split_namespace_pathname(fullname) - # if parent is root - if parent_pathname == ROOT_NAME: - child_id = NamespaceNameToId.get(fullname=fullname).id - return (ROOT_ID, child_id) - - child_result = NamespaceNameToId.get(fullname=fullname) - parent_result = NamespaceNameToId.get(fullname=parent_pathname) - return (parent_result.id, child_result.id) + child_id = self._get_id_by_name(fullname=fullname) + parent_id = self._get_id_by_name(fullname=parent_pathname) + return (parent_id, child_id) def create_namespace(self, namespace_pathname: str, *, parent_id: uuid.UUID = None) -> uuid.UUID: _log.debug(f"Creating namespace for [namespace_pathname=\"{namespace_pathname}\"]") if namespace_pathname == ROOT_NAME: - raise BadArgumentException("\"\" is reserved for the root namespace!") + raise NamespaceExistsException(namespace_pathname, "\"\" is reserved for the root namespace!") if not is_pathname_format_valid(namespace_pathname): raise IllegalNameException(namespace_pathname) diff --git a/tests/interface/test_exceptions.py b/tests/interface/test_exceptions.py new file mode 100644 index 0000000..5bac114 --- /dev/null +++ b/tests/interface/test_exceptions.py @@ -0,0 +1,15 @@ +import pytest +from memas.interface.exceptions import ErrorCode, MemasException + + +def test_memas_exception_output_object(): + e = MemasException(ErrorCode.NamespaceExists, "test message", additional_details="test details") + + assert e.return_obj() == {"error_code": ErrorCode.NamespaceExists.value, + "msg": "test message", "details": "test details"} + + +def test_memas_exception_details(): + e = MemasException(ErrorCode.NamespaceExists, "test message") + + assert "details" not in e.return_obj() diff --git a/tests/storage_driver/test_memas_metadata.py b/tests/storage_driver/test_memas_metadata.py index 2dea87b..4820475 100644 --- a/tests/storage_driver/test_memas_metadata.py +++ b/tests/storage_driver/test_memas_metadata.py @@ -1,5 +1,11 @@ +import pytest +from unittest import mock +from cassandra.cqlengine.query import DoesNotExist +from memas.interface.exceptions import NamespaceDoesNotExistException +from memas.interface.namespace import ROOT_ID from memas.storage_driver.memas_metadata import split_corpus_pathname, split_namespace_pathname +from memas.storage_driver.memas_metadata import MemasMetadataStoreImpl def test_split_corpus_pathname(): @@ -8,3 +14,17 @@ def test_split_corpus_pathname(): def test_split_namespace_pathname(): assert split_namespace_pathname("namespace.user.bot") == ("namespace.user", "bot") + + +def test_get_id_by_name_root(): + store = MemasMetadataStoreImpl() + assert ROOT_ID == store._get_id_by_name("") + + +@mock.patch('memas.storage_driver.memas_metadata.NamespaceNameToId') +def test_get_id_by_name_doesnt_exist(name_to_id): + name_to_id.get.side_effect = DoesNotExist() + + store = MemasMetadataStoreImpl() + with pytest.raises(NamespaceDoesNotExistException): + store._get_id_by_name("XD")