jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

Clarification: Reconfiguration of Client #1271

Open mkarg opened 2 weeks ago

mkarg commented 2 weeks ago

A JAX-RS Client can be configured like this (example taken from JavaDocs):

 Client client = ClientBuilder.newClient();

 client.property("MyProperty", "MyValue")
         .register(MyProvider.class)
         .register(MyFeature.class);

After these lines, client references to an instance that has the property MyProperty set to MyValue and that has MyProvider and MyFeature registered.

What neither the Jakarta REST Specification not the Client JavaDocs tell is, what happens to client if we extend the example like this:

Client c2 = client.register(Foo.class).property("MyProperty", "Bar");

I assume it is undisputed that c2 refers to a Client additionally having Foo registered and property MyProperty changed to Bar.

But what about client? Does client (like c2) also have Foo registered and property Bar set to MayValue now, or is client still holding the value MyValue and does not have Foo registered?

We should discuss this question, find a concensus, and adapt the JavaDocs.

Opinions?

spericas commented 2 weeks ago

I don't understand how the separation of the calls to property and register into different statements should make any difference. If the first statement updated the internal state of client, so should the second statement. Maybe I'm not understanding your point.

jamezp commented 2 weeks ago

Maybe I'm not understanding either, but I agree with @spericas. There is nothing in the spec nor the JavaDoc that says a client instance is immutable. Those chained methods come from the Configurable interface which seems to indicate, via the examples, that implementations should be mutable.

As a general rule, for each JAX-RS component class there can be at most one registration — class-based or instance-based — configured at any given moment. Implementations MUST reject any attempts to configure a new registration for a provider class that has been already registered in the given configurable context earlier. Implementations SHOULD also raise a warning to inform the user about the rejected component registration.

For example:

config.register(GzipInterceptor.class, WriterInterceptor.class);
config.register(GzipInterceptor.class); // Rejected by runtime.
config.register(new GzipInterceptor()); // Rejected by runtime.
config.register(GzipInterceptor.class, 6500); // Rejected by runtime.

config.register(new ClientLoggingFilter());
config.register(ClientLoggingFilter.class); // rejected by runtime.
config.register(ClientLoggingFilter.class,
        ClientResponseFilter.class); // Rejected by runtime.
jansupol commented 2 weeks ago

The behavior can actually be good to emphasize if not already done so in the Spec. While the mutability of the client can be clear, the consequences as in the following example can be surprising:

        Client client = ClientBuilder.newClient()
                .register(new ClientRequestFilter() {
            @Override
            public void filter(ClientRequestContext requestContext) throws IOException {
                Object property = requestContext.getConfiguration().getProperty("ABORT_PROPERTY");
                requestContext.abortWith(Response.status(property == null ? 200 : (int) property).build());
            }
        });
        try (Response r = client.target("http://localhost:9090").request().get()) {
            System.out.println(r.getStatus());
        }
        Client client2 = client.property("ABORT_PROPERTY", 555);
        try (Response r = client.target("http://localhost:9090").request().get()) {
            System.out.println(r.getStatus());
        }

Even though the abortion status was set on a "different" client, the behavior of the first client is changed and the first request returns status 200, and the second 555.

While with the client the behavior is quite clear, with the WebTarget mutability and immutability this can be much more puzzling.

mkarg commented 2 weeks ago

The point of my question simply is: Neither the spec nor the JavaDocs explicitly mandate that the result of .register() and .property() MUST update the original object, but only says, that the RETURNED object is updated. So there COULD be an implementation that returns a DIFFERENT object which is updated, while the original is NOT. Portable applications need to know:

To sum up, I do not plea for any particular solution, but only for a small change of the JavaDocs that MANDATE (a) or (b).

mkarg commented 2 weeks ago

As a general rule, for each JAX-RS component class there can be at most one registration — class-based or instance-based — configured at any given moment. Implementations MUST reject any attempts to configure a new registration for a provider class that has been already registered in the given configurable context earlier. Implementations SHOULD also raise a warning to inform the user about the rejected component registration.

"SHOULD" is not "MUST". Do we all agree that what we actually want the runtime to do is "MUST" then we have to write "MUST".

jamezp commented 2 weeks ago

FWIW I'm fine if we want a clarification, but I don't really see how it's not obvious. IoW if .register() or .property() didn't update the original object, the method chaining itself would not work. In the following example

 Client client = ClientBuilder.newClient();

 client.property("MyProperty", "MyValue")
         .register(MyProvider.class)
         .register(MyFeature.class);

if the client instance was immutable only the MyFeatcure.class would be registered. I don't know how you could not assume that

Client c2 = client.register(Foo.class).property("MyProperty", "Bar");

would not create a new client. I don't really see how it could be interpreted any other way TBH. Again, though clarification is totally fine.

mkarg commented 2 weeks ago

@jamezp Isn't the problem obvious? See:

Client client = ClientBuild.newClient(); // client refers to object 1 initially, which is immutable

client.property("MyProperty", "MyValue") // object 1 is untouched, property() returns a modified immutable copy -> object 2
        .register(MyProvider.class) // object 2 is untouched, register() returns a modified immutable copy -> object 3
        .register(MyFeature.class) // object 3 is untouched, register() returns a modified immutable copy -> object 4

I agree that this interpretation is wrong for register() due to the example given on the Configurable page (see your original posting), but note:

I understand that you all share the vision:

So let's simply agree to clearly say exactly that in the JavaDocs (I would volunteer for the needed changes).

(BTW, just in case you wonder where this discussion comes from: Yes, there are use cases where it would make sense to be able to have a lightweight Client clone with different configuration -- but this is is a separate discussion...)

jamezp commented 2 weeks ago

I agree that being explicit with the documentation makes sense.