Allow access to thrown exception in `ResponseContext`

[GregJohnStewart]

Description

I have a universal mapper in my app that maps all error responses to a unified error response json object.

However, I have noticed that in certain cases, I cannot determine the root cause of a thrown error. This is despite the ResponseContext I am handling has the error in it's throwable field.. There simply just doesn't appear to be a way to retrieve said exception.

Above, clearly the info is there, (bottom entry, the cause of the throwable field is my intended exception to handle) just no way to access:

Unless I am missing something, there should be a way to get this information.

This only seems to affect cases where the throwable isn't set as the response context's entity. Depending on the intent, this could be the bug in itself? I'm noticing when jackson serialization errors occur (as an example), the entity of the response context is just null.

My mapper, in case curious/ relevant: https://github.com/Epic-Breakfast-Productions/OpenQuarterMaster/blob/main/software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/filters/ExceptionObjectNormalizer.java

Implementation ideas

Adding appropriate getters to the response context(s).

It looks like if there was a getter for the inner context field, that would suffice

[quarkus-bot[bot]]

/cc @FroMage (rest)

[geoand]

Thanks for reporting.

I must admit that I have trouble understanding what's going on. Would it be possible for you to strip down the code to the bare minimum that demonstrates the problem?

[FroMage]

I think, the problem is that the user is writing a request filter and wants to access the exception, but the API for the response context only lets you access the "normal flow" properties, such as status code, headers, entity: https://jakarta.ee/specifications/platform/10/apidocs/jakarta/ws/rs/container/containerresponsecontext

Usually, people will not write filters to map exceptions, but exception mappers: https://quarkus.io/guides/rest#exception-mapping.

