Correct handling of JAX-RS requests without an Accept header

[t1]

I'm working mostly on WildFly (actually all the way back to since JBoss 4.0.2; currently 31.0.1). I generally let my JAX-RS services directly return instances of my own domain type, e.g. Product. If one of my clients doesn't specify which Content-Type they are ready to Accept, they get a 500 Internal Server Error. IIUC, this is because RestEasy determines that it should return an application/octet-stream, but there is no MessageBodyWriter that could produce that from my domain type (only for byte[], etc.).

I see many people work around this by annotating all of their resource methods with @Produces(APPLICATION_JSON). But IIUC, this mechanism was originally intended for when you have multiple methods for the same path returning a String (et.al.). The @Produces annotation can then be used to pick the method matching best to the Accept header sent by the client. This is not only extra work, it also would prevent using a new MessageBodyWriter for a new Content-Type, e.g. application/yaml.

For a long time, I had assumed this to be the result of the spec defining application/octet-stream to be the fallback. But I just found out in an JAX-RS issue, that the specified algorithm is not implemented correctly in RestEasy: As JsonBindingProvider (and ResteasyJackson2Provider, and JAXBXmlRootElementProvider, and probably some more) can produce a JSON (et.al.) from my Product. So the set of potential producers P found in step 2 should contain them all. The set of acceptable content types A determined in step 4 has to be */*. Then step 8 should match and one of those types (preferably application/json) should be picked; step 9 should never be reached.

I assume that the corresponding issue exists on the @Consumes side.

I would very much appreciate it if this could be fixed and make my life easier. Thanks!

P.S.: I still wonder why this is not part of the TCK.

[jamezp]

I will see if I can create a reproducer for this. Thank you for reporting this @t1

[t1]

@NicoNes: I must admit, you're right 😁 So the problem must be somewhere in the implementation of this algorithm-pair, not in the MBWs themselves.

[t1]

@NicoNes: Even in the latest WildFly 33 beta, I'm still forced to annotate all of my REST controllers with @Produces(APPLICATION_JSON). Is there any way I can help to resolve this?

[jamezp]

I'll make this a task to look at next week again. I'm sorry that it fell further down my list than I expected it to.

[jamezp]

Again, I'm so sorry for the long delay on this. I really didn't mean to push this off so much. I've re-read everything and the spec and I'm not sure I completely agree that what RESTEasy is doing, is wrong. Let me explain :) Where, I think, we all agree this failure is coming from:

Else set P={V(writers)} where writers is the set of MessageBodyWriter that support the class of the returned entity object.

I don't think the issue is in

resteasy/resteasy-core/src/main/java/org/jboss/resteasy/core/ServerResponseWriter.java

Line 341 in 701c5fc

Map<MessageBodyWriter<?>, Class<?>> mbws = providerFactory.getPossibleMessageBodyWritersMap(type, generic,

. The reason being is that MBW.isWritable() is returning false there for all the built-in MBW's. The reason, for example, the org.jboss.resteasy.plugins.providers.jsonb.JsonBindingProvider is returning false, is because it specifically checks in incoming media type.

There could be an argument it should return true on */*. However, I don't really see how it could determine that the type passed in is serializable to JSON. Possibly for Jakarta JSON Binding this is less of an issue because I'm not sure what types it won't support.

That said, to me the issue is really in the MessageBodyWriter implementations. The MBW should return true if it can write the type.

Please do let me know if you feel I'm way off base.

FWIW Jakarta XML Binding is deprecated for Jakarta EE 11 and no longer required for the Jakarta REST 4.0 specification. RESTEasy and WildFly definitely still plan on supporting XML though.

[jamezp]

As an example, IMO this is how it should be fixed main...jamezp:resteasy:update-object-mbws. Note with this specific change, the chosen type is application/json.

If we all agree this seems correct, I'll file an issue and get a fix up tomorrow. I plan on doing a new release this week for 6.2.10.Final to get into WildFly 33.0.1.Final which will be released next week.

[jamezp]

mediaType = mediaType == null ? MediaType.APPLICATION_OCTET_STREAM_TYPE : mediaType;

I'm not sure I follow this. I don't see a InboundMessageContext in RESTEasy.

[t1]

That seems to be from opensaml. Sorry for the confusion.

[NicoNes]

Hey @jamezp,

Well I think @t1 also already proposed to fix MBW isWriteable(...) method to support both */* and application/* as you did. My answer to that was it is going against the intented use of the isWriteable(...) method. According to the spec this method is never supposed to be called with a non concrete media type. Here is my previous answer about it:

Well, the intented use of isWritable(....) is described later in this spec section where you can see that:

- first at step 2 we have to determine the media-type of the response
- then at step 5 we have to call isWriteable with this media-type on each MBW.

This step 2 redirect us to our algorithm and as you know, this algorithm will always return a concrete media-type. It well never return the non concrete media-types */* or application/* (see step 8 and step 9).
So back at step 5 of this algorithm, isWritable(....) will never be invoked with the non concrete media-types */* or application/*.
Contrary to what you said, if client request */* (or does not specify no Accept header) and our algorithm fails to determine the response media-type, then the choosen media-type will be the default one application/octet-stream. Thus at step 5 of this this algorithm, isWritable(....) will be invoked with application/octet-stream not */*.

So based on this intented use of isWritable(....), there is no need to add an extra constraint on MBW developpers by requiring them to also support the non concrete media-type application/* or */* in their isWritable(...) implementation.

We all agree this failure is coming from:

Else set P={V(writers)} where writers is the set of MessageBodyWriter that support the class of the returned entity object.

But as I said before according to my understanding of the spec:

The goal of this point is to extract the media-types of all the MBWs able to write the returned entity object regardless of the media-types requested by the client.
In other words, it about knowing the media-types that can be used by the JAKARTA-RS application to write the returned entity object, regardless of the media-types requested by the client.
(The negociation between those media-types and those requested by the client is done later from step 5 to 10 and does not rely on isWriteable(...) method).

So I still think that this point shoudl be implemented as I described it previously (see below). Why does it seems wrong to you ?

Let's suppose that I have both JsonBindingProvider and JAXBElementProvider in my classpath and the returned entity object is of type Product not annotated with @XMLRootElement.
Basically, the implementation of the last bullet point of step 2 should be :

Select all MBW from classpath => JsonBindingProvider and JAXBElementProvider
Extract all media-types produced by JsonBindingProvider => application/json, application/*+json, text/json
For each of these media-types check if JsonBindingProvider can write Product and if true add this media-type to P :
    JsonBindingProvider.isWritable(Product.class,...,MediaType.valueOf(application/json) is true=> P={application/json}
    JsonBindingProvider.isWritable(Product.class,...,MediaType.valueOf(application/*+json) is true=>P={application/json, application/*+json}
    JsonBindingProvider.isWritable(Product.class,...,MediaType.valueOf(text/json) is true=> P={application/json, application/*+json, text/json}
Extract all media-types produced by JAXBElementProvider => application/xml, application/*+xml, text/xml, text/*+xml
For each of these media-types check if JAXBElementProvider can write Product and if true add this media-type to P :
    JAXBElementProvider.isWritable(Product.class,...,MediaType.valueOf(application/xml) is false because Product is not annotated with @XMLRootElement => P={application/json, application/*+json, text/json}
    JAXBElementProvider.isWritable(Product.class,...,MediaType.valueOf(application/*+xml) is false because Product is not annotated with @XMLRootElement => P={application/json, application/*+json, text/json}
    JAXBElementProvider.isWritable(Product.class,...,MediaType.valueOf(text/xml) is false because Product is not annotated with @XMLRootElement => P={application/json, application/*+json, text/json}
    JAXBElementProvider.isWritable(Product.class,...,MediaType.valueOf(text/+xml) is false because Product is not annotated with @XMLRootElement => P={application/json, application/*+json, text/json}
P={application/json, application/*+json, text/json}

So at the end of step 2 we know for sure that our application can write Prodcut in either application/json, application/*+json, text/json.
The other steps left after step 2 are about negotiating between those media-types and those requested by the client to select the best one that matches the client request.

-- Nicolas

[jamezp]

I've been doing a lot of testing and reading on this today. I'm still not sure how this could work. Given you're above exception where P={application/json, application/*+json, text/json, */*, text/plain} for a return type of java.lang.String, the chosen type would be application/json even though we're returning a String and it likely should be text/plain.

The problem is when accept is, or effectively is, */* then a.isCompatible(p) will always be true. This results all the types being added to M and application/json being chosen.

Granted, maybe this could still be considered a RESTEasy bug. However, it would be a rather large refactor and possibly a change in current behavior.

The simplest option I see would be to add a configuration parameter that could be set so when no MediaType can be resolved, it uses the defined MediaType as a default.

FWIW, these are my changes I've tested today main...jamezp:update-object-mbws

[NicoNes]

I've been doing a lot of testing and reading on this today. I'm still not sure how this could work. Given you're above exception where P={application/json, application/*+json, text/json, /, text/plain} for a return type of java.lang.String, the chosen type would be application/json even though we're returning a String and it likely should be text/plain.

The problem is when accept is, or effectively is, / then a.isCompatible(p) will always be true. This results all the types being added to M and application/json being chosen.

I don't undertsand your concern about this. I mean, if P contains application/json, it means that there is a MBW able to write an instance of java.lang.String into application/json. So if accept header is, or effectively is, */* then why the choosen type should be text/plain rather than application/json or any concrete type of P ?

Granted, maybe this could still be considered a RESTEasy bug. However, it would be a rather large refactor and possibly a change in current behavior.

The simplest option I see would be to add a configuration parameter that could be set so when no MediaType can be resolved, it uses the defined MediaType as a default.

I agree with you that it is a large refactor that will lead to change in behavior for sure. However, if we assume there is a bug compared to what is written in the specification, then I think we should fix it and let this new behavior become the default one. We could use a configuration parameter to allow users to switch on the old behavior if they want to. WDYT ?

FWIW, these are my changes I've tested today main...jamezp:update-object-mbws

I checkout your changes and they looks good to me. I made little changes on it here : https://github.com/resteasy/resteasy/compare/main...NicoNes:Resteasy:update-object-mbws?expand=1

• UndefinedMediaTypeMessageBodyWriterTest.noAcceptString() => Expected media type is MediaType.APPLICATION_JSON_TYPE instead of MediaType.APPLICATION_OCTET_STREAM_TYPE
• UndefinedMediaTypeMessageBodyWriterTest.noAcceptByte() => Expected media type is MediaType.APPLICATION_JSON_TYPE instead of MediaType.APPLICATION_OCTET_STREAM_TYPE
• ResteasyProviderFactory.getSupportedMediaTypes(...) => Return a Set<MediaType> instead of a Map<MediaType, Class<?>> because the values of the Map are not really used. More, because of the way they are added to the Map, some media types are filtered out (see UndefinedMediaTypeMessageBodyWriterTest.checkGetSupportedMediaTypes where text/plain media type was filtered out).
• ServerResponseWriter => try to handle SegmentNode.RESTEASY_SERVER_HAS_PRODUCES as it was before the changes

-- Nicolas

[jamezp]

I don't undertsand your concern about this. I mean, if P contains application/json, it means that there is a MBW able to write an instance of java.lang.String into application/json. So if accept header is, or effectively is, / then why the choosen type should be text/plain rather than application/json or any concrete type of P ?

The problem might be in choosing the correct MBW TBH. What I was seeing was the StringTextStart appears to have been selected so it wasn't valid JSON being written. TBH for a java.lang.String, I'm not sure how it would be written in JSON :) I guess I should look at that.

Also, your changes may have fixed that. I'm definitely going to have a look your changes and test them. Thank you very much!

[NicoNes]

I did the same observation than you in the choosing of the MBW and my changes did not modify it.

• For unit test UndefinedMediaTypeMessageBodyWriterTest.noAcceptString() the selected media type is application/json and the MBW used to write the entity is StringTextStar because providers.getMessageBodyWriter(String.class, String.class,, null, MediaType.APPLICATION_JSON_TYPE) returns it.
• For unit test UndefinedMediaTypeMessageBodyWriterTest.noAcceptByte() the selected media type is application/json and the MBW used to write the entity is ByteArrayProvider because providers.getMessageBodyWriter(byte[].class, byte[].class,, null, MediaType.APPLICATION_JSON_TYPE) returns it.

