Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove @Provider annotation from JsonParseExceptionMapper and JsonMappingExceptionMapper #22

Closed
pnsantos opened this issue Jul 25, 2013 · 28 comments
Milestone

Comments

@pnsantos
Copy link

Currently jackson's json provider package (com.fasterxml.jackson.jaxrs.json) includes ExceptionMappers for JsonMappingException and JsonParseException.

This may cause unpredictable behaviour if the application also has an ExceptionMapper for either JsonMappingException or JsonParseException and includes in the registration the com.fasterxml.jackson.jaxrs.json package (where the Json and Jaxb providers are located). The result is that depending on which class gets registered first it will be the one used by jersey. From what I can gather the order by which they get picked up is random, sometimes the app's mappers get used, other times it's the jackson ones.

I understand the reasoning behind the inclusion of these mappers, but I think their registration as provider should at least be configurable (maybe as a feature?). It could also be a good idea to put them in another package to avoid people registering the com.fasterxml.jackson.jaxrs.json package and "accidentally" also registering the mappers.

@cowtowncoder
Copy link
Member

Hmmh. I don't know what could be done here actually: these are NOT registered anywhere, just offered for convenience. For any usage would have to be either explicit (intentional), or use scanning from classpath.
I am personally against use of scanning, for exactly these kinds of reasons; but for some reason, Oracle seems to be pushing for more usage. Go figure. I think they nowadays do things wrong by default.

Be that as it may, I am open to suggestions for improvement. But don't quite see an obvious solution here.

@pnsantos
Copy link
Author

I solved this "problem" by using javax.ws.rs.Application to programmatically, and explicitly register my classes (in that way no automatic classpath scan is done), because if I used only web.xml somewhow jackson included classes would be automatically scanned and registered by glassfish (btw is there any way to disable automatic classpath scanning when using web.xml?)

But I guess you're right, if this is due to the automatic scan then there's nothing jackson could do (except document this possible behaviour and possible ways to avoid it)

@cowtowncoder
Copy link
Member

Ok. I hope we'll figure something out -- I agree that this is problematic, so your problem report is valid.
I just can't yet see what to do with it.

One possibility would be to just remove @Provider annotation; and require users to sub-class, or explicitly register it. This would be safer. Would this make more sense?

@pnsantos
Copy link
Author

That would certainly be an option. With minimal effor the user could "activate" that provider, and if he wanted he could override the methods to have it's own implementation. Users that don't know about that class and create their own won't have any "unexpected" mappings which is good imo.

@alex-sherwin
Copy link

I agree with just removing @Provider, but unfortunately it would break existing implementations

@mpellegrini
Copy link

I know this is an old post, but since it still remains open I would like to add some more commentary in hopes of removing the @Provider annotation from JsonMappingExceptionMapper and JsonParseExceptionMapper

As described above, due to explicitly marking JsonMappingExceptionMapper or JsonParseExceptionMapper with @Provider it will get auto-registerd with JAX-RS implementations and therefore make it more difficult to override this exception mapper with something more customized. And since these were offered for convenience it seems to defeat the intended purpose.

With regard to the comment "...removing @Provider, unfortunately it would break existing implementations" this is not entirely true. The net effect would be the same, a JsonMappingException or JsonParseException would be raised and sent to the client including the same 400 BAD REQUEST. The only thing that would be different would be the text returned to client.

@YuriHeupa
Copy link

There is any update on this issue? Any workaround to override the exceptions?

@cowtowncoder
Copy link
Member

No progress that I know of; I don't think anyone is working on it.

@h-yaron
Copy link

h-yaron commented Feb 4, 2016

I found a workaround, when JacksonJaxbJsonProvider is registered than the JsonMappingExceptionMapper and JsonParseExceptionMapper will not be registered by JacksonFeature (At least in the version we use).

Simple add this to your JerseyConfigurer implementation configure method:
config.register(JacksonJaxbJsonProvider.class, MessageBodyReader.class, MessageBodyWriter.class);

@chamalis
Copy link

Thank you h-yaron, your workaround worked for me, despite the fact that I was not explicitly registering JacksonJaxbProvider.

@chenjianjx
Copy link

chenjianjx commented May 29, 2016

Please remove all the Exception Mappers and let the users have their own. Really bad design, isn't it? Tons of developers' hours have been wasted.

The price would be incompatibility with previous versions, but it is worth it.

