jakartaee / rest

Jakarta RESTful Web Services
Other
365 stars 121 forks source link

Inconsistencies in Jax-rs Client Implemenation wenn calling PUT with null value #632

Open rsoika opened 6 years ago

rsoika commented 6 years ago

I regognized inconsistencies between Jersey and RestEasy implementation of the Jax-RS Client code. The following code does not work in Jersey:

javax.ws.rs.clientClient client;
....
Response response = client.target(uri).request().put(null);

An IllegalStateException is thrown:

java.lang.IllegalStateException: Entity must not be null for http method PUT.
...  |  at org.glassfish.jersey.client.JerseyInvocation.validateHttpMethodAndEntity(JerseyInvocation.java:155)

The same code is accepted by RestEasy.

You may argue that a PUT method without a body does not make sense. But I don't think the jax-rs client implementation should try to educate the developer for good API. In the end, this behavior only limits the client.

And the following jax-rs resource method is valid for both implementations:

@PUT
@Path("/my-resource/{id}")
public Response backup(@PathParam("id") String id, @QueryParam("options") String options) {
      ....
}

Implementing the client code independent form one of these platforms (jersey/resteasy) forces the developer to create a dummy object:

Response response = client.target(uri).request().put(Entity.xml(""));

This call is now valid for both implementations.

I don't know how I can give a wise advice here as I am 'only' developing against the jax-rs api. But it would be great if both implementations have a similar behavior. Maybe this issue can be fixed in some way for the next version of the spec?

chkal commented 6 years ago

@rsoika Thanks for bringing this up.

I agree that the spec or the API docs should state whether null is allowed for the entity in the PUT case or not. I've mixed feelings about that. On the one hand using PUT without a body doesn't make sense from a semantic perspective. On the other hand I don't think that implementations should explicitly prevent the user from sending such requests.

Other thoughts?

mkarg commented 6 years ago

+1 for allowing null with PUT, as we should not artificially restrict the API. There might be valid use cases, even when we do not know them so far.

chkal commented 6 years ago

I just did a quick Google search and it looks like there are APIs allowing PUT with empty body like the Spotify API for Play/Resume.

Let me emphasize this again: There is no doubt that using PUT in this context is weird from the semantic perspective, but there are APIs out there implemented this way. And currently you cannot use the JAX-RS client API in a portable way to call such APIs.

spericas commented 6 years ago

I agree, I don’t see the value of restricting it at the API level.

rsoika commented 6 years ago

And we must see, that the same api allows the implementation of such kind of resource methods:

@PUT
@Path("/my-resource/{id}")
public Response backup(@PathParam("id") String id, @QueryParam("options") String options) {
      ....
}

Even if this looks weird. I have done this by myself (and I am not proud about it). But so the same api should not avoid me to call the method with its own client.

jansupol commented 6 years ago

This is a discussion that people had about the HTTP 1.1 RFC, I spent some time and there is quite a number of posts saying it's prohibited by the RFC to have an empty entity and there is a number of posts saying that it is (strictly) prohibited not.

Jersey allows to disable the validation by using ClientProperties.SUPPRESS_HTTP_COMPLIANCE_VALIDATION and then PUT null is allowed

mkarg commented 3 years ago

I think what the target of a cross-vendor API like JAX-RS should be is not restricting people synthetically, so I would say, let's agree that such checks are beneficial but should be disabled by default. Just had the same case again today and it is really annoying to have Jersey-specific options to be set in an otherwise Jersey-free source code.

mkarg commented 3 years ago

Off-Topic: Just tried Jersey 2.32, and this does not switch off the exception! Maybe a bug in Jersey not allowing to override properties at that late point?

    private static final Client CLIENT = ClientBuilder.newClient();
    static {
        CLIENT.property("jersey.config.client.suppressHttpComplianceValidation", true);
    }

Edit: Correcting myself. After spending another 30 minutes into Jersey's absurd intention to teach me something I cannot change anyways (as the server is a third-party product with a fixed API on which I have no control) I am really unnoyed to see that actually the above code is working, but that it clutters the log with WARNINGs... Is there a way to tell Jersey that such unwanted user-paternalism shall be logged with a level far lower than WARNING, actually?

spericas commented 3 years ago

Off-Topic: Just tried Jersey 2.32, and this does not switch off the exception! Maybe a bug in Jersey not allowing to override properties at that late point?

    private static final Client CLIENT = ClientBuilder.newClient();
    static {
        CLIENT.property("jersey.config.client.suppressHttpComplianceValidation", true);
    }

Edit: Correcting myself. After spending another 30 minutes into Jersey's absurd intention to teach me something I cannot change anyways (as the server is a third-party product with a fixed API on which I have no control) I am really unnoyed to see that actually the above code is working, but that it clutters the log with WARNINGs... Is there a way to tell Jersey that such unwanted user-paternalism shall be logged with a level far lower than WARNING, actually?

Perhaps by creating a Jersey issue and discussing this there instead of here? 😄

chkal commented 3 years ago

Back to the original discussion. It looks like @spericas, @mkarg and I agree that we should allow null entities in the client API. Anyone interested in providing a pull request? Maybe even @rsoika?