Return XML formatted error messages

[mosiac1]

We noticed a regression in our testing suite for internal plugins which seems to be cause by bfbcaf2, specifically the upgrade from io.airlift:airlift version 249 to 266 which bumps the jetty version to 12.0.13.

The test added in this PR passed on io.airlift:airlift version 252, which would indicate what caused the regression was added in jetty 12.0.12.

The root cause is that a RequestLoggerController has unclosed sessions when the Airlift Boostrap lifecycle is getting closed.
This is because runtime exceptions (that do not inherit from WebApplicationException) thrown during request processing (here thrown in the credentials provider plugin, but anywhere works really) causes jetty to not call our response filter RequestFilter#filter(ContainerRequestContext, ContainerResponseContext), thus not closing the request logger session.

I'm looking into how to fix this regression.

[mosiac1]

@ragnard you are probably most familiar with jetty and airlift, any idea why this is happening? This would seem like a jetty bug, but I haven't isolated it to that.

[mosiac1]

The ContainerResponseFilter works correctly if there is ExceptionMapper which processes the exceptions.

Repurposed the PR to add a mapper for any Throwable that returns an error response formatted based on https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingRESTError.html

[Randgalt]

Please add compact constructor with notNull checks

[vagaerg]

Some of them may be null though right? They're being excluded with the @JsonIncludeannotation

[mosiac1]

True, but since we saw Optional works I'm in favour of using that and maintaining null safety. We can remove the @JsonInclude annotation.
We can always come up with values for code, message and resource.

[vagaerg]

Happy to maintain null safety, but we should then make sure we never construct one of these with a null value. We don't want to have our exception handling logic result in an unhandled exception because of a null check when constructing the response

[Randgalt]

A few comments

[vagaerg]

Should we create a constructor that takes in optionals for all fields and uses a default if the optional is empty?

For instance, if the exception has a null message (which may happen) we'd fail at the requireNonNull stage and then we lose all the information for the other fields that may be present.

[mosiac1]

Good point!
I think making all the fields optionals is a bit verbose. The code and resources should always be resolvable (since these are the response status code and the request path), so we can make just the message an optional.