I also wonder if a java.lang.String like Test entity was a valid JSON and it seems to be valid : https://stackoverflow.com/questions/7487869/is-this-simple-string-considered-valid-json. FWIW both StringTextStar and JsonBindingProvider generate the same output for Test entity String so I guess java.lang.String is really valid JSON.
If we consider that java.lang.String is not a valid JSON then maybe we should have to review the StringTextStar MBW to prevent it writing application/json

-- Nicolas

[t1]

"Test entity" is perfectly valid JSON... it's just not a JSON Object, but simply a plain JSON String. Note the quotes, and StringTextStar seems to just write the string, without quotes, and without escaping contained quotes. But JsonBindingProvider uses JSON-B, so it should have the surrounding as well as escaped quotes.

[t1]

I also just re-read the algorithms and your comments and I was wrong on several occasions. But I think, once you found it, the solution is actually rather simple: let me point you again to step 2. of the inner algorithm for Determining the MediaType of Responses. As we assume that there is no @Produces annotation on our example @GET method, nor on the class, then the last bullet point is relevant: "Else set 𝑃={𝑉(writers)} where writers is the set of MessageBodyWriter that support the class of the returned entity object." The RestEasy JsonBindingProvider supports Object, so the values it declares in the @Produces annotation are in this set P: application/json, application/*+json, and text/json.

In step 8., it says: "If 𝑚 is a concrete type, set 𝑀selected=𝑚, finish." Assuming there is no other MBW that can produce something with a higher priority, the selected media type must be application/json: it's the first, and it's concrete (I understand application/*+json would not be considered "concrete", but that term is not defined in the spec, is it?).

The outer algorithm for the Message Body Writer seems to be implemented correctly. And I was wrong to suggest to change the writer. I understand (now) that it actually must not handle */*! If it would, the container wouldn't know what type that actually is, and could only return */* as Content-Type... clearly unsatisfying to the client.

For, e.g., a Product return type, this works perfectly straight forward. For a String return type, e.g. a method that returns only the description of a product, it's okay to return it as application/json with the body containing only a JSON string. If the client doesn't like that encoding, it must say that it wants, e.g. text/plain.

If we all agree on this, I could provide an additional pair of eyes to look into the code.

I think a config parameter should really only be our last option.

[jamezp]

@t1 I do think we pretty much all agree. I think we will need a configuration parameter to revert to the current behavior. I'm pretty sure some people might be relying on it.

I do need to get a 6.2.10.Final out so I can get it integrated into WildFly 33.0.1.Final which is set to be released on 2024-08-22. I'm not sure I'd be comfortable delaying a release for this, but we could consider it.

[jamezp]

One concern I have with these changes is there are 18 test failures.

Failed Tests:
org.jboss.resteasy.test.core.spi.ResourceClassProcessorBasicTest
    interfaceTest - Time elapsed: 0.004 [FAILED]
        Reason: expected: <true> but was: <false>
Total failed for org.jboss.resteasy.test.core.spi.ResourceClassProcessorBasicTest: 1
org.jboss.resteasy.test.providers.disabled.DisabledProvidersTest
    testBuiltinProviderDisabled - Time elapsed: 0.004 [FAILED]
        Reason: Expected a status of 500 got 200: "string" ==> expected: <500> but was: <200>
Total failed for org.jboss.resteasy.test.providers.disabled.DisabledProvidersTest: 1
org.jboss.resteasy.test.providers.jaxb.BadContentTypeTest
    testBadRequestAfterHtmlError - Time elapsed: 0.004 [FAILED]
        Reason: The returned response status is not the expected one ==> expected: <500> but was: <406>
    testHtmlError - Time elapsed: 0.005 [FAILED]
        Reason: The returned response status is not the expected one ==> expected: <500> but was: <406>
