Adds logging for validation exceptions #53
Conversation
racheldaniel
requested review from
drewbanin,
cesher,
peter-bertuglia and
rviswanathan-dbt
| @@ -174,6 +175,14 @@ class SQLConfig(BaseModel): | |||
| sql: str | |||
|
|
|||
|
|
|||
| @app.exception_handler(RequestValidationError) | |||
| async def validation_exception_handler(request: Request, exc: RequestValidationError): | |||
There was a problem hiding this comment.
this is dope @racheldaniel!! Totally a question from a place of my own ignorance: why is this a 422 vs. a 400? Is it possible that the 422 response below could actually change the response code generated by the app, or would every validation error result in a 422?
There was a problem hiding this comment.
@drewbanin Thanks, and a great question!
My understanding is that the FastAPI RequestValidationError that we're catching would normally always yield a 422: Unprocessable Entity if left unhandled. This error class is an extension of the pydantic class ValidationError-- I think we're pretty safe to throw a 422 for these specific exceptions.
As far as 400 vs. 422-- I'm using the 422 only because that was what was being thrown organically. I would be more than fine throwing a 400-- we're not distinguishing between syntactic and semantic inaccuracies here, which is what the difference is supposed to be. FastAPI throws as 422 whether you leave a field out of the body or send the wrong data type, which seems odd to me.
I have zero preference here, so if it feels better to others to go ahead and make this a 400 I'm totally down. However there shouldn't be any issue with changing other errors to 422s as far as I'm aware!
There was a problem hiding this comment.
ok word! If the underlying exception would result in a 422, then i have no problem at all with generating our own exception instead! Thanks so much for the additional info :)
If a malformed request is sent to the dbt-server, we're currently unable to see any information other than the
422 Unprocessable Entityerror.I found this solution in a FastAPI github issue, and tested running the following curl command locally, omitting an expected field:
curl localhost:8585/push -H 'Content-Type: application/json' -d '{"state_id":"c88a8c249f7675b008a24ad18b4c33ec","body":{"file_1": {"contents":"value", "hash":""}}}'Before:
After:
