ExceptionMapperFactory should take into account priority of mappers as set by @Priority annotation

jerseyrobot commented 9 years ago

Hello Jersey Team,

After upgrading from 2.6 to 2.13 I discover issue with Jersey exception mappers. I do have my own exception mapper defined for JsonMappingException (among many other mappers), but whenever I send invalid JSON, the Jersey is using default exception mapper that comes registered with JacksonFeature (see logs below):

Request:

2014-12-03 13:14:59,540 [qtp880578076-30] INFO  com.github.sarxos.elm.Application - 15 * Server has received a request on thread qtp880578076-30
15 > POST https://x.x.x.x:8443/xxx/api/register 15 > Content-Type: application/json; charset=UTF-8
{
"t":"abba",
"s":44444,
}

And the response:

2014-12-03 13:14:59,549 [qtp880578076-30] DEBUG org.glassfish.jersey.tracing.general - EXCEPTION_MAPPING Exception mapper [com.fasterxml.jackson.jaxrs.base.JsonMappingExceptionMapper @499ca5ec] maps [com.fasterxml.jackson.databind.JsonMappingException @6600e06] ('Numeric value (44444) out of range of Java byte
 at [Source: org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream@2eda6dd2; line: 3, column: 10] (through reference chain: com.github.sarxos.elm.Device["s"])') to <400/CLIENT_ERROR|Bad Request> [ 0.09 ms]
2014-12-03 13:14:59,550 [qtp880578076-30] INFO  com.github.sarxos.elm.Application - 16 * Server responded with a response on thread qtp880578076-30
16 < 400
16 < Content-Type: text/plain
Numeric value (44444) out of range of Java byte
 at [Source: org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream@2eda6dd2; line: 3, column: 10] (through reference chain: com.github.sarxos.elm.Device["s"])

This fragment is important:

Exception mapper [com.fasterxml.jackson.jaxrs.base.JsonMappingExceptionMapper @499ca5ec] maps [com.fasterxml.jackson.databind.JsonMappingException @6600e06]

I can clearly see, that Jersey is using default JsonMappingExceptionMapper instead of my own mapper I created especially for this purpose.

I suspect that the effect I'm observing is caused by the enhancement implemented by #2555 in 2.7, commit 4f04eda4079305b8f5b357902de7d9e09c581d34.

I'm unable to reproduce it on the development environment (Ubuntu 14, Oracle Java 8, Eclipse), but on the test environment (CentOS 6, OpenJDK 7) it is reproducible in 100% cases. I suspect that JARs order or class loading time is important here. In the Jersey ExceptionMapperFactory these two mappers (custom one I expect to be used, and a default one) has the same inheritance distance calculated, so I guess that it's safe to assume that the first one found will be always returned and there is no other order rule here.

Therefore, due to above, maybe it's worth to consider adding e.g. tt>@Piority</tt annotation value to be compared when distance for all available mappers of the same type is exactly the same? What do you think?

Environment

OpenJDK 7 (1.7.0_65)
CentOS 6
Jetty Embedded 9.x
Affected Versions

[2.13]

jerseyrobot commented 6 years ago

Issue Imported From: https://github.com/jersey/jersey/issues/2994
Original Issue Raised By:@glassfishrobot
Original Issue Assigned To: @mpotociar

jerseyrobot commented 9 years ago

@glassfishrobot Commented Reported by sarxos

jerseyrobot commented 9 years ago

@glassfishrobot Commented @japod said: Until this gets implemented, you should be able to use https://jersey.java.net/apidocs/2.13/jersey/org/glassfish/jersey/spi/ExtendedExceptionMapper.html to workaround the issue.

jerseyrobot commented 9 years ago

@glassfishrobot Commented sarxos said: Jakub,

Thank you. For now I w/a this by adding this piece of code in my ResourceConfig:

@Inject
public MyResourceConfig(ServiceLocator locator) {
    // w/a for JERSEY-2722  register(JacksonJaxbJsonProvider.class, MessageBodyReader.class, MessageBodyWriter.class);
    // ... }

This is to skip the code from if block from line 81 in JacksonFeature.java.

jerseyrobot commented 8 years ago

@glassfishrobot Commented gdemecki said: @Jakub what is the correct usage of the ExtendedExceptionMapper?

I'm asking because with ExtendedExceptionMapper I've failed to override built-in exception mappers:

com.fasterxml.jackson.jaxrs.base.JsonParseExceptionMapper
com.fasterxml.jackson.jaxrs.base.JsonMappingExceptionMapper

Only workaround given by @sarxos worked fine for me.

jerseyrobot commented 8 years ago

@glassfishrobot Commented tomasz.kalkosinski said: This is still an issue in 2.22.1. In effect you cannot implement your own ExceptionMapper that implements ExceptionMapper interface. Default ValidationExceptionMapper is always first on list and logic in ExceptionMapperFactory#find always prefers first one found.

Jakub Podlesak you cannot use ExtendedExceptionMapper because ExceptionMapperFactory#isPreferredCandidate can use it only if sameDistance is different, not equal.

I cannot workaround that and remove ValidationExceptionMapper from processing ValidationException.

jerseyrobot commented 8 years ago

@glassfishrobot Commented gdemecki said: tomasz.kalkosinski your own ExceptionMapper should always have a higher priority than anything provided by Jersey. If you encounter the opposite - please immediately report a bug.

In your case, it can be related to the #3425 regression bug - which appears when you're using two Jersey extensions at the same time: jersey-bean-validation and jersey-weld2-se/ jersey-cdi1x. Because without a dependency to Weld, your custom ValidationExceptionMapper will take precedence over the one provided by jersey-bean-validation module.

jerseyrobot commented 7 years ago

@glassfishrobot Commented This issue was imported from java.net JIRA JERSEY-2722

xak2000 commented 5 years ago

Still doesn't work in Jersey 2.27.

And actually this is not an improvement, this is a bug.

As per JAX-RS 2.1 specification

4.1.3 Priorities

Application-supplied providers enable developers to extend and customize the JAX-RS runtime. Therefore, an application-supplied provider MUST always be preferred over a pre-packaged one if a single one is required.

Application-supplied providers may be annotated with @Priority. If two or more providers are candidates for a certain task, the one with the highest priority is chosen: the highest priority is defined to be the one with the lowest value in this case. That is, @Priority(1) is higher than @Priority(10). If two or more providers are eligible and have identical priorities, one is chosen in an implementation dependent manner.

The default priority for all application-supplied providers is javax.ws.rs.Priorities.USER. The general rule about priorities is different for filters and interceptors since these providers are collected into chains.

(the emphasis is mine)

All three pre-packaged exception mappers (I'm use jersey-media-json-jackson and jersey-bean-validation modules): JsonMappingExceptionMapper, JsonParseExceptionMapper and ValidationExceptionMapper - takes priority before my custom exception mappers for same exception class.

The @Priority doesn't help. .register(MyJsonParseExceptionMapper.class, 1) also doesn't help.

For `JacksonFeature` this workaround works:

context.register(JacksonJaxbJsonProvider.class, MessageBodyReader.class, MessageBodyWriter.class);

It works only because JacksonFeature directly checks if JacksonJaxbJsonProvider.class class is already registered and skips own exception mapper registrations in this case. But it also skips registration of JacksonFilteringFeature and FilteringJacksonJaxbJsonProvider which sholud be registered instead of JacksonJaxbJsonProvider if EntityFilteringFeature is enabled. So this workaround works only if filtering feature is not required.

For `ValidationExceptionMapper` this workaround works:

.register(new AbstractBinder() {
    @Override
    protected void configure() {
        bind(MyValidationExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class);
    }
})

It works because standard ValidationBinder registers exception mapper like this:

        // Custom Exception Mapper and Writer - registering in binder to make possible for users register their own providers.
        bind(ValidationExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class);
        bind(ValidationErrorMessageBodyWriter.class).to(MessageBodyWriter.class).in(Singleton.class);

Ironically, the comment above states that this way of registering allows users to register custom exception mappers. :-)

In both cases custom mappers should work out of the box, just by registering them using discovery mechenism or manually by .register(MyExceptionMapper.class), and without any fragile workarounds.

ktalebian commented 4 years ago

@xak2000 bind(MyValidationExceptionMapper.class).to(ExceptionMapper.class).in(Singleton.class); works partially for me.

It does not work on the request validation using the validation annotations (using @Valid etc still uses the default validation mapper).

However, it does work if I manually throw a new ValidationException somewhere in the code. Any ideas why?

eclipse-ee4j / jersey