quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.34k stars 2.55k forks source link

Possibility to change MediaType using RestEasy Reactive #35206

Open DMaxter opened 11 months ago

DMaxter commented 11 months ago

Description

When using @Consumes("text/xml; charset=utf-8") for example, it gets converted to something likeMediaType("text", "xml", ["charset" -> "utf-8"]) and in the request, the header sent is Content-Type: text/xml;charset=utf-8.

For my use case I explicitly need to have text/xml; charset=utf-8 (note the space after the semicolon) and I can't find a way to override the header, because the parsing of the @Consumes annotation will trim the spaces.

If I add the @ClientHeaderParam annotation to the RestClient to override the Content-Type header, it doesn't seem to have any effect.

Using a ClientRequestFilter, by overriding the request headers of the received context also seems to have no effect

Finally, still using a ClientRequestFilter I am able to retrieve the media type, however I am not able to change it

P.S: Would be really useful if it could be available in Quarkus 2 as I wasn't able to migrate to Quarkus 3 yet

Implementation ideas

I think the easiest way to accomplish this is by creating a setter method on the ClientRequestContext for the MediaType annotation

quarkus-bot[bot] commented 11 months ago

/cc @FroMage (resteasy-reactive), @Sgitario (resteasy-reactive), @geoand (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

geoand commented 10 months ago

Are you looking for this in the REST Client? Also, why do you need the extra space?

Finally, what's the reason you can't migrate to Quarkus 3?

DMaxter commented 10 months ago

Are you looking for this in the REST Client?

I'm using RestEasy Reactive client.

why do you need the extra space?

I need the extra space because I'm issuing a request to an external service and if the space doesn't exist I get an HTTP 400 Bad Request. If there is one or more spaces the request gets through

what's the reason you can't migrate to Quarkus 3?

I haven't migrated yet because this new feature has higher priority and changing to Quarkus 3 will require a lot of rework due to the Hibernate changes

geoand commented 10 months ago

I need the extra space because I'm issuing a request to an external service and if the space doesn't exist I get an HTTP 400 Bad Request. If there is one or more spaces the request gets through

This sounds very odd, I've never heard of a system that is so strict (and I dare say incorrect) in the media type parsing...

@Sgitario do you think this is something that we can adress somehow?

gsmet commented 10 months ago

I had a look at various examples on the Internet and AFAICS I see a space after the ;. For instance on the W3C website here: https://www.w3.org/International/articles/http-charset/index.en.html .

So while the spec might allow not having a space, I wonder if we should add one if it's not too hard.

geoand commented 10 months ago

So while the spec might allow not having a space, I wonder if we should add one if it's not too hard.

+1

geoand commented 10 months ago

Actually, don't any of the methods described here work for you? Asking because I tried something like:

    @GET
    @Path("/whatever")
    @ClientHeaderParam(name = "Content-Type", value = "{calculateContentType}")
    byte[] call(@NotBody String usedForComputingContentType);

    default String calculateContentType(ComputedParamContext context) {
        return "application/json; param2=" + context.methodParameters().get(1).value();
    }

and it worked just fine - as in the content type contained the expected space.

However, I think that only works for Quarkus 3.

gsmet commented 10 months ago

I'm having a look at adding a space.

geoand commented 10 months ago

@gsmet where do you mean? Asking because as per my comment above, setting an arbitrary content type is already supported.

If you are saying that you are looking into always adding a space, I don't think we want to do that as I am fairly sure we would have the reverse breakage somewhere...

gsmet commented 10 months ago

Well, given all the examples I could find on the Internet in reference documentation (W3C, Mozilla DN typically) have a space, and apparently we have things in the wild expecting it, I'm not sure it's a good idea to not have one. Why would we be willing to take the risk of incompatibility?

This code apparently comes from RESTEasy (it's in MediaTypeHeaderDelegate).

@jamezp WDYT?

geoand commented 10 months ago

Given that it's been like this forever, I am not really willing to risk breaking in the other direction.

Furthermore, this is the first time we have heard of an incompatibility in 4+ years of Quarkus.

gsmet commented 10 months ago

I understand your point but IMHO our current implementation, while valid, is not in line with the principle of least astonishment.

Sure, most of the implementations out there follow the spec so will support our version too. But you always have obscure impls that are dumb as hell and just implement what's out there, and usually, that's the ones that are not maintained anymore and in which you can't fix anything.

Also remember we are not the most common framework out there so it's always to our disadvantage when something doesn't work out of the box when using Quarkus.

Obviously, it's not something that we would backport as it could break tests. And yes, I agree it's not ideal. But I'm not sure the decision is as easy.

Anyway, it's your component, so your decision.

geoand commented 10 months ago

Yeah, that makes complete sense.

IMHO, if we do provide a way to override (which in Quarkus 3 we do), that seems like a practical enough treatment of the problem.

DMaxter commented 10 months ago

Actually, don't any of the methods described here work for you? Asking because I tried something like:

    @GET
    @Path("/whatever")
    @ClientHeaderParam(name = "Content-Type", value = "{calculateContentType}")
    byte[] call(@NotBody String usedForComputingContentType);

    default String calculateContentType(ComputedParamContext context) {
        return "application/json; param2=" + context.methodParameters().get(1).value();
    }

and it worked just fine - as in the content type contained the expected space.

However, I think that only works for Quarkus 3.

I have tried it and in fact for Quarkus 2, it doesn't work

In the mean time I came up with a fix for this. Just created a proxy and all the traffic to the service passes through there, where the Content-Type is overridden. This only works for HTTP, however, I can live with that for now

geoand commented 10 months ago

I have tried it and in fact for Quarkus 2, it doesn't work

All the more reason to move to Quarkus 3 😎

DMaxter commented 10 months ago

Soon it will happen 😉

jamezp commented 10 months ago

Well, given all the examples I could find on the Internet in reference documentation (W3C, Mozilla DN typically) have a space, and apparently we have things in the wild expecting it, I'm not sure it's a good idea to not have one. Why would we be willing to take the risk of incompatibility?

This code apparently comes from RESTEasy (it's in MediaTypeHeaderDelegate).

@jamezp WDYT?

This is a good question. All the examples I see as well, example https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type, do have a space after the ;. It definitely makes me a little nervous to make this change as like @geoand said, it's been this way forever. Even the RFC shows it with the space.

Sgitario commented 10 months ago

Sorry to come up here too late. As far as I see from RFC, the following media types are equivalent and valid:

  text/html;charset=utf-8
  Text/HTML;Charset="utf-8"
  text/html; charset="utf-8"
  text/html;charset=UTF-8

I don't think it is worth taking the risk of changing the current format after having it in place for so long taking into account that is perfectly valid.

What I think is that we should respect what the users set. For example, having something like:

@GET
@Path("/whatever")
@Consumes("text/xml; charset=utf-8")
String get();

The media type should include the space.

Another option is to make it configurable, tho I'm not a big fan of adding another configuration.

FroMage commented 10 months ago

What I think is that we should respect what the users set

I think the problem happens because we parse it, perhaps? So we have to reconstruct it.

But yeah, we could keep the original string, and provided nobody's tweaked it (like using a filter to add an encoding or language) we could produce the original header.

Feels a bit weird to add non-spec-compliant compatibility logic for such an odd case. I wonder about the time required to implement and test this, and to maintain it.

An alternate solution would be to make sure that we can use @ClientHeaderParam(name = "Content-Type", value = "text/html;charset=utf-8;whateverelse") and not use @Consumes and it will Just Work™. That… feels like a valid use-case, no?

Unless we need the @Consumes annotation to select the body serialiser?

jamezp commented 10 months ago

+1 Since this is the first case I'm aware of where this is an issue, I'd hesitate to make a change like this.

I think the problem happens because we parse it, perhaps? So we have to reconstruct it.

Yes, this is true. It's parsed into a MediaType, then when produced for the header value it's reconstructed.