RHDM-919: Improved error messages #238

Christopher-Chianelli · 2019-05-09T20:09:57Z

Modified the backend to return a JSON containing information about the exception (including an i18n Translation Key) when an exception occurs.

Fonifan

Besides minor issues, LGTM. Great job 👍!

...-gwtui/src/main/java/org/optaweb/employeerostering/gwtui/client/exception/RESTException.java

...rces/org/optaweb/employeerostering/gwtui/client/resources/i18n/OptaWebUIConstants.properties

rsynek

Please see my comments in line. Apart from those, there is no test coverage showing the exception handling works as expected.

rsynek · 2019-05-13T10:57:39Z

.../src/main/java/org/optaweb/employeerostering/server/common/jaxrs/OptaWebExceptionMapper.java

+        try {
+            return objectMapper.writeValueAsString(serverSideException);
+        } catch (JsonProcessingException e) {
+            return "{}";


The information about the exception in JSON serialization has not been propagated anywhere. I would expect to at least log the exception and inform the client that e.g. "There was an issue with retrieving the root cause, due to...more information can be found in a server log."

This is a difficult case: the client is expecting a ServerException JSON object. Trying to serialize the exception could cause another exception (example: objectMapper is null). So we either need to hard code a ServerException JSON object for this case (issue: fragile) or have some way to indicate the error to the client (so we don't try to deserialize the error message as a JSON object). Perhaps I can use different error codes to indicate different cases. WDYT?

My suggestion was to communicate towards the client that there was an issue, but its serialization failed too - find more information in server log. Basically providing as much information as possible, in a nice format.
Right now if the serialization fails, client gets just an empty JSON object and there is nothing in the server log either.

PLAIN_TEXT MediaType is now used when Exception serialization fails (as well as logging the error to the server), and a message telling the user to check the server logs for more details is now shown.

rsynek · 2019-05-13T10:59:17Z

.../src/main/java/org/optaweb/employeerostering/server/common/jaxrs/OptaWebExceptionMapper.java

+        ObjectMapper objectMapper = objectMapperProvider.getContext(ServerSideException.class);
+        ServerSideException serverSideException;
+        if (exception instanceof IllegalArgumentException) {
+            serverSideException = new ServerSideException(exception, "ServerSideException.illegalArgument",


Suggestions: String literals - especially those with some specific meaning - are usually better to declare as constants.

Might move the different exception types/i18n keys to an enum, and make the enum extract the ServerSideException from the Exception.

Enum sounds good, not sure what you mean about the extraction of the ServerSideException. Would you maybe include some gist or update the PR to illustrate it?

PR Updates; extraction refers to creating the ServerSideExceptionInfo for a given exception (see ExceptionMapping class).

...a/org/optaweb/employeerostering/gwtui/client/gwtjackson/ServerSideExceptionDeserializer.java

...shared/src/main/java/org/optaweb/employeerostering/shared/exception/ServerSideException.java

…ss and add some tests.

…l check

.../src/main/java/org/optaweb/employeerostering/server/common/jaxrs/OptaWebExceptionMapper.java

...a/org/optaweb/employeerostering/gwtui/client/pages/rotation/RotationPageViewportBuilder.java

rsynek

Please see the comments - mainly about suggested test coverage.

rsynek · 2019-05-20T06:14:28Z

.../java/org/optaweb/employeerostering/gwtui/client/common/FailureShownRestCallbackFactory.java

+        }
+
+        @Override
+        public void onFailure(final Throwable throwable) {


Would you please explain why is the behaviour of onError different than of onFailure ?

onError occurs when we have a Response (which may have a Json Body and if so contain a Server Exception); onFailure occurs when we only have a Throwable (and thus cannot extract a Server Exception from it).

...-gwtui/src/main/java/org/optaweb/employeerostering/gwtui/client/exception/RESTException.java

...a/org/optaweb/employeerostering/gwtui/client/gwtjackson/ServerSideExceptionDeserializer.java

rsynek · 2019-05-20T06:28:19Z

...-rostering-server/src/main/java/org/optaweb/employeerostering/server/util/HierarchyTree.java

+ * @param <K> The type of the key element in the Hierarchy
+ * @param <V> The type of the value element in the Hierarchy
+ */
+public class HierarchyTree<K, V> {


@Christopher-Chianelli @ge0ffrey was it the plan to fully implement https://issues.jboss.org/browse/PLANNER-933 now? Given we want to rewrite most of the application, we should think twice about adding every such a complex piece of code.

I think the rewrite apply mostly to the client code (i.e. most of the server code will remain the same). HierarchyTree is server code, so the probability of it changing in the rewrite is low.

rsynek · 2019-05-20T08:48:06Z

@Christopher-Chianelli one more thing; does the dialog showing a full stacktrace have to display the error icon? Right now there is no space between the picture and text; I think the presence of the picture is questionable.

Christopher-Chianelli · 2019-05-21T13:39:42Z

@rsynek No, I think it was requested for there to be an error icon in the original Error Popup, which the Stack Trace uses. However, I can easily remove/change it.

rsynek · 2019-05-23T08:56:24Z

@rsynek No, I think it was requested for there to be an error icon in the original Error Popup, which the Stack Trace uses. However, I can easily remove/change it.

I am not a UX expert, but I cannot see any added value by having the icon there. As the stack trace text is closely attached to the icon, it doesn't look really well.

rsynek

So summarize the remaining concerns:

I suggest adding tests also for the OptaWebExceptionMapper
please consider removing the icon from the detailed error screen

Otherwise looks good and we can merge. Thanks!

rsynek · 2019-05-23T09:05:55Z

.../src/test/java/org/optaweb/employeerostering/gwtui/client/gwtjackson/TestJsonReaderImpl.java

+ * is the "next token" (i.e. if the current token in the parser is 10,
+ * then nextInt() should return 10)
+ */
+public class TestJsonReaderImpl implements JsonReader {


It's a pity there is no common interface (I haven't found any) between gwt-jackson and jackson-core, that would suit our needs. That way you could just switch the implementations.

rsynek · 2019-05-23T09:08:43Z

.../src/main/java/org/optaweb/employeerostering/server/common/jaxrs/OptaWebExceptionMapper.java

+import org.optaweb.employeerostering.shared.exception.ServerSideExceptionInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 @Provider
 public class OptaWebExceptionMapper implements ExceptionMapper<Exception> {


This class has no test coverage - please take a look at the SonarCloud coverate report. If you have any troubles seeing the report, let me know, I will be happy to help.

It is partially covered in the integration test (although we don't inspect the JSON data in the integration test). I can add some JSON inspection tests to the integration tests (as well as create a unit test for it).

rsynek

Thanks for the additional coverage and various improvements discussed in this PR.

LGTM.

Christopher-Chianelli requested a review from rsynek May 9, 2019 20:10

RHDM-919: Improved error messages

a738b9f

Christopher-Chianelli force-pushed the RHDM-919 branch from 63fa9ef to a738b9f Compare May 9, 2019 20:11

Fonifan reviewed May 10, 2019

View reviewed changes

...-gwtui/src/main/java/org/optaweb/employeerostering/gwtui/client/exception/RESTException.java Show resolved Hide resolved

...rces/org/optaweb/employeerostering/gwtui/client/resources/i18n/OptaWebUIConstants.properties Outdated Show resolved Hide resolved

rsynek requested changes May 13, 2019

View reviewed changes

Christopher-Chianelli added 4 commits May 15, 2019 13:17

Refactor code to use enums, remove some code smells

23d15fe

Fixed some bugs

4ef063c

Add a HierarchyTree data type to find the most specific exception cla…

fa0e945

…ss and add some tests.

Changed catch Throwable to catch Exception, used a List to remove nul…

0888da2

…l check

rsynek reviewed May 16, 2019

View reviewed changes

.../src/main/java/org/optaweb/employeerostering/server/common/jaxrs/OptaWebExceptionMapper.java Outdated Show resolved Hide resolved

yurloc reviewed May 16, 2019

View reviewed changes

...a/org/optaweb/employeerostering/gwtui/client/pages/rotation/RotationPageViewportBuilder.java Outdated Show resolved Hide resolved

Fixed a formatting issue

8e05376

rsynek requested changes May 20, 2019

View reviewed changes

Add client side tests for reading errors from the server

6e0efb6

Christopher-Chianelli requested a review from rsynek May 22, 2019 17:06

rsynek reviewed May 23, 2019

View reviewed changes

Remove Error Popup Icon, Add ExceptionMapper tests

4cd97ad

rsynek approved these changes May 28, 2019

View reviewed changes

Christopher-Chianelli merged commit d6b6479 into kiegroup:master May 28, 2019

Christopher-Chianelli deleted the RHDM-919 branch November 19, 2019 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHDM-919: Improved error messages #238

RHDM-919: Improved error messages #238

Christopher-Chianelli commented May 9, 2019

Fonifan left a comment

rsynek left a comment

rsynek May 13, 2019

Christopher-Chianelli May 13, 2019

rsynek May 14, 2019

Christopher-Chianelli May 15, 2019

rsynek May 13, 2019

Christopher-Chianelli May 13, 2019

rsynek May 14, 2019

Christopher-Chianelli May 15, 2019

rsynek left a comment

rsynek May 20, 2019

Christopher-Chianelli May 21, 2019

rsynek May 23, 2019

rsynek May 20, 2019

Christopher-Chianelli May 21, 2019

rsynek commented May 20, 2019

Christopher-Chianelli commented May 21, 2019

rsynek commented May 23, 2019

rsynek left a comment

rsynek May 23, 2019

rsynek May 23, 2019

Christopher-Chianelli May 23, 2019

rsynek left a comment

RHDM-919: Improved error messages #238

RHDM-919: Improved error messages #238

Conversation

Christopher-Chianelli commented May 9, 2019

Fonifan left a comment

Choose a reason for hiding this comment

rsynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsynek commented May 20, 2019

Christopher-Chianelli commented May 21, 2019

rsynek commented May 23, 2019

rsynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsynek left a comment

Choose a reason for hiding this comment