-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: incorporate api commons exception handlers #175
Changes from 1 commit
de1df71
b662bdb
948b6c7
0ff3cd1
9dc628e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,7 @@ | ||
import logging | ||
|
||
from fastapi import Depends, Request, UploadFile, BackgroundTasks, status, HTTPException | ||
from fastapi import Depends, Request, UploadFile, BackgroundTasks, status | ||
from fastapi.responses import JSONResponse, FileResponse | ||
from regtech_api_commons.api.router_wrapper import Router | ||
from regtech_api_commons.api.exceptions import RegTechHttpException | ||
from sbl_filing_api.entities.models.model_enums import UserActionType | ||
from sbl_filing_api.services import submission_processor | ||
from typing import Annotated, List | ||
|
@@ -33,7 +32,6 @@ async def set_db(request: Request, session: Annotated[AsyncSession, Depends(get_ | |
|
||
|
||
router = Router(dependencies=[Depends(set_db), Depends(verify_user_lei_relation)]) | ||
logger = logging.getLogger(__name__) | ||
|
||
|
||
@router.get("/periods", response_model=List[FilingPeriodDTO]) | ||
|
@@ -60,14 +58,16 @@ async def post_filing(request: Request, lei: str, period_code: str): | |
try: | ||
return await repo.create_new_filing(request.state.db_session, lei, period_code) | ||
except IntegrityError: | ||
raise HTTPException( | ||
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}", | ||
) | ||
else: | ||
return JSONResponse( | ||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
content=f"The period ({period_code}) does not exist, therefore a Filing can not be created for this period.", | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_404_NOT_FOUND, | ||
name="Filing Period Not Found", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @billhimmelsbach @shindigira @meissadia not sure how far along front end is with the status code; is it ok to switch most of the 422s in the filing API which are filing doesn't exist, submission doesn't exist, or filing period doesn't exist to 404s? It makes more sense in my mind; since in this example a filing period doesn't exist, therefore a not found error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am going to take your updates here and build a wiki page with response codes based on when and we can discuss those during devchat? |
||
detail=f"The period ({period_code}) does not exist, therefore a Filing can not be created for this period.", | ||
) | ||
|
||
|
||
|
@@ -76,20 +76,23 @@ async def post_filing(request: Request, lei: str, period_code: str): | |
async def sign_filing(request: Request, lei: str, period_code: str): | ||
filing = await repo.get_filing(request.state.db_session, lei, period_code) | ||
if not filing: | ||
return JSONResponse( | ||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
content=f"There is no Filing for LEI {lei} in period {period_code}, unable to sign a non-existent Filing.", | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_404_NOT_FOUND, | ||
name="Filing Not Found", | ||
detail=f"There is no Filing for LEI {lei} in period {period_code}, unable to sign a non-existent Filing.", | ||
) | ||
latest_sub = await repo.get_latest_submission(request.state.db_session, lei, period_code) | ||
if not latest_sub or latest_sub.state != SubmissionState.SUBMISSION_ACCEPTED: | ||
return JSONResponse( | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_403_FORBIDDEN, | ||
content=f"Cannot sign filing. Filing for {lei} for period {period_code} does not have a latest submission the SUBMISSION_ACCEPTED state.", | ||
name="Filing Action Forbidden", | ||
detail=f"Cannot sign filing. Filing for {lei} for period {period_code} does not have a latest submission the SUBMISSION_ACCEPTED state.", | ||
) | ||
if not filing.contact_info: | ||
return JSONResponse( | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_403_FORBIDDEN, | ||
content=f"Cannot sign filing. Filing for {lei} for period {period_code} does not have contact info defined.", | ||
name="Filing Action Forbidden", | ||
detail=f"Cannot sign filing. Filing for {lei} for period {period_code} does not have contact info defined.", | ||
) | ||
""" | ||
if not filing.institution_snapshot_id: | ||
|
@@ -121,9 +124,10 @@ async def upload_file( | |
|
||
filing = await repo.get_filing(request.state.db_session, lei, period_code) | ||
if not filing: | ||
return JSONResponse( | ||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
content=f"There is no Filing for LEI {lei} in period {period_code}, unable to submit file.", | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_404_NOT_FOUND, | ||
name="Filing Not Found", | ||
detail=f"There is no Filing for LEI {lei} in period {period_code}, unable to submit file.", | ||
) | ||
try: | ||
submitter = await repo.add_user_action( | ||
|
@@ -142,25 +146,23 @@ async def upload_file( | |
submission.state = SubmissionState.SUBMISSION_UPLOADED | ||
submission = await repo.update_submission(submission) | ||
except Exception as e: | ||
logger.error( | ||
f"Error while trying to process Submission {submission.id}", e, exec_info=True, stack_info=True | ||
) | ||
submission.state = SubmissionState.UPLOAD_FAILED | ||
submission = await repo.update_submission(submission) | ||
return JSONResponse( | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
content=f"{e}", | ||
) | ||
name="Submission Unprocessable", | ||
detail=f"Error while trying to process Submission {submission.id}", | ||
) from e | ||
background_tasks.add_task(submission_processor.validation_monitor, period_code, lei, submission, content) | ||
|
||
return submission | ||
|
||
except Exception as e: | ||
logger.error("Error while trying to process SUBMIT User Action", e, exec_info=True, stack_info=True) | ||
return JSONResponse( | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
content=f"{e}", | ||
) | ||
name="Submission Unprocessable", | ||
detail="Error while trying to process SUBMIT User Action", | ||
) from e | ||
|
||
|
||
@router.get("/institutions/{lei}/filings/{period_code}/submissions", response_model=List[SubmissionDTO]) | ||
|
@@ -192,17 +194,19 @@ async def get_submission(request: Request, id: int): | |
async def accept_submission(request: Request, id: int, lei: str, period_code: str): | ||
submission = await repo.get_submission(request.state.db_session, id) | ||
if not submission: | ||
return JSONResponse( | ||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
content=f"Submission ID {id} does not exist, cannot accept a non-existing submission.", | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_404_NOT_FOUND, | ||
name="Submission Not Found", | ||
detail=f"Submission ID {id} does not exist, cannot accept a non-existing submission.", | ||
) | ||
if ( | ||
submission.state != SubmissionState.VALIDATION_SUCCESSFUL | ||
and submission.state != SubmissionState.VALIDATION_WITH_WARNINGS | ||
): | ||
return JSONResponse( | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_403_FORBIDDEN, | ||
content=f"Submission {id} for LEI {lei} in filing period {period_code} is not in an acceptable state. Submissions must be validated successfully or with only warnings to be accepted.", | ||
name="Submission Action Forbidden", | ||
detail=f"Submission {id} for LEI {lei} in filing period {period_code} is not in an acceptable state. Submissions must be validated successfully or with only warnings to be accepted.", | ||
) | ||
|
||
accepter = await repo.add_user_action( | ||
|
@@ -226,9 +230,10 @@ async def put_institution_snapshot(request: Request, lei: str, period_code: str, | |
if result: | ||
result.institution_snapshot_id = update_value.institution_snapshot_id | ||
return await repo.upsert_filing(request.state.db_session, result) | ||
return JSONResponse( | ||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
content=f"A Filing for the LEI ({lei}) and period ({period_code}) that was attempted to be updated does not exist.", | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_404_NOT_FOUND, | ||
name="Filing Not Found", | ||
detail=f"A Filing for the LEI ({lei}) and period ({period_code}) that was attempted to be updated does not exist.", | ||
) | ||
|
||
|
||
|
@@ -253,9 +258,10 @@ async def put_contact_info(request: Request, lei: str, period_code: str, contact | |
result = await repo.get_filing(request.state.db_session, lei, period_code) | ||
if result: | ||
return await repo.update_contact_info(request.state.db_session, lei, period_code, contact_info) | ||
return JSONResponse( | ||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
content=f"A Filing for the LEI ({lei}) and period ({period_code}) that was attempted to be updated does not exist.", | ||
raise RegTechHttpException( | ||
status_code=status.HTTP_404_NOT_FOUND, | ||
name="Filing Not Found", | ||
detail=f"A Filing for the LEI ({lei}) and period ({period_code}) that was attempted to be updated does not exist.", | ||
) | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe more of a stylistic thing, but throwing an exception for non-200 responses, and let exception handlers deal with the rest looks better in my eyes; thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen both ways, and I don't have a preference but sticking with one way is ideal. I know there are a couple of places where we're raising and HttpException but others we're returning JSON responses. Philosophically non-200 responses are errors/exceptions/deviations from happy-path so raising versus returning makes a lot of sense. I say we go with that as a standard approach across the board.