jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

Change default Content-Type to be application/json instead of application/octet-stream #1229

Closed t1 closed 3 months ago

t1 commented 3 months ago

The Jakarta REST spec defines the default content type to be application/octet-stream. This leads to the ridiculous situation that, at least on WildFly, clients get a 500 Internal Server Error if they don't request a content type by sending an Accept header, because WF doesn't provide a MessageBodyWriter for application/octet-stream. The common workaround is that people annotate all of their REST methods to @Produce(APPLICATION_JSON), which was intended to differentiate methods for the same path based on the type acceptable by the client... and rendering the whole MessageBodyWriter mechanism useless; e.g. even if you, or your runtime platform, would add a YamlMessageBodyWriter, your clients couldn't use it without you changing all of your applications. The same is true for clients sending entities without a Content-Type header, rendering the MessageBodyReader mechanism useless.

Simply changing the default to be application/json would make life as a developer much easier: this is by far the most commonly requested and often even implicitly expected Content-Type, and every runtime will provide for a MessageBodyReader/Writer. It would be a breaking change, though; but this is acceptable for a major version 4.0. And while it might be possible some people rely on the current default behaviour, I have never heard of anybody actually using application/octet-stream at all, not to speak of relying on this to be the default. It's not even defined what application/octet-stream means exactly; is it supposed to be Java Serialization? SRLY?!?

mkarg commented 3 months ago

I am not convinced that breaking backwards compatibility here is actually the best solution, as apparently this problem only exists on WildFly. Wouldn't it be much easier to (a) convince Wildfly to contain an MBW for octet-stream or (b) simply put such on the class-path / add it to your application or (c) add a container request filter that simply sets a default content-type header? At least the last one is done in five minutes, without breaking anybody's code.

jansupol commented 3 months ago

WF doesn't provide a MessageBodyWriter for application/octet-stream

I wanted to leave this up to the WF guys... WF, as well as any other Jakarta REST implementation does have MessageBodyWriter for application/octet-stream, depending on the Java entity type. The Specification requires the MessageBodyWriter for application/octet-stream for:

It also is not a good practice not to use a well-defined Content-Type, @Consumes and @Produces in a production application.

jamezp commented 3 months ago

@t1 What version of WildFly are you using? Do you have an example method because WildFly definitely includes a MessageBodyWriter for application/octet-stream.

mkarg commented 3 months ago

Jan,

regarding your last sentence I need to say that I do actively encourage people to NOT use @Consumes in resource methods, but to instead make up their mind about what set of MIME types they want to support and then either stick with the default ones OR actively bundle a JAX-RS Extension that provides exactly one that is wanted. I see @Consumes on resource methods as nonsense, as it restricts the method to that MIME type, while it should be up to the runtime to decide from the set of bundled providers.

-Markus

Von: jansupol @.*** Gesendet: Donnerstag, 7. März 2024 14:10 An: jakartaee/rest Cc: Markus KARG; Comment Betreff: Re: [jakartaee/rest] Change default Content-Type to be application/json instead of application/octet-stream (Issue #1229)

WF doesn't provide a MessageBodyWriter for application/octet-stream

I wanted to leave this up to the WF guys... WF, as well as any other Jakarta REST implementation does have MessageBodyWriter for application/octet-stream, depending on the Java entity type. The Specification requires https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1#standard_entity_providers the MessageBodyWriter for application/octet-stream for:

It also is not a good practice not to use a well-defined Content-Type, @Consumes and @Produces in a production application.

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/issues/1229#issuecomment-1983474135 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM7PNZKATOX5PRGFR2AYTTYXBRLPAVCNFSM6AAAAABEIQRJDKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBTGQ3TIMJTGU . You are receiving this because you commented. https://github.com/notifications/beacon/AAM7PNZ6XBLOXSKPNSYTJ4LYXBRLPA5CNFSM6AAAAABEIQRJDKWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTWHFU5O.gif Message ID: @.***>

t1 commented 3 months ago

I'll have to adapt and clarify my suggestion: it makes perfect sense to use application/octet-stream for the types the spec requires them for (@jansupol listed them) and that default should stay as it is. But when I return my domain model type, e.g. Product, it's just undefined, what that octet stream should include. In this case, the default should become application/json.

jamezp commented 3 months ago

One option might be to support @Consumes and @Produces on a jakarta.ws.rs.core.Application which would be the default for the application and it's resources.

t1 commented 3 months ago

One option might be to support @Consumes and @Produces on a jakarta.ws.rs.core.Application which would be the default for the application and it's resources.

That still would prevent supporting other content types by simply adding a new MessageBodyReader/Writer to your application or to the runtime environment.

NicoNes commented 3 months ago

That still would prevent supporting other content types by simply adding a new MessageBodyReader/Writer to your application or to the runtime environment.

According to the specification, if you do not annotate your class or method with @Produces to tell the set of media type you want to support for this method then the response media type dertemination mechanism takes all MessageBodyWriter available at runtime into consideration.

So if your resource class or method is not annotated with @Produces, the client does not set no Accept header, and a MessageBodyWriter able to write your domain object e.g. Product exists at runtime, then it must be used.

Simply changing the default to be application/json would make life as a developer much easier: this is by far the most commonly requested and often even implicitly expected Content-Type, and every runtime will provide for a MessageBodyReader/Writer.

I agree that application/json would be a better choice as a media type to use when response media type dertemination mechanism does not return a concrete media-type.

t1 commented 3 months ago

So if your resource class or method is not annotated with @Produces, the client does not set no Accept header, and a MessageBodyWriter able to write your domain object e.g. Product exists at runtime, then it must be used.

The generic MessageBodyWriters are just fine. I don't want to write specific MessageBodyWriters for all my domain objects. I want things to work OOTB! And everything works already just fine, as long as the client says what they want. Only this somewhat strange default forces me to do extra steps.

If I return a byte[], java.io.File, etc., I'd better specify what it actually contains. It could be a JSON or an image or whatever. application/octet-stream is not very helpful for a client to find out what this is supposed to be. But in this case, this is clearly my responsibility as an application developer. If I simply return or accept my domain object, the MessageBodyWriters/Readers are a super elegant mechanism... I just can't think of any way to improve on that!

NicoNes commented 3 months ago

That still would prevent supporting other content types by simply adding a new MessageBodyReader/Writer to your application or to the runtime environment.

I was just answering to this specific part to say that nothing in the spec prevent supporting other media-type by only adding MessageBodyReader/Writer (hand made or not) to the application or runtime environment.

And everything works already just fine, as long as the client says what they want.

If you are in WF, I assume that artifact resteasy-jackson2-provider is provided by default and thus MBW ResteasyJackson2Provider exists at runtime. So according to the specification:

For what I saw, RESTEasy does not stricly follow the spec at step 2 and that's why the response media type determination mechanism returns application/octet-stream instead of application/json.
That also the reason why the client is forced to specify a compatbile Accept header to make step 2 work as expected. I let @jamezp confirm my analysys.

So if my analysys is right your current issue is more an implementation issue. But Apart of that possible implementation issue, I agree with you that application/json would be a better default nowadays.

-- Nicolas

spericas commented 3 months ago

I think Nicolas is correct, as long as the response entity type is “handled” by the MBW.

— Santiago

On Mar 10, 2024, at 1:13 PM, NicoNes @.***> wrote:

That still would prevent supporting other content types by simply adding a new MessageBodyReader/Writer to your application or to the runtime environment.

I was just answering to this specific part to say that nothing in the spec prevent supporting other media-type by only adding MessageBodyReader/Writer (hand made or not) to the application or runtime environment.

And everything works already just fine, as long as the client says what they want.

If you are in WF, I assume that artifact resteasy-jackson2-provider is provided by defaulthttps://urldefense.com/v3/__https://docs.jboss.org/resteasy/docs/4.4.2.Final/userguide/html/json.html*d4e1347__;Iw!!ACWV5N9M2RV99hQ!IJfBr66PU0QOhgpuTFFTOenOMLyin9HvD31lDDvpaLazaaBUrMwL8RrFJkr5A0k2jJP0o-sHNLkJNiGV1dC5H6wTBhLY2lNXxfA$ and thus MBW ResteasyJackson2Provider https://urldefense.com/v3/__https://github.com/resteasy/resteasy/blob/main/providers/jackson2/src/main/java/org/jboss/resteasy/plugins/providers/jackson/ResteasyJackson2Provider.java__;!!ACWV5N9M2RV99hQ!IJfBr66PU0QOhgpuTFFTOenOMLyin9HvD31lDDvpaLazaaBUrMwL8RrFJkr5A0k2jJP0o-sHNLkJNiGV1dC5H6wTBhLY68nSXtc$ exists at runtime. So according to the specificationhttps://urldefense.com/v3/__https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1*determine_response_type__;Iw!!ACWV5N9M2RV99hQ!IJfBr66PU0QOhgpuTFFTOenOMLyin9HvD31lDDvpaLazaaBUrMwL8RrFJkr5A0k2jJP0o-sHNLkJNiGV1dC5H6wTBhLYtaK0-60$:

For what I saw, RESTEasy does not stricly follow the spec at step 2 and that's why the response media type determination mechanism returns application/octet-stream instead of application/json. That also the reason why the client is forced to specify a compatbile Accept header to make step 2 work as expected. I let @jamezphttps://urldefense.com/v3/__https://github.com/jamezp__;!!ACWV5N9M2RV99hQ!IJfBr66PU0QOhgpuTFFTOenOMLyin9HvD31lDDvpaLazaaBUrMwL8RrFJkr5A0k2jJP0o-sHNLkJNiGV1dC5H6wTBhLYDCoJhn0$ confirm my analysys.

So if my analysys is right your current issue is more an implementation issue. But Apart of that possible implementation issue, I agree with you that application/json would be a better default nowadays.

-- Nicolas

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/jakartaee/rest/issues/1229*issuecomment-1987298416__;Iw!!ACWV5N9M2RV99hQ!IJfBr66PU0QOhgpuTFFTOenOMLyin9HvD31lDDvpaLazaaBUrMwL8RrFJkr5A0k2jJP0o-sHNLkJNiGV1dC5H6wTBhLY6JxppSs$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABKT2N5C6TGETEFITLHJYZDYXSID5AVCNFSM6AAAAABEIQRJDKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXGI4TQNBRGY__;!!ACWV5N9M2RV99hQ!IJfBr66PU0QOhgpuTFFTOenOMLyin9HvD31lDDvpaLazaaBUrMwL8RrFJkr5A0k2jJP0o-sHNLkJNiGV1dC5H6wTBhLYn9RFNBU$. You are receiving this because you are subscribed to this thread.Message ID: @.***>

t1 commented 3 months ago

Oh... okay, cool. I never studied these algorithms before, because they work exactly how I expect it. TIL. So this is a bug in RestEasy/WildFly. One could argue that there should be a TCK confirming this.

jamezp commented 3 months ago

And everything works already just fine, as long as the client says what they want.

If you are in WF, I assume that artifact resteasy-jackson2-provider is provided by default and thus MBW ResteasyJackson2Provider exists at runtime. So according to the specification:

FWIW in WildFly, Jakarta JSON Binding is used by default. Jackson can be enabled, but it's not the default.

  • at the end of step 2, since neither the resource class nor method is annotatetd with Produces, then "P" would contain at least all media-types defined by ResteasyJackson2Provider : application/json, application/*+json, text/json
  • at the end of step 4, since client does not define no Accept headers, "A" would be: */*
  • at the end of step 5 "M" would be: application/json, application/*+json, text/json
  • the response media type determintation should end at step 8 with: application/json

For what I saw, RESTEasy does not stricly follow the spec at step 2 and that's why the response media type determination mechanism returns application/octet-stream instead of application/json. That also the reason why the client is forced to specify a compatbile Accept header to make step 2 work as expected. I let @jamezp confirm my analysys.

So if my analysys is right your current issue is more an implementation issue. But Apart of that possible implementation issue, I agree with you that application/json would be a better default nowadays.

-- Nicolas

If anyone has an example reproducer that would be helpful :) Feel free to file an issue at https://issues.redhat.com/browse/RESTEASY and I can have a look.

jamezp commented 3 months ago

Oh... okay, cool. I never studied these algorithms before, because they work exactly how I expect it. TIL. So this is a bug in RestEasy/WildFly. One could argue that there should be a TCK confirming this.

Please feel free to file an issue at https://issues.redhat.com/browse/RESTEASY. I'm happy to take a look at it.

t1 commented 3 months ago

I've created a discussion at RestEasy.

I would close this issue, but maybe someone has an idea on how to add this to the TCK?

t1 commented 3 months ago

@jamezp : sorry, didn't read your comment in time. Should I move the discussion to the RESTEASY ticket queue?

jamezp commented 3 months ago

@jamezp : sorry, didn't read your comment in time. Should I move the discussion to the RESTEASY ticket queue?

No, a discussion is totally fine. I can file an issue for it later :)

t1 commented 3 months ago

The only thing left open is, if we need new tests in the TCK. Should I rename this issue or close it and create a new one? Or is it too unimportant? I'm fine with all options.

jansupol commented 3 months ago

I agree that application/json would be a better choice as a media type to use when response media type dertemination mechanism does not return a concrete media-type.

I remember back in JAX-RS 2.1 days when JSON-B was added as an automatic serialize-all for JSON, there was a (security) argument that people do not want automatically all internal data to be serialized to a client in the case they forget to add their MBW (as JSON-B serializes all private fields). So the customers were forced to "select" JSON-B at least by media type. I am not sure how valid is that argument nowadays when Jakarta users are more accustomed to JSON-B.

mkarg commented 3 months ago

I would say the argument still is valid.

We also need to understand and accept that JAX-RS is just a foundational API but not a full-blown ready-to-use framework product, so it is pretty ok that the API leaves out some convenience things that app frameworks can add on their own.

Some years back XML was the lingua franca and we did the mistake to hard-code it into JAX-RS, now we had to remove that hard link, resulting in backwards incompatibilities. We should not repeat the same mistake. Who knows if in some years JSON is outdated and people all switch to protobuf etc.? So I am -1 for changing the default just "for fashion" every other season.

t1 commented 3 months ago

(security) argument... the customers were forced to "select" JSON-B at least by media type.

Emphasis on "customers", i.e. the consumers of the API? They should generally be considered untrustworthy, so they should not be allowed to "select" unsafe things. I don't follow that line of reasoning.

JAX-RS is just a foundational API but not a full-blown ready-to-use framework product

It serves me perfectly as a full-blown ready-to-use framework product. There is only one feature left that I'd like to be added: Problem-Details, but we discussed that otherwise.

So I am -1 for changing the default just "for fashion" every other season.

After learning a lot here, I completely agree. Only the TCK could check those specified algorithms more thoroughly 😁

mkarg commented 3 months ago

So I am -1 for changing the default just "for fashion" every other season.

After learning a lot here, I completely agree. Only the TCK could check those specified algorithms more thoroughly 😁

So does this imply that we can close this issue?

t1 commented 3 months ago

As there seems to be not much interest in adding this to the TCK, I'm closing this. Thanks a lot!