Skip to content
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

Investigate failed response object not sending correct 500 #206

Open
jcadam14 opened this issue Aug 5, 2024 · 1 comment
Open

Investigate failed response object not sending correct 500 #206

jcadam14 opened this issue Aug 5, 2024 · 1 comment

Comments

@jcadam14
Copy link
Contributor

jcadam14 commented Aug 5, 2024

When testing new fi3 data inserted into the database manually, an incorrect value of -1 instead of NULL was put into the FI table for tax_id and rssd_id. This caused the object to be sent back from endpoints to fail data validation. The response to the client didn't have the message, it was a server 500 that also ended up getting wrapped in a CORS error.

@michaeljwood
Copy link
Contributor

I'm not sure what all, if anything, changed since this issue was posted, but I am getting a response I would expect in this situation, just maybe not terribly helpful. e.g.

HTTP/1.1 500 Internal Server Error
date: Wed, 22 Jan 2025 20:15:13 GMT
server: uvicorn
content-length: 68
content-type: application/json

{
  "error_name": "Internal Server Error",
  "error_detail": "server error"
}

The logs do provide the actual failure, but that doesn't really help the client out.

I can think of a couple ways to handle this, so I'm curious to get some other thoughts on it. My initial thought is that in practice the data should be created through the API, so data validation will be caught in those initial requests, and never make it into the database. So, I'm thinking it might be nice to add a new exception handler for the ResponseValidationErrors similar to the existing handler for RequestValidationErrors to make that response a little nicer (still with a 500 response code most likely).
Further, I am thinking it might be a good idea to create a FastAPI wrapper similar to the RouterWrapper that will install all the existing (and this new) exception handlers so that they don't have to be specified in each application.

This would result in a response like the following:

HTTP/1.1 500 Internal Server Error
date: Wed, 22 Jan 2025 20:17:27 GMT
server: uvicorn
content-length: 406
content-type: application/json

{
  "error_name": "Response Validation Failure",
  "error_detail": "[{'type': 'value_error', 'loc': ('response',), 'msg': 'Value error, Invalid tax_id -1. Must be a valid TIN in the format of 12-3456789.', 'input': <regtech_user_fi_management.entities.models.dao.FinancialInstitutionDao object at 0x111826e40>, 'ctx': {'error': ValueError('Invalid tax_id -1. Must be a valid TIN in the format of 12-3456789.')}}]"
}

Seems like it might not be a bad idea to try to trim that message down a bit though.

Another possible solution (either instead of, or in addition to the above) would be to add a check constraint to the column(s) that are at issue so that the invalid data is rejected at the database level, ensuring it could never get in this state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants