Report user errors in filter as HTTP 400 Bad Request and not 501 Not Implemented #658
Conversation
Codecov Report
@@ Coverage Diff @@
## master #658 +/- ##
==========================================
- Coverage 92.81% 92.79% -0.02%
==========================================
Files 67 67
Lines 3520 3526 +6
==========================================
+ Hits 3267 3272 +5
- Misses 253 254 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I don't see any issues with this, thanks @markus1978 !
I only have some comments/questions:
Instead of bypassing the rest of the function, I guess you could just one could simply utilize orig_exc attributes for status and title below as well, falling back to the current values?
What do you think?
This should make it more flexible when it comes to introducing other exceptions in the future for the Lark grammar handlers.
Also, I wouldn't be averse to a test of this :) E.g. passing different Exceptions to the exception handler function and checking the outputs?
|
Ready for review in my eyes @ml-evs |
There was a problem hiding this comment.
This looks fine to me, thanks @markus1978 and @CasperWA.
@CasperWA: rebase/merge as you wish after updating the branch, you should be able to self-accept and merge as its not your PR.
- Return correct str value for custom exceptions The StrReprMixin used for custom exceptions has its magic method __str__ updated to directly return `detail` if set, else return __repr__. - Remove use of general Exception in elasticsearch. - Update doc strings for exception handlers.
There are different causes for filtertransformer failure that need to be treated differently. Mostly its about raising NotImplementedError (-> 501 Not Implemented) if a certain filter language feature is not supported, but there is also the case where the user causes static semantic errors, e.g. using wrong property names, using operators on properties where they are not allowed (-> 400 Bad Request).
I saw that the mongo transformer was raising a
optimade.server.exception.BadRequestto indicate a user error and then was copying this for the elastictransformer. However, all exceptions during a lark tree visit are wrapped in aVisitErrorand the exception handler (optimade.server.exception_handlers.grammar_not_implemented_handler) is handling all these errors with a 501/Not implemented.This PR adds a special treatment for
BadRequestwrapped in aVisitError.