@cowtowncoder
Copy link
Member

cowtowncoder commented May 31, 2016

Once again do note that the implementation is merely included in jar, and NOT explicitly registered ANYWHERE. If you choose to include it or decide to use classpath auto-discovery, it may be found. Either way you are including it and can as well exclude it. Especially if you are using classpath-discovery you are sort of getting whatever there might be, and should really consider how fragile it is to use mechanism that relies on something as arbitrary as what might be found from fully expanded classpath, including all dependencies of your web app and everything it depends indirectly.

@cowtowncoder
Copy link
Member

@mpellegrini Do you have a link to JAX-RS specification (or implementation specific documents) that detail how auto-discovery for @Provider entries is set? Since this can only occur via classpath auto-discovery, use of which is dangerous and (IMO) anti-pattern, it would seem unwise for any framework to enable it by default.

If it is the case that major frameworks (or, worse, JAX-RS 2.x) specify default to use classpath auto-discovery I will consider removal of @Provider annotation. Regardless I will bring this up on mailing list to get some unbiased information (biased just in the sense that anyone ending here has been affected by the issue; so any users who do not find this problematic are not going to be commenting).

@mpellegrini
Copy link

@cowtowncoder I am currently working in and constrained to using JavaEE 6 level specs. Therefore the reference to JAX-RS is to the 1.1 Spec (https://jsr311.java.net/nonav/releases/1.1/spec/spec.html). Not sure if JAX-RS 2.0 changes this behavior or not. In Chapter 2 Applications, section 2.3 Publication it states the following (see bold highlights) :

It is RECOMMENDED that implementations support the Servlet 3 framework pluggability mechanism to enable portability between containers and to avail themselves of container-supplied class scanning facilities. When using the pluggability mechanism the following conditions MUST be met:

If no Application subclass is present the added servlet MUST be named:
javax.ws.rs.core.Application

and all root resource classes and providers packaged in the web application MUST be included in the published JAX-RS application. The application MUST be packaged with a web.xml that specifies a servlet mapping for the added servlet.

If an Application subclass is present and there is already a servlet defined that has a servlet initialization parameter named:
javax.ws.rs.Application

whose value is the fully qualified name of the Application subclass then no new servlet should be added by the JAX-RS implementation’s ContainerInitializer since the application is already being handled by an existing servlet.

If an Application subclass is present that is not being handled by an existing servlet then the servlet added by the ContainerInitializer MUST be named with the fully qualified name of the Application subclass. If the Application subclass is annotated with @ApplicationPath and no servlet-mapping exists for the added servlet then a new servlet mapping is added with the value of the @ApplicationPath annotation with ”/*” appended otherwise the existing mapping is used. If the Application subclass is not annotated with @ApplicationPath then the application MUST be packaged with a web.xml that specifies a servlet mapping for the added servlet. It is an error for more than one application to be deployed at the same effective servlet mapping

In either of the latter two cases, if both Application.getClasses and Application.getSingletons return an empty list then all root resource classes and providers packaged in the web application MUST be included in the published JAX-RS application. If either getClasses or getSingletons return a non-empty list then only those classes or singletons returned MUST be included in the published JAX-RS application.

@cowtowncoder
Copy link
Member

@mpellegrini Thank you for including relevant section(s). One follow-up question: what does 'root' in "all root resource classes and providers packaged in the web application" mean?

Also: assuming that @Provider annotation was removed from exception mapper implementation, could it be explicitly registered either as an instance, or by Class? If annotation may be omitted I have objection to removing it, but if it is always required it would require sub-classing by users which seems to defeat the purpose.

@cowtowncoder cowtowncoder changed the title Support enabling/disabling included ExceptionMappers Remove @Provider annotation from JsonParseExceptionMapper and JsonMappingExceptionMapper Jun 2, 2016
@cowtowncoder
Copy link
Member

Multiple people have confirmed that @Provider is not needed if explicitly registering things, so I will go ahead and remove that annotation from 2.8. While this does change some of behavior, user feedback has been such that this change is preferred over current state; I did not get any feedback to suggest otherwise on mailing lists.

@cowtowncoder cowtowncoder modified the milestones: 2,3,, 2.8.0 Jun 3, 2016
@v-mihaylov
Copy link

@cowtowncoder, seems like you just changed the comment in JsonParseExceptionMapper but forgot to actually remove the @Provider annotation.

@cowtowncoder
Copy link
Member

@v-mihaylov Thank you! Yes, looks like I did, will fix.

@ghulamemustafa
Copy link

ghulamemustafa commented Aug 26, 2016

I am struggling with same issue even after updating to jackson-jaxrs-base-2.8.1. My ExceptioMapper is just never invoked.

@Provider
public class JsonParseExceptionMapper implements ExceptionMapper<JsonParseException> {

    @Override
    public Response toResponse(JsonParseException exception) {
        exception.printStackTrace();
        return Response.status(ResponseCodes.KO_PARSING_ERROR.getHttpStatus())
                .entity("Parsing Error").build();
    }
}

@cowtowncoder
Copy link
Member

@ghulamemustafa Make sure that version of actual provider (like jackson-jaxrs-json-provider) has 2.8 versio as well.

Other than that perhaps this is then an issue with container not finding the exception mapper: Jackson JAX-RS module can not register your exception mappers so you need to make that happen.

@ghulamemustafa
Copy link

ghulamemustafa commented Aug 27, 2016

@cowtowncoder (All jars were already updated to latest releases) I got my issue solved in following way.
since i am using jersey-media-json-jackson to use jackson as provider with jersey , it was explicitly registering JsonParseExceptionMapper and JsonMappingExceptionMapper. I used @priority annotation with my exception mappers so that my ExceptionMappers are preferred over native.

here is the code.

@Priority(4000)
@Provider
public class JsonParseExceptionMapper implements ExceptionMapper<JsonParseException> {

    @Override
    public Response toResponse(JsonParseException exception) {
        exception.printStackTrace();
        return Response.status(ResponseCodes.KO_PARSING_ERROR.getHttpStatus())
                .entity("Parsing Error").build();
    }
}

@cowtowncoder
Copy link
Member

@ghulamemustafa Thank you for sharing your solution! Not sure why default priority would be set so high that custom ones would not be used, but it does sound like this is what is happening.

@pjajara
Copy link

pjajara commented Oct 17, 2016

@cowtowncoder Your solution works. I think its a hack but I can live with it for now :)

Thanks for sharing it with community.

juliancesar added a commit to demoiselle/framework that referenced this issue Jan 9, 2017
não deixava fazer EsxceptionMapper para as Exceptions JsonParser e
JsonMapper -
FasterXML/jackson-jaxrs-providers#22
@chellia
Copy link

chellia commented Apr 18, 2017

With @priority(4000) also it is not working. sometimes it is picking one over the other.

@cen1
Copy link

cen1 commented May 12, 2017

Weirdly enough, even though this was supposedly fixed in 2.8 we are on 2.8.5 and there is still some stray mapper being defined somewhere and gets picked up by Jersey before our own mapper. @priority fix seems to help but I am not yet sure it's a full fix. I'll try to find where the offending mapper comes from.

@paolobazzi
Copy link

paolobazzi commented Dec 12, 2017

As mentioned in this Stackoverflow thread the JackonFeature within jersey-media-json-jackson artifact registeres exception mappers for JsonParseException and JsonMappingException.

Since the JacksonFeature cannot be changed, as long as you need to use this feature and the feature registers those exception mappers directly within its code, the only workaround is to register own exception mappers for both exceptions using a lower priority.

@Priority(1) // override Jackson JsonMappingExceptionMapper 
public class JsonMappingExceptionMapper extends AbstractExceptionMapper<JsonMappingException> {
//...
}

@kspar
Copy link

kspar commented Mar 12, 2018

I was also using Jersey 2.26 and jersey-media-json-jackson as Jersey's user guide recommends (https://jersey.github.io/documentation/latest/media.html#json.jackson). I was encountering this bug and no previously mentioned workarounds worked for me. I ended up switching jersey-media-json-jackson for Jackson's own jackson-jaxrs-json-provider. This way no default ExceptionMappers are included and the problem is solved. Keep in mind that Jackson's own JSON provider must be registered manually (it won't be discovered by Jersey's metainf services lookup unlike Jersey's JacksonJaxbJsonProvider):
register(com.fasterxml.jackson.jaxrs.json.JacksonJaxbJsonProvider.class);

@mrharrylv
Copy link

Have the same issue, but would like to leave @Provider as I need Applications auto scan ( do not want to register manually all the classes). And Priority does not work at all.
Any permanent workarounds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests