From fb9c867391e97edf14480052818f2a87b32c4513 Mon Sep 17 00:00:00 2001 From: jcadam14 <41971533+jcadam14@users.noreply.github.com> Date: Thu, 12 Dec 2024 07:57:44 -0700 Subject: [PATCH] Added filing validators class, rearranged code (#530) Closes #529 --- pyproject.toml | 3 +- src/.env.local | 3 +- src/sbl_filing_api/config.py | 14 ++- src/sbl_filing_api/routers/filing.py | 74 ++++++------ .../services/request_action_validator.py | 105 +++--------------- .../services/validators/__init__.py | 1 + .../services/validators/base_validator.py | 40 +++++++ .../services/validators/filing_validators.py | 42 +++++++ .../validators/institution_validators.py | 31 ++++++ .../services/validators/period_validators.py | 15 +++ .../validators/submission_validators.py | 22 ++++ tests/api/routers/test_filing_api.py | 11 +- .../services/test_request_action_validator.py | 33 +++--- 13 files changed, 232 insertions(+), 162 deletions(-) create mode 100644 src/sbl_filing_api/services/validators/__init__.py create mode 100644 src/sbl_filing_api/services/validators/base_validator.py create mode 100644 src/sbl_filing_api/services/validators/filing_validators.py create mode 100644 src/sbl_filing_api/services/validators/institution_validators.py create mode 100644 src/sbl_filing_api/services/validators/period_validators.py create mode 100644 src/sbl_filing_api/services/validators/submission_validators.py diff --git a/pyproject.toml b/pyproject.toml index 3fae18c3..26ea194a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -79,7 +79,8 @@ env = [ "FS_DOWNLOAD_CONFIG__PROTOCOL=file", "ENV=TEST", "MAIL_API_URL=http://mail-api:8765/internal/confirmation/send", - 'REQUEST_VALIDATORS__SIGN_AND_SUBMIT=["check_lei_status","check_lei_tin","check_filing_exists","check_sub_accepted","check_voluntary_filer","check_contact_info"]' + 'REQUEST_VALIDATORS__SIGN_AND_SUBMIT=["valid_lei_status","valid_lei_tin","valid_filing_exists","valid_sub_accepted","valid_voluntary_filer","valid_contact_info"]', + 'REQUEST_VALIDATORS__FILING_CREATE=["valid_period_exists", "valid_no_filing_exists"]' ] testpaths = ["tests"] diff --git a/src/.env.local b/src/.env.local index 0d7c13fe..fd070a49 100644 --- a/src/.env.local +++ b/src/.env.local @@ -21,4 +21,5 @@ FS_UPLOAD_CONFIG__ROOT="../upload" EXPIRED_SUBMISSION_CHECK_SECS=120 SERVER_CONFIG__RELOAD="true" MAIL_API_URL=http://mail-api:8765/internal/confirmation/send -REQUEST_VALIDATORS__SIGN_AND_SUBMIT=["check_lei_status","check_lei_tin","check_filing_exists","check_sub_accepted","check_voluntary_filer","check_contact_info"] \ No newline at end of file +REQUEST_VALIDATORS__SIGN_AND_SUBMIT=["valid_lei_status","valid_lei_tin","valid_filing_exists","valid_sub_accepted","valid_voluntary_filer","valid_contact_info"] +REQUEST_VALIDATORS__FILING_CREATE=["valid_period_exists", "valid_no_filing_exists"] \ No newline at end of file diff --git a/src/sbl_filing_api/config.py b/src/sbl_filing_api/config.py index b5e85e61..0309de73 100644 --- a/src/sbl_filing_api/config.py +++ b/src/sbl_filing_api/config.py @@ -85,14 +85,16 @@ def build_postgres_dsn(cls, postgres_dsn, info: ValidationInfo) -> Any: class RequestActionValidations(BaseSettings): sign_and_submit: Set[str] = { - "check_lei_status", - "check_lei_tin", - "check_filing_exists", - "check_sub_accepted", - "check_voluntary_filer", - "check_contact_info", + "valid_lei_status", + "valid_lei_tin", + "valid_filing_exists", + "valid_sub_accepted", + "valid_voluntary_filer", + "valid_contact_info", } + filing_create: Set[str] = {"valid_period_exists", "valid_no_filing_exists"} + model_config = SettingsConfigDict(env_prefix="request_validators__", env_file=env_files_to_load, extra="allow") diff --git a/src/sbl_filing_api/routers/filing.py b/src/sbl_filing_api/routers/filing.py index 7a3adf14..8e954705 100644 --- a/src/sbl_filing_api/routers/filing.py +++ b/src/sbl_filing_api/routers/filing.py @@ -76,52 +76,42 @@ async def get_filings(request: Request, period_code: str): return await repo.get_filings(request.state.db_session, user.institutions, period_code) -@router.post("/institutions/{lei}/filings/{period_code}", response_model=FilingDTO) +@router.post( + "/institutions/{lei}/filings/{period_code}", + response_model=FilingDTO, + dependencies=[ + Depends(set_context({UserActionContext.PERIOD, UserActionContext.FILING})), + Depends(validate_user_action(request_action_validations.filing_create, "Filing Create Forbidden")), + ], +) @requires("authenticated") async def post_filing(request: Request, lei: str, period_code: str): - period = await repo.get_filing_period(request.state.db_session, filing_period=period_code) - - if period: - filing = await repo.get_filing(request.state.db_session, lei, period_code) - if filing: - raise RegTechHttpException( - status_code=status.HTTP_409_CONFLICT, - name="Filing Creation Conflict", - detail=f"Filing already exists for Filing Period {period_code} and LEI {lei}", - ) - creator = None - try: - creator = await repo.add_user_action( - request.state.db_session, - UserActionDTO( - user_id=request.user.id, - user_name=request.user.name, - user_email=request.user.email, - action_type=UserActionType.CREATE, - ), - ) - except Exception: - logger.exception("Error while trying to create the filing.creator UserAction.") - raise RegTechHttpException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - name="Filing.creator UserAction error", - detail="Error while trying to create the filing.creator UserAction.", - ) - - try: - return await repo.create_new_filing(request.state.db_session, lei, period_code, creator_id=creator.id) - except Exception: - raise RegTechHttpException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - name="Filing Creation Error", - detail=f"An error occurred while creating a filing for LEI {lei} and Filing Period {period_code}.", - ) + creator = None + try: + creator = await repo.add_user_action( + request.state.db_session, + UserActionDTO( + user_id=request.user.id, + user_name=request.user.name, + user_email=request.user.email, + action_type=UserActionType.CREATE, + ), + ) + except Exception: + logger.exception("Error while trying to create the filing.creator UserAction.") + raise RegTechHttpException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + name="Filing.creator UserAction error", + detail="Error while trying to create the filing.creator UserAction.", + ) - else: + try: + return await repo.create_new_filing(request.state.db_session, lei, period_code, creator_id=creator.id) + except Exception: raise RegTechHttpException( - status_code=status.HTTP_404_NOT_FOUND, - name="Filing Period Not Found", - detail=f"The period ({period_code}) does not exist, therefore a Filing can not be created for this period.", + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + name="Filing Creation Error", + detail=f"An error occurred while creating a filing for LEI {lei} and Filing Period {period_code}.", ) diff --git a/src/sbl_filing_api/services/request_action_validator.py b/src/sbl_filing_api/services/request_action_validator.py index c56b9658..2723e032 100644 --- a/src/sbl_filing_api/services/request_action_validator.py +++ b/src/sbl_filing_api/services/request_action_validator.py @@ -1,20 +1,18 @@ import inspect -import json import logging -from abc import ABC, abstractmethod -from enum import StrEnum -from http import HTTPStatus -from typing import Any, Dict, List, Set - import httpx + from async_lru import alru_cache +from enum import StrEnum from fastapi import Request, status +from http import HTTPStatus +from typing import Set + from regtech_api_commons.api.exceptions import RegTechHttpException from sbl_filing_api.config import settings -from sbl_filing_api.entities.models.dao import FilingDAO, SubmissionDAO -from sbl_filing_api.entities.models.model_enums import SubmissionState from sbl_filing_api.entities.repos import submission_repo as repo +from sbl_filing_api.services.validators.base_validator import get_validation_registry log = logging.getLogger(__name__) @@ -22,6 +20,7 @@ class UserActionContext(StrEnum): FILING = "filing" INSTITUTION = "institution" + PERIOD = "period" class FiRequest: @@ -63,89 +62,6 @@ async def get_institution_data(fi_request: FiRequest): get_institution_data.cache_invalidate(fi_request) -class ActionValidator(ABC): - """ - Abstract Callable class for action validations, __subclasses__ method leveraged to construct a registry - """ - - name: str - - def __init__(self, name: str): - super().__init__() - self.name = name - - @abstractmethod - def __call__(self, *args, **kwargs): ... - - -class CheckLeiStatus(ActionValidator): - def __init__(self): - super().__init__("check_lei_status") - - def __call__(self, institution: Dict[str, Any], **kwargs): - try: - is_active = institution["lei_status"]["can_file"] - if not is_active: - return f"Cannot sign filing. LEI status of {institution['lei_status_code']} cannot file." - except Exception: - log.exception("Unable to determine lei status: %s", json.dumps(institution)) - return "Unable to determine LEI status." - - -class CheckLeiTin(ActionValidator): - def __init__(self): - super().__init__("check_lei_tin") - - def __call__(self, institution: Dict[str, Any], **kwargs): - if not (institution and institution.get("tax_id")): - return "Cannot sign filing. TIN is required to file." - - -class CheckFilingExists(ActionValidator): - def __init__(self): - super().__init__("check_filing_exists") - - def __call__(self, filing: FilingDAO, lei: str, period: str, **kwargs): - if not filing: - return f"There is no Filing for LEI {lei} in period {period}, unable to sign a non-existent Filing." - - -class CheckSubAccepted(ActionValidator): - def __init__(self): - super().__init__("check_sub_accepted") - - async def __call__(self, filing: FilingDAO, **kwargs): - if filing: - submissions: List[SubmissionDAO] = await filing.awaitable_attrs.submissions - if not len(submissions) or submissions[0].state != SubmissionState.SUBMISSION_ACCEPTED: - filing.lei - filing.filing_period - return f"Cannot sign filing. Filing for {filing.lei} for period {filing.filing_period} does not have a latest submission in the SUBMISSION_ACCEPTED state." - - -class CheckVoluntaryFiler(ActionValidator): - def __init__(self): - super().__init__("check_voluntary_filer") - - def __call__(self, filing: FilingDAO, **kwargs): - if filing and filing.is_voluntary is None: - return f"Cannot sign filing. Filing for {filing.lei} for period {filing.filing_period} does not have a selection of is_voluntary defined." - - -class CheckContactInfo(ActionValidator): - def __init__(self): - super().__init__("check_contact_info") - - def __call__(self, filing: FilingDAO, **kwargs): - if filing and not filing.contact_info: - return f"Cannot sign filing. Filing for {filing.lei} for period {filing.filing_period} does not have contact info defined." - - -validation_registry = { - validator.name: validator for validator in {Validator() for Validator in ActionValidator.__subclasses__()} -} - - def set_context(requirements: Set[UserActionContext]): """ Sets a `context` object on `request.state`; this should typically include the institution, and filing; @@ -159,9 +75,13 @@ def set_context(requirements: Set[UserActionContext]): async def _set_context(request: Request): lei = request.path_params.get("lei") period = request.path_params.get("period_code") - context = {"lei": lei, "period": period} + context = {"lei": lei, "period_code": period} if lei and UserActionContext.INSTITUTION in requirements: context = context | {UserActionContext.INSTITUTION: await get_institution_data(FiRequest(request, lei))} + if period and UserActionContext.PERIOD in requirements: + context = context | { + UserActionContext.PERIOD: await repo.get_filing_period(request.state.db_session, period) + } if period and UserActionContext.FILING in requirements: context = context | {UserActionContext.FILING: await repo.get_filing(request.state.db_session, lei, period)} request.state.context = context @@ -181,6 +101,7 @@ def validate_user_action(validator_names: Set[str], exception_name: str): async def _run_validations(request: Request): res = [] + validation_registry = get_validation_registry() for validator_name in validator_names: validator = validation_registry.get(validator_name) if not validator: diff --git a/src/sbl_filing_api/services/validators/__init__.py b/src/sbl_filing_api/services/validators/__init__.py new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/src/sbl_filing_api/services/validators/__init__.py @@ -0,0 +1 @@ + diff --git a/src/sbl_filing_api/services/validators/base_validator.py b/src/sbl_filing_api/services/validators/base_validator.py new file mode 100644 index 00000000..9f7900e4 --- /dev/null +++ b/src/sbl_filing_api/services/validators/base_validator.py @@ -0,0 +1,40 @@ +import pkgutil +import importlib + +from abc import ABC, abstractmethod + + +class ActionValidator(ABC): + """ + Abstract Callable class for action validations, __subclasses__ method leveraged to construct a registry + """ + + name: str + + def __init__(self, name: str): + super().__init__() + self.name = name + + @abstractmethod + def __call__(self, *args, **kwargs): ... + + +_validation_registry = None + + +def get_validation_registry(): + # use package reflection to import all subclasses of the base ActionValidator under the current + # package so that __subclasses__ can find loaded subs. Do this once to keep subsequent requests + # from being impacted in performance. + global _validation_registry + if not _validation_registry: + package = __package__ + p = importlib.import_module(package) + for _, module_name, is_pkg in pkgutil.iter_modules(p.__path__): + if module_name not in __name__: + importlib.import_module(f"{package}.{module_name}") + _validation_registry = { + validator.name: validator for validator in {Validator() for Validator in ActionValidator.__subclasses__()} + } + + return _validation_registry diff --git a/src/sbl_filing_api/services/validators/filing_validators.py b/src/sbl_filing_api/services/validators/filing_validators.py new file mode 100644 index 00000000..f88d66fe --- /dev/null +++ b/src/sbl_filing_api/services/validators/filing_validators.py @@ -0,0 +1,42 @@ +import logging + +from sbl_filing_api.entities.models.dao import FilingDAO +from .base_validator import ActionValidator + +log = logging.getLogger(__name__) + + +class ValidNoFilingExists(ActionValidator): + def __init__(self): + super().__init__("valid_no_filing_exists") + + def __call__(self, filing: FilingDAO, period_code: str, lei: str, **kwargs): + if filing: + return f"Filing already exists for Filing Period {period_code} and LEI {lei}" + + +class ValidFilingExists(ActionValidator): + def __init__(self): + super().__init__("valid_filing_exists") + + def __call__(self, filing: FilingDAO, lei: str, period_code: str, **kwargs): + if not filing: + return f"There is no Filing for LEI {lei} in period {period_code}, unable to sign a non-existent Filing." + + +class ValidVoluntaryFiler(ActionValidator): + def __init__(self): + super().__init__("valid_voluntary_filer") + + def __call__(self, filing: FilingDAO, **kwargs): + if filing and filing.is_voluntary is None: + return f"Cannot sign filing. Filing for {filing.lei} for period {filing.filing_period} does not have a selection of is_voluntary defined." + + +class ValidContactInfo(ActionValidator): + def __init__(self): + super().__init__("valid_contact_info") + + def __call__(self, filing: FilingDAO, **kwargs): + if filing and not filing.contact_info: + return f"Cannot sign filing. Filing for {filing.lei} for period {filing.filing_period} does not have contact info defined." diff --git a/src/sbl_filing_api/services/validators/institution_validators.py b/src/sbl_filing_api/services/validators/institution_validators.py new file mode 100644 index 00000000..038d3868 --- /dev/null +++ b/src/sbl_filing_api/services/validators/institution_validators.py @@ -0,0 +1,31 @@ +import json +import logging + +from typing import Any, Dict + +from .base_validator import ActionValidator + +log = logging.getLogger(__name__) + + +class ValidLeiStatus(ActionValidator): + def __init__(self): + super().__init__("valid_lei_status") + + def __call__(self, institution: Dict[str, Any], **kwargs): + try: + is_active = institution["lei_status"]["can_file"] + if not is_active: + return f"Cannot sign filing. LEI status of {institution['lei_status_code']} cannot file." + except Exception: + log.exception("Unable to determine lei status: %s", json.dumps(institution)) + return "Unable to determine LEI status." + + +class ValidLeiTin(ActionValidator): + def __init__(self): + super().__init__("valid_lei_tin") + + def __call__(self, institution: Dict[str, Any], **kwargs): + if not (institution and institution.get("tax_id")): + return "Cannot sign filing. TIN is required to file." diff --git a/src/sbl_filing_api/services/validators/period_validators.py b/src/sbl_filing_api/services/validators/period_validators.py new file mode 100644 index 00000000..94d27991 --- /dev/null +++ b/src/sbl_filing_api/services/validators/period_validators.py @@ -0,0 +1,15 @@ +import logging + +from sbl_filing_api.entities.models.dao import FilingPeriodDAO +from .base_validator import ActionValidator + +log = logging.getLogger(__name__) + + +class ValidPeriodExists(ActionValidator): + def __init__(self): + super().__init__("valid_period_exists") + + def __call__(self, period: FilingPeriodDAO, period_code: str, **kwargs): + if not period: + return f"The period ({period_code}) does not exist, therefore a Filing can not be created for this period." diff --git a/src/sbl_filing_api/services/validators/submission_validators.py b/src/sbl_filing_api/services/validators/submission_validators.py new file mode 100644 index 00000000..c0ed055a --- /dev/null +++ b/src/sbl_filing_api/services/validators/submission_validators.py @@ -0,0 +1,22 @@ +import logging + +from typing import List + +from sbl_filing_api.entities.models.dao import FilingDAO, SubmissionDAO +from sbl_filing_api.entities.models.model_enums import SubmissionState +from .base_validator import ActionValidator + +log = logging.getLogger(__name__) + + +class ValidSubAccepted(ActionValidator): + def __init__(self): + super().__init__("valid_sub_accepted") + + async def __call__(self, filing: FilingDAO, **kwargs): + if filing: + submissions: List[SubmissionDAO] = await filing.awaitable_attrs.submissions + if not len(submissions) or submissions[0].state != SubmissionState.SUBMISSION_ACCEPTED: + filing.lei + filing.filing_period + return f"Cannot sign filing. Filing for {filing.lei} for period {filing.filing_period} does not have a latest submission in the SUBMISSION_ACCEPTED state." diff --git a/tests/api/routers/test_filing_api.py b/tests/api/routers/test_filing_api.py index aaa89a97..4f298b19 100644 --- a/tests/api/routers/test_filing_api.py +++ b/tests/api/routers/test_filing_api.py @@ -100,17 +100,20 @@ def test_post_filing( # Filing already exists res = client.post("/v1/filing/institutions/1234567890ZXWVUTSR00/filings/2024/") - assert res.status_code == 409 - assert res.json()["error_detail"] == "Filing already exists for Filing Period 2024 and LEI 1234567890ZXWVUTSR00" + assert res.status_code == 403 + assert ( + res.json()["error_detail"] + == "['Filing already exists for Filing Period 2024 and LEI 1234567890ZXWVUTSR00']" + ) # testing with a period that does not exist get_filing_mock.return_value = None get_filing_period_by_code_mock.return_value = None res = client.post("/v1/filing/institutions/1234567890ZXWVUTSR00/filings/2025/") - assert res.status_code == 404 + assert res.status_code == 403 assert ( res.json()["error_detail"] - == "The period (2025) does not exist, therefore a Filing can not be created for this period." + == "['The period (2025) does not exist, therefore a Filing can not be created for this period.']" ) get_filing_period_by_code_mock.return_value = get_filing_period_mock.return_value diff --git a/tests/services/test_request_action_validator.py b/tests/services/test_request_action_validator.py index 41cd8d0d..ae3505ab 100644 --- a/tests/services/test_request_action_validator.py +++ b/tests/services/test_request_action_validator.py @@ -2,6 +2,7 @@ from logging import Logger import pytest + from fastapi import Request from pytest_mock import MockerFixture from regtech_api_commons.api.exceptions import RegTechHttpException @@ -55,7 +56,7 @@ def request_mock_valid_context(mocker: MockerFixture, request_mock: Request, fil request_mock.state.context = { "lei": "1234567890ABCDEFGH00", - "period": "2024", + "period_code": "2024", UserActionContext.INSTITUTION: { "tax_id": "12-3456789", "lei_status_code": "ISSUED", @@ -70,7 +71,7 @@ def request_mock_valid_context(mocker: MockerFixture, request_mock: Request, fil def request_mock_invalid_context(mocker: MockerFixture, request_mock: Request, filing_mock: FilingDAO) -> Request: request_mock.state.context = { "lei": "1234567890ABCDEFGH00", - "period": "2024", + "period_code": "2024", UserActionContext.INSTITUTION: { "lei_status_code": "LAPSED", "lei_status": {"name": "Lapsed", "code": "LAPSED", "can_file": False}, @@ -88,12 +89,12 @@ def log_mock(mocker: MockerFixture) -> Logger: async def test_validations_with_errors(request_mock_invalid_context: Request): run_validations = validate_user_action( { - "check_lei_status", - "check_lei_tin", - "check_filing_exists", - "check_sub_accepted", - "check_voluntary_filer", - "check_contact_info", + "valid_lei_status", + "valid_lei_tin", + "valid_filing_not_exists", + "valid_sub_accepted", + "valid_voluntary_filer", + "valid_contact_info", }, "Test Exception", ) @@ -120,12 +121,12 @@ async def test_validations_with_errors(request_mock_invalid_context: Request): async def test_validations_no_errors(request_mock_valid_context: Request): run_validations = validate_user_action( { - "check_lei_status", - "check_lei_tin", - "check_filing_exists", - "check_sub_accepted", - "check_voluntary_filer", - "check_contact_info", + "valid_lei_status", + "valid_lei_tin", + "valid_filing_exists", + "valid_sub_accepted", + "valid_voluntary_filer", + "valid_contact_info", }, "Test Exception", ) @@ -133,7 +134,7 @@ async def test_validations_no_errors(request_mock_valid_context: Request): async def test_lei_status_bad_api_res(request_mock: Request, httpx_unauthed_mock): - run_validations = validate_user_action({"check_lei_status"}, "Test Exception") + run_validations = validate_user_action({"valid_lei_status"}, "Test Exception") context_setter = set_context({UserActionContext.INSTITUTION}) await context_setter(request_mock) @@ -143,7 +144,7 @@ async def test_lei_status_bad_api_res(request_mock: Request, httpx_unauthed_mock async def test_lei_status_good_api_res(request_mock: Request, httpx_authed_mock): - run_validations = validate_user_action({"check_lei_status"}, "Test Exception") + run_validations = validate_user_action({"valid_lei_status"}, "Test Exception") context_setter = set_context({UserActionContext.INSTITUTION}) await context_setter(request_mock) with pytest.raises(RegTechHttpException) as e: