[CLN] make ValueError and TypeError present as InvalidArgumentError #3017
Conversation
HammadB
requested review from
codetheweb and
tazarov
and removed request for
codetheweb
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
| except ValueError as e: | ||
| return JSONResponse( | ||
| content={"error": "InvalidArgumentError", "message": str(e)}, | ||
| status_code=400, | ||
| ) | ||
| except TypeError as e: | ||
| return JSONResponse( | ||
| content={"error": "InvalidArgumentError", "message": str(e)}, | ||
| status_code=400, | ||
| ) |
There was a problem hiding this comment.
should we transform these error types to a ChromaError inside this handler and then allow the normal error flow to handle them?
There was a problem hiding this comment.
I thought about that, but it felt silly / not helpful to do so, I could go either way - and don't have a principled way of deciding since the change is localized - I.e we'd catch this type, turn it into a chroma error and rethrow.
If you have a way to decide lmk - I can change it.
There was a problem hiding this comment.
yeah current method seems fine
There was a problem hiding this comment.
I'd like to understand this a bit better; how do we know both of these are due to an InvalidArgumentError ? Why don't we throw a ChromaError that represents an InvalidArgumentError further down the stack?
There was a problem hiding this comment.
ValueError / TypeError
"A TypeError occurs when an operation or function is applied to an object of inappropriate type. A ValueError occurs when a built-in operation or function receives an argument that has the right type but an inappropriate value"
Map to chromas invalid argument error. Rather than replace ALL instances in our code of ValueError/TypeError I opted to catch them and transmit them to clients as an InvalidArgumentError.
There was a problem hiding this comment.
The global replace is probably the 'right' thing in the long run (and an easy starter issue); let's make an issue for that. Meantime this looks fine to land.
…3017) ## Description of changes *Summarize the changes made by this PR.* - Improvements & Bug fixes - This will propagate value/type errors as invalid argument errors - New functionality - None ## Test plan *How are these changes tested?* - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes None
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
None