Note that the spec says that response filters should be invoked after the exception mappers turn the exception into a response (See https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1.html#server-runtime):

A response mapped from an exception MUST be processed using the filter chain and the interceptor chain (if an entity is present in the mapped response)

So… if you must handle your exception in your filter, you can register an exception mapper and bundle the exception into a Response entity… which your filter can then use.

But I'd advise dealing with exceptions in the exception mapper, not in the response filter.

Does this help?

[GregJohnStewart]

Mappers I am using for my custom exceptions, but I feel like is possibly sub-optimal for making sure all exceptions are handled in this way

The main issue with mappers is I'm not looking to change the current response besides provide a useful data structure to outline the issue (mappers would require me to know/ handle all possible thrown exceptions and know what kind of status code to throw back. I would prefer to let Quarkus handle that logic, and at the end just tie in my unified error object)

Edit: sorry, I've been up since early and re-read your response. Shoving the exception into the entity in a mapper is actually a good idea, assuming the rest (status code) is handled

[GregJohnStewart]

@geoand the logic at/ after this line here is where I need to have access to the thrown exception: https://github.com/Epic-Breakfast-Productions/OpenQuarterMaster/blob/main/software%2Fcore%2Foqm-core-api%2Fsrc%2Fmain%2Fjava%2Ftech%2Febp%2Foqm%2Fcore%2Fapi%2Ffilters%2FExceptionObjectNormalizer.java#L105

I'm not available to make up a simpler example at the moment

[GregJohnStewart]

At the end of it' I'm just asking for a getter for the held throwable field. Stretch maybe a has/hadException. My own usecase aside, I don't think it's a stretch to want to know that context in certain cases

[geoand]

PR is certainly welcome

[FroMage]

I'm not sure we should add access to the exception in the response filter. I feel like this is bypassing the separation of responsabilities in the API.

Exception mappers are meant for turning exceptions into responses. This sounds precisely like what you're trying to do.

Unless I'm misunderstanding what you want.

The main issue with mappers is I'm not looking to change the current response besides provide a useful data structure to outline the issue (mappers would require me to know/ handle all possible thrown exceptions and know what kind of status code to throw back. I would prefer to let Quarkus handle that logic, and at the end just tie in my unified error object)

So… it looks like you want to let Quarkus turn exceptions into responses… but then later tweak the response based on the exception?

Looking at https://github.com/Epic-Breakfast-Productions/OpenQuarterMaster/blob/main/software%2Fcore%2Foqm-core-api%2Fsrc%2Fmain%2Fjava%2Ftech%2Febp%2Foqm%2Fcore%2Fapi%2Ffilters%2FExceptionObjectNormalizer.java#L92 it looks like you definitely want to set the response from two exception mappers.

I can't guess any reason why you'd want to do that in a response filter rather than in two exception mappers. Problems with precedence, perhaps?

[GregJohnStewart]

Sure, I set it for my custom exceptions, but looking to do this body mapping for all exceptions, which I don't necessarily want to determine the 4xx vs 5xx` of them

[FroMage]

Oh, now I get it. You want to map exceptions to JSON but still forward the HTTP Status Code selection to other exception mappers.

That's an interesting use-case. We did talk about a richer exception mapping API in the past.

Here, I suggested an API that would allow you to delegate to the other exception mappers (excluding the current one):

public class ExceptionsMappers {
  @ServerExceptionMapper
  public RestResponse<Error> mapUnhandledException(RuntimeException ex, /* injected API */ ExceptionMapperHandler mapper) {
    if(ex instanceof WebApplicationException {
      return mapper.mapException(ex.getCause()); // this would go back via the mapper
    }
    // Custom error for any other unhandled exceptions
    ...
  }

  @ServerExceptionMapper
  public RestResponse<Error> mapJsonProcessingException(JsonProcessingException ex) {
    // Custom error for jackson parsing issues, usually wrapped into WebApplicationException
    ...
  }
}

This would allow you to declare a blanket exception mapper, for say Throwable and then forward back to the other exception mappers, get the HTTP Status, and map that back to JSON.

The only issue is that due to the way exception mappers are mapped, a Throwable handler is only invoked when there are no more specific handlers, such as JsonProcessingException here.

So you'd need to be able to either declare that your handler had a higher priority than more specific handlers, or declare your mapper for each more-specific handler with a higher priority.

I think we have that sort of use-case missing from our API. We should think about it, because we've been tweaking the exception mapper API and behaviour for a while and keep coming up with new limitations :-/

Any opinion @geoand ?

[geoand]

That does sounds reasonable, but we would probably need to guard for cases where the mapping goes into an endless loop

[FroMage]

Well, I'm thinking that any mapper that calls back into the system will be excluded from the recursive call, until the last mapper (if they're all recursive) cannot map anything back via recursion (empty list of mappers since they're all pending in the stack).

On the other hand, we're bound to find someone who will want their third mapper to unwrap an exception and map back the exception cause via the full chain of mappers :-/

[geoand]

On the other hand, we're bound to find someone who will want their third mapper to unwrap an exception and map back the exception cause via the full chain of mappers :-/

Yeah...

[GregJohnStewart]

Respectfully, I still don't see the problem for just adding a has/hadException` + getter in the current response context. IMO it's relevant context

I intend to make a PR, but lack the time until tomorrow likely. If you would rather me hold off that's fine too

[FroMage]

Well that would certainly be easier than rework the exception mapper API… except it doesn't feel right, because response mappers are meant to tweak the response, not turn exceptions into responses.

It's entirely possible for exception mappers to generate a proper response, and then your filter would replace this response because there was an exception thrown. Then we'd get into questions of when to clear that exception so that response filters would not act on it. Or how to unwrap it (same problem as for exception mappers).

From your code, I see special cases for DbModValidationException, DbNotFoundException and ViolationReport.

IMO these should be exception mappers, where you produce an entity of type ErrorMessage (with specific info).

From then, you can keep your response filter and turn any other remaining HTTP error status into an ErrorMessage.

What info would be missing for you that would require you to have access to the Exception in the response filter?

[GregJohnStewart]

In my case, again, I am less trying to handle exceptions, but just to provide a unified error response format. I don't want to provide anything else but a consistent and expected error format for consumers of my API (along with the already provided HTTP status, etc provided by Quarkus).

An example exception is when things like Jackson serialization fails; I don't control (nor really want to control) the mapping myself. But adding the flavortext from Jackson's error would be a great help for users. I don't think this is added to the default response object (which is the standard details/error id)?

I also noticed that the entity is null at this stage for such exceptions.

I could conceptualize that knowing an exception (and what exception) happened somewhere in the chain could be useful info in other cases as well.

[FroMage]

In my case, again, I am less trying to handle exceptions, but just to provide a unified error response format. I don't want to provide anything else but a consistent and expected error format for consumers of my API (along with the already provided HTTP status, etc provided by Quarkus).

Perhaps we could think of an API that allows generating error responses in a custom way. So rather than create plain-text error responses, we'd ask the application what format we should use. The JSON Error RFC comes to mind as an option. This would indeed be easier to achieve with a ContainerResponseFilter that can access the error.

An example exception is when things like Jackson serialization fails; I don't control (nor really want to control) the mapping myself. But adding the flavortext from Jackson's error would be a great help for users. I don't think this is added to the default response object (which is the standard details/error id)?

I'm not sure what Jackson errors currently throw, or how you'd improve it. Could you provide examples?

I also noticed that the entity is null at this stage for such exceptions.

That is likely correct, if the exception mapper did not set any entity on the response, which means no HTTP body.

[GregJohnStewart]

On the Jackson bit, I've noticed Quarkus throws a rather generic 400 error (or sometimes 500, depending on the nature/ where it's thrown) if the json can't be deserialized. The result is not having anything in the returned body that described why the deserialization failed (as just empty string, despite the cause of the thrown field has that flavortext)

The plain case is malformed json, but I've had issues when there are deeper structural issues that don't map correctly but there's currently just no feedback for the user.

To that point I could just add a mapper but again, I don't want to decide anything else but the returned body

[FroMage]

This sounds like something we should improve out of the box for all Jackson errors, whether in plain text or in JSON.

In prod mode, however, by design, we do not include the body of exceptions, we just log them, for security reasons.

But in DEV mode we definitly want that improvement. If you could give us a reproducer with these sample Jackson error cases, that'd be great :)