fix #1733: remove unnecessary exception re-throwing #1734
Conversation
| $request->getQueryParams() | ||
| ); | ||
| } catch (\Exception $e) { | ||
| throw new SQLException($e->getMessage()); |
There was a problem hiding this comment.
I believe some of these changes are a little too loose. This is the only place this exception is transformed from a generic Exception to an instance of SQLException for example, which in turn controls the error code provided in the API. Could you verify @dur41d ?
There was a problem hiding this comment.
@rijkvanzanten I found that exceptions are being handled differently in different places. Sometimes it's re-thrown as is and sometimes it being converted into a different exception type. With the lack of convention i think letting the exception bubble up will simplify code and debugging as the stack-trace is not circumvented. The API convent convert any unhanded exception to error code 500 or whatever as it sees fit and in one place.
There was a problem hiding this comment.
Yeah I agree, but changing that right now means that the error codes that are returned will change, which is technically a breaking change....
Refactoring all the errors to be properly thrown, and then handled by bubbling up is probably too much effort right now, but would you be able to leave the "double catch" for the ones that do the exception mapping to different types? At least that way it isn't a breaking change 🙂
|
@dur41d would you be able to leave the "double catch" for the ones that do the exception mapping to different types? At least that way it isn't a breaking change 🙂 |
fixing #1733