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

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

Closed
t1 opened this issue Mar 6, 2024 · 23 comments

Comments

@t1
Copy link

t1 commented Mar 6, 2024

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
Copy link
Contributor

mkarg commented Mar 6, 2024

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
Copy link
Contributor

jansupol commented Mar 7, 2024

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:

  • byte[]
  • java.lang.String
  • java.io.InputStream
  • java.io.Reader
  • java.io.File
  • jakarta.activation.DataSource
  • StreamingOutput

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

@jamezp
Copy link
Contributor

jamezp commented Mar 7, 2024

@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
Copy link
Contributor

mkarg commented Mar 7, 2024 via email

@t1
Copy link
Author

t1 commented Mar 8, 2024

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
Copy link
Contributor

jamezp commented Mar 8, 2024

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
Copy link
Author

t1 commented Mar 9, 2024

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
Copy link
Contributor

NicoNes commented Mar 9, 2024

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
Copy link
Author

t1 commented Mar 10, 2024

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
Copy link
Contributor

NicoNes commented Mar 10, 2024

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:

  • 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

@spericas
Copy link
Contributor

spericas commented Mar 11, 2024 via email

@t1
Copy link
Author

t1 commented Mar 11, 2024

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
Copy link
Contributor

jamezp commented Mar 11, 2024

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
Copy link
Contributor

jamezp commented Mar 11, 2024

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
Copy link
Author

t1 commented Mar 11, 2024

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
Copy link
Author

t1 commented Mar 11, 2024

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

@jamezp
Copy link
Contributor

jamezp commented Mar 11, 2024

@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
Copy link
Author

t1 commented Mar 13, 2024

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
Copy link
Contributor

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
Copy link
Contributor

mkarg commented Mar 13, 2024

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
Copy link
Author

t1 commented Mar 14, 2024

(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
Copy link
Contributor

mkarg commented Mar 16, 2024

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
Copy link
Author

t1 commented Mar 18, 2024

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

@t1 t1 closed this as completed Mar 18, 2024
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

6 participants