Total failed for org.jboss.resteasy.test.providers.jaxb.BadContentTypeTest: 2
org.jboss.resteasy.test.providers.mediatype.BlacklistedMediaTypeTest
    testByteArrayProvider - Time elapsed: 0.004 [FAILED]
        Reason: expected: <200> but was: <500>
    testDataSourceProvider - Time elapsed: 0.002 [FAILED]
        Reason: expected: <200> but was: <500>
    testEnvelopedWriter - Time elapsed: 0.002 [FAILED]
        Reason: expected: <200> but was: <500>
    testFileProvider - Time elapsed: 0.002 [FAILED]
        Reason: expected: <200> but was: <500>
    testFileRangeWriter - Time elapsed: 0.002 [FAILED]
        Reason: expected: <200> but was: <500>
    testInputStreamProvider - Time elapsed: 0.002 [FAILED]
        Reason: expected: <200> but was: <500>
    testReaderProvider - Time elapsed: 0.002 [FAILED]
        Reason: expected: <200> but was: <500>
    testStreamingOutputProvider - Time elapsed: 0.002 [FAILED]
        Reason: expected: <200> but was: <500>
    testStringTextStar - Time elapsed: 0.002 [FAILED]
        Reason: expected: <200> but was: <500>
Total failed for org.jboss.resteasy.test.providers.mediatype.BlacklistedMediaTypeTest: 9
org.jboss.resteasy.test.providers.noproduces.ProviderWithNoProducesTest
    testWriteFoo - Time elapsed: 0.006 [FAILED]
        Reason: expected: <200> but was: <500>
Total failed for org.jboss.resteasy.test.providers.noproduces.ProviderWithNoProducesTest: 1
org.jboss.resteasy.test.resource.basic.MediaTypeNegotiationTest
    testAcceptStarXml - Time elapsed: 0.002 [FAILED]
        Reason: expected: <406> but was: <500>
Total failed for org.jboss.resteasy.test.resource.basic.MediaTypeNegotiationTest: 1
org.jboss.resteasy.test.resource.basic.ResourceLocatorTest
    testAnnotationFreeSubresource - Time elapsed: 0.003 [FAILED]
        Reason: expected: <text/plain;charset=UTF-8> but was: <application/json>
Total failed for org.jboss.resteasy.test.resource.basic.ResourceLocatorTest: 1
org.jboss.resteasy.test.resource.basic.SubResourceLocatorTest
    test657 - Time elapsed: 0.007 [FAILED]
        Reason: Wrong response content ==> expected: <{}> but was: <bill>
Total failed for org.jboss.resteasy.test.resource.basic.SubResourceLocatorTest: 1
org.jboss.resteasy.test.resource.path.ResourceMatchingTest
    testNoProduces - Time elapsed: 0.001 [FAILED]
        Reason: expected: <application/octet-stream> but was: <application/json>
Total failed for org.jboss.resteasy.test.resource.path.ResourceMatchingTest: 1
Total failed: 18

No ERROR tests found.


*******************************************************************
Total: 3379 - PASSED: 3326 - FAILED: 18 - ERRORS: 0 - SKIPPED: 35
*******************************************************************

I've not looked at each one yet, but I never like changing tests for bug fixes if possible :)

[NicoNes]

Yes I also notice all those tests failing and I started to try fixing few of them.

For what I saw yet, few are failling even if the feature under test has nothing to do with the behavior we are tying to fix. So fixing those tests is just about making them more "accurate" to only focus on the feature that is under test.

• For example this test ResourceClassProcessorBasicTest.interfaceTest could be fixed by adding @Produces(MediaType.APPLICATION_XML) on the interface ResourceClassProcessorProxy.
• This one DisabledProvidersTest.testBuiltinProviderDisabled could be fixed by adding org.jboss.resteasy.plugins.providers.jsonb.JsonBindingProvider to the resteasy.disable.providers property.

I will try to continue the test review but I already suppose that there are few tests for the current behavior we fixed. I think those tests will have to be changed to review what is under test.

-- Nicolas

[jamezp]

I've filed https://issues.redhat.com/browse/RESTEASY-3538 for this. I think it's going to take a little time to do as I think we might need to do a fairly large refactor to get this right. The current changes I've got in my testing branch are not correct.

[jamezp]

Here is the initial look at the changes, #4302. It's not at all complete as there are a bunch of TODO's I need to address and documentation needs to be updated. However, I wanted to get some eyes on it.