eclipse-ee4j / jersey

Eclipse Jersey Project - Read our Wiki:
https://github.com/eclipse-ee4j/jersey/wiki
Other
692 stars 355 forks source link

Inconsistent media type negotitation #3605

Open jerseyrobot opened 7 years ago

jerseyrobot commented 7 years ago

Hi,

Let's consider following resource running into a Jersey container:

@Path("echo")
public class EchoResource {

    @XmlRootElement
    public static class Message {

        private String message;

        public Message() {
        }

        public String getMessage() {
            return this.message;
        }

        public void setMessage(String message) {
            this.message = message;
        }

    }

    @Path("/echo1")
    @Produces({ "*/xml" })
    @GET
    public Response echo1(@QueryParam("msg") String msg) {
        Message message = new Message();
        message.setMessage(String.valueOf(msg));
        return Response.ok(message).build();
    }

    @Path("/echo2")
    @Produces({ "*/xml", MediaType.APPLICATION_OCTET_STREAM })
    @GET
    public Response echo2(@QueryParam("msg") String msg) {
        Message message = new Message();
        message.setMessage(String.valueOf(msg));
        return Response.ok(message).build();
    }

}

javax.ws.rs.InternalServerErrorException: HTTP 500 Internal Server Error at org.glassfish.jersey.server.internal.MappableExceptionWrapperInterceptor.aroundWriteTo(MappableExceptionWrapperInterceptor.java:90) at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:162) at org.glassfish.jersey.message.internal.MessageBodyFactory.writeTo(MessageBodyFactory.java:1130) at org.glassfish.jersey.server.ServerRuntime$Responder.writeResponse(ServerRuntime.java:711) at org.glassfish.jersey.server.ServerRuntime$Responder.processResponse(ServerRuntime.java:444) at org.glassfish.jersey.server.ServerRuntime$Responder.process(ServerRuntime.java:434) at org.glassfish.jersey.server.ServerRuntime$2.run(ServerRuntime.java:329) at org.glassfish.jersey.internal.Errors$1.call(Errors.java:271) at org.glassfish.jersey.internal.Errors$1.call(Errors.java:267) at org.glassfish.jersey.internal.Errors.process(Errors.java:315) at org.glassfish.jersey.internal.Errors.process(Errors.java:297) at org.glassfish.jersey.internal.Errors.process(Errors.java:267) at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:317) at org.glassfish.jersey.server.ServerRuntime.process(ServerRuntime.java:305) at org.glassfish.jersey.server.ApplicationHandler.handle(ApplicationHandler.java:1154) at org.glassfish.jersey.servlet.WebComponent.serviceImpl(WebComponent.java:473) at org.glassfish.jersey.servlet.WebComponent.service(WebComponent.java:427) at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:388) at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:341) at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:228) at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:812) at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:587) at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:221) at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1127) at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:515) at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185) at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1061) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141) at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:97) at org.eclipse.jetty.server.Server.handle(Server.java:499) at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:311) at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:257) at org.eclipse.jetty.io.AbstractConnection$2.run(AbstractConnection.java:544) at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:635) at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:555) at java.lang.Thread.run(Thread.java:745) Caused by: org.glassfish.jersey.message.internal.MessageBodyProviderNotFoundException: MessageBodyWriter not found for media type=application/octet-stream, type=class org.nne.jaxrs.ext.oauth2.server.demo.rs.resource.EchoResource$Message, genericType=class org.nne.jaxrs.ext.oauth2.server.demo.rs.resource.EchoResource$Message. at org.glassfish.jersey.message.internal.WriterInterceptorExecutor$TerminalWriterInterceptor.aroundWriteTo(WriterInterceptorExecutor.java:247) at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:162) at org.glassfish.jersey.server.internal.JsonWithPaddingInterceptor.aroundWriteTo(JsonWithPaddingInterceptor.java:106) at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:162) at org.glassfish.jersey.server.internal.MappableExceptionWrapperInterceptor.aroundWriteTo(MappableExceptionWrapperInterceptor.java:86) ... 35 more

I was expecting the first call to work as the second one, but it does not because of the fact that echo1 method id annotated with @Produces({ "*/xml" }) which contains only 1 media type.

The code to check seems to be MethodSelectingRouter.usePreSelectedMediaType(...) called by MethodSelectingRouter.determineResponseMediaType(...).

Maybe this method should become:

private static boolean usePreSelectedMediaType(final RequestSpecificConsumesProducesAcceptor selectedMethod,
                                                   final List<AcceptableMediaType> acceptableMediaTypes) {
        // Resource method is annotated with @Produces and this annotation contains only one MediaType.
        if (!selectedMethod.producesFromProviders
                && selectedMethod.methodRouting.method.getProducedTypes().size() == 1
             && !MediaTypes.isWildcard(selectedMethod.methodRouting.method.getProducedTypes().get(0)) {
            return true;
        }

        // There is only one (non-wildcard) acceptable media type - at this point the pre-selected method has to be chosen so
        // there are compatible writers (not necessarily writeable ones).
        return acceptableMediaTypes.size() == 1 && !MediaTypes.isWildcard(acceptableMediaTypes.get(0));
    }

What do you think ?

jerseyrobot commented 6 years ago
jerseyrobot commented 7 years ago

@pavelbucek Commented It might seem inconsistent, but it should be compliant with the JAX-RS Spec, chapter "3.8 Determining the MediaType of Responses"

we can't change the implementation without changing the spec.

jerseyrobot commented 7 years ago

@NicoNes Commented Hi @pavelbucek,

Actually I red this chapter of the spec before writting the issue and I still keep on saying that this inconsistency is not because of the spec. Lets apply the spec step by step for each of the two cases I used:

1st case: method annotated with @Produces({ "*/xml" }) and Accept header set to "application/*"

  1. Not applicable in our case

  2. P = {"*/xml"}

  3. P is not empty so P = {"*/xml"}

  4. A is not empty so A = {"application/*"}

  5. MediaType.valueOf("application/*").isComptatible(MediaType.valueOf("*/xml")) returns true so M = {"application/*"} (the most specific).

  6. M is not empty so lets jump to step 7

  7. M.length = 1 so sort operation returns M = {"application/*"}

  8. As M does not contain any concrete type the selected media type can not be determinated here. Go to step 9

  9. M contains {"application/*"} so selected media type is Mselected = "application/octet-stream"

2nd case: method annotated with @Produces({ "*/xml", "application/octet-stream" }) and Accept header set to "application/*"

  1. Not applicable in our case

  2. P = {"*/xml", "application/octet-stream"}

  3. P is not empty so P = {"*/xml", "application/octet-stream"}

  4. A is not empty so A = {"application/*"}

  5. Both MediaType.valueOf("application/*").isComptatible(MediaType.valueOf("*/xml")) and MediaType.valueOf("application/*").isComptatible(MediaType.valueOf("application/octet-stream")) returns true so M = {"application/*", "application/octet-stream"} (the most specific).

  6. M is not empty so lets jump to step 7

  7. Sort operation returns M = {"application/octet-stream", "application/*"}

  8. As M contains a concrete type the selected media type is Mselected = "application/octet-stream". Finish so no need to process other steps

So according to the spec in both cases the selected media type should be "application/octet-stream" and as I do not provide any MessageBodyWritter for this media type an exception should be thrown.

Am I wrong ?

jerseyrobot commented 7 years ago

@NicoNes Commented Hi,

Again I was doing few more tests about MediaType negotiation and I spotted another weird behavior inconsistent with the spec according to me.

Here is the case:

Application set up:

@Produces(MediaType.APPLICATION_JSON)
public class CustomJacksonJaxbJsonProvider extends com.fasterxml.jackson.jaxrs.json.JacksonJaxbJsonProvider{

}

Test case:

Lets call a non existing method using any HTTP client with Accept header set to application/xml.

Current behavior:

The custom ExceptionMapper will do its jobs and catch the NotFoundException and return the Message entity . Althought the incoming request Accept header is set to application/xml, the server will choose to use the CustomJacksonJaxbJsonProvider decicated to application/json to write the Message entity. So on the client side the message is JSON formatted however no Content-Type header is present.

Expected behavior:

The custom ExceptionMapper will do its jobs and catch the NotFoundException and return the Message entity . According to the spec I was expecting the server to use one of the Jersey XML providers (MessageBodyWritter) to write the Message entity. So on the client side the message will be XML formatted and Content-Type header set to application/xml.

Step by step the media type negotiation should be:

  1. Not applicable in our case

  2. Since we are dealing with a 404 there is no method and no class selected. So we have to select all MessageBodyWriter able to handle the Message entity class. Jersey XML providers XmlRootObjectJaxbProvider and my custom CustomJacksonJaxbJsonProvider should be selected among others since their are typed as MessageBodyWritter<Object>. So P will contains {...,"application/xml", "application/json", ....}

  3. P is not empty so P = {...,"application/xml", "application/json", ....}

  4. A is not empty so A = {"application/xml"}

  5. MediaType.valueOf("application/xml").isComptatible(MediaType.valueOf("application/xml")) returns true MediaType.valueOf("application/xml").isComptatible(MediaType.valueOf("application/json")) returns false so M = {"application/xml"} (the most specific).

    1. M is not empty so lets jump to step 7
  6. M.length = 1 so sort operation returns M = {"application/xml"}

  7. As M contains a concrete type the selected media type is Mselected = "application/xml". Finish so no need to process other steps