CHE-1232. Add mapper for RuntimeException #1547
Conversation
|
do we have a test checking these runtime exceptions are correctly handle (so that a mapper does the job) ? |
| public class RuntimeExceptionMapper implements ExceptionMapper<RuntimeException> { | ||
| @Override | ||
| public Response toResponse(RuntimeException exception) { | ||
| String errorMessage = exception.getMessage(); |
There was a problem hiding this comment.
Findbug usually recommends usage of getlocalizedmessage instead of getMessage
|
@benoitf I suppose it is hard to test such thing because it require Everrest. And to make it really reliable it should launch whole Che server. |
|
@skabashnyuk Please take a look |
|
@garagatyi AFAIK EverrestJetty.class listener allow to test REST components and we already test some filters, REST Service, etc so I don't see the issue of being hard to be tested ? |
|
EverrestJetty can handle that I think. |
|
LGTM |
| public class RuntimeExceptionMapper implements ExceptionMapper<RuntimeException> { | ||
| @Override | ||
| public Response toResponse(RuntimeException exception) { | ||
| String errorMessage = exception.getLocalizedMessage(); |
There was a problem hiding this comment.
Maybe it would be better to log this error because we should not throw RuntimeException in services at all and usually it is possible in case of bug
@garagatyi @skabashnyuk @vparfonov WDYT?
There was a problem hiding this comment.
@sleshchenko @vparfonov Done!
|
Build # 983 - FAILED Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/983/ to view the results. |
|
I still don't see this server code being tested ? |
|
-1 to merge it without test |
|
@benoitf @skabashnyuk I don't merge it. I need to finish my current issue, so - don't worry - I will do it :-) |
| public Response toResponse(RuntimeException exception) { | ||
| String errorMessage = exception.getLocalizedMessage(); | ||
| if (!isNullOrEmpty(errorMessage)) { | ||
| LOG.error(errorMessage); |
There was a problem hiding this comment.
I would say that LOG.warn(errorMessage, exception); would be better. And put it before check
There was a problem hiding this comment.
I would say that LOG.error(errorMessage, exception)
There was a problem hiding this comment.
+1 for log error with stacktrace. RuntimeException here means bug in code, so we should findout how to fix that ASAP and stacktrace will definitely help
|
Build finished. |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/11/ |
Signed-off-by: Roman Nikitenko <rnikitenko@codenvy.com>
|
@benoitf FYI server code is tested |
|
LGTM |
| LOG.error("RuntimeException occurred", exception); | ||
| } | ||
|
|
||
| return Response.serverError().build(); |
There was a problem hiding this comment.
Mb it would be better to send ServiceError with default message to avoid writing client side's logic for expecting service error or empty body ?
|
Build # 73 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/73/ to view the results. |
|
I've updated pull request |
|
Build # 106 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/106/ to view the results. |
|
ok |
1 similar comment
|
ok |
|
Build # 131 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/131/ to view the results. |
|
@skabashnyuk @vparfonov @riuvshin Can we merge it now? |
RomanNikitenko
force-pushed
the
CHE-1232
branch
from
74094a0 to
aae97a6
Compare
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/376/ |
|
ok |
Signed-off-by: Roman Nikitenko <rnikitenko@codenvy.com>
The mapper will allow to get readable error messages on client side for RuntimeException
@vparfonov please review
Signed-off-by: Roman Nikitenko rnikitenko@codenvy.com