API exception responses should contain more detail

[jan-law]

Resolves #435. Exceptions that get re-thrown as a new HttpStatusException(500, e) by a handler get caught and handled by WebServer. The new HttpStatusException does not contain an error message itself, but the WebServer can append the underlying exception message to the API exception responses with e.getCause().getMessage():

Slightly unrelated question: Why does JaCoCo show that the WebServer failure handler isn't covered by any unit tests? I found an integration test for ClientUrl that only tests for successful scenarios.

[jan-law]

Question: I noticed the web client notification omits the error message if I intentionally throw an error in ClientUrlGetHandler. (The other errors I’ve tested display enough detail in their notifications). Is this worth looking into?

[andrewazores]

Slightly unrelated question: Why does JaCoCo show that the WebServer failure handler isn't covered by any unit tests?

I think that actually is the case. WebServerTest.java is pretty sparse and mostly just tests some happy paths on other methods of the WebServer, but doesn't go into testing any of the router setup which would include the failureHandler. Testing here would definitely be a nice code quality improvement. The WebServer#start() method should also probably be refactored and split up to make testing easier, and failureHandler could be converted to an actual method on the webserver instead of being a method-scope lambda function.

I found an integration test for ClientUrl that only tests for successful scenarios.

The ClientUrlGetHandler that implements that API is pretty simple, and doesn't expect the calling client to provide any kind of parameters. So in an integration test (ClientUrlIT) scenario, the happy path/successful cases are really the only ones that can be tested.

The ClientUrlGetHandlerTest unit test is more comprehensive and does cover at least some failure scenarios, although I haven't checked the code coverage report so I'm not sure what that figure is exactly.

Question: I noticed the web client notification omits the error message if I intentionally throw an error in ClientUrlGetHandler. (The other errors I’ve tested display enough detail in their notifications). Is this worth looking into?

Yes, sounds worth looking into. That's just about the first API call that the web client makes, and the response tells the client what URL to connect to for the websocket notification channel, so if any exception occurs here then 1) something is definitely wrong with the Cryostat deployment, 2) the web-client is going to be only partially functional for the user, so they should be given some proper information that something is wrong and some hint about what the issue is.

[jan-law]

the web-client is going to be only partially functional for the user, so they should be given some proper information that something is wrong and some hint about what the issue is.

Thanks, I'll check it out and let you know what I find.

Testing here would definitely be a nice code quality improvement.

Should I create an issue for refactoring Webserver#start, including adding tests? It seems to be outside the scope of the API exceptions issue.

[andrewazores]

Nice and simple :-)

I think, from the screenshot you've provided and from what I remember of how I implemented the various exception handling paths that lead here, that this is probably already a sufficient fix for the issue.

But, it might be possible to enhance this message further - it's worth experimenting with at least, although in the end we may just come back to the current solution you have here.

Take a look at this utility class: https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/exception/ExceptionUtils.html

We already have this as a dependency and use this class in a couple of other places.

The methods getMessage and getThrowables in particular could be neat here to provide even more context around these exceptions.

[andrewazores]

Should I create an issue for refactoring Webserver#start, including adding tests? It seems to be outside the scope of the API exceptions issue.

Yes, this is definitely out of scope of this issue/this PR. I do think it's a work item worth adding to the backlog, so do go ahead and file an issue (cleanup label).

[jan-law]

Question: I noticed the web client notification omits the error message if I intentionally throw an error in ClientUrlGetHandler.

The console is showing a SyntaxError caused by the NotificationChannel class. I think the {} message we previously saw in the web ui is the message header of theSyntaxError: Unexpected token I in JSON at position 0, not the API error message. Here's what happens if I edit NotificationChannel to print the error message:

In NotificationChannelService.tsx, lines 64-67, it looks like the clientUrl gets parsed from the json response without checking that the json response returned an OK status code. As a result, it tries to read a url from the response, but finds the letter I from Internal Server Error instead.

To fix this, we might need to edit the web client to check for server errors before parsing a url from the json response?

[andrewazores]

Nice investigation, that explanation makes sense to me. I've filed a corresponding bug in the -web repo.

[andrewazores]

Nice. Here's what I observed just now, when I did the same modification to WebServer#getDownloadURL, ie:

--- a/src/main/java/io/cryostat/net/web/WebServer.java
+++ b/src/main/java/io/cryostat/net/web/WebServer.java
@@ -251,7 +251,7 @@ public class WebServer {
 
     public String getDownloadURL(JFRConnection connection, String recordingName)
             throws URISyntaxException, IOException {
-        return new URIBuilder(getHostUri())
+        String s = new URIBuilder(getHostUri())
                 .setScheme(server.isSsl() ? "https" : "http")
                 .setPathSegments(
                         "api",
@@ -263,6 +263,7 @@ public class WebServer {
                 .build()
                 .normalize()
                 .toString();
+        throw new URISyntaxException(s, "just because");
     }

I think this looks pretty good and definitely gives good context as to what the reason for the exception was, without forcing the user to go look in the Cryostat log.