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

Clarification: Reconfiguration of Client #1271

Open
mkarg opened this issue Jun 14, 2024 · 8 comments
Open

Clarification: Reconfiguration of Client #1271

mkarg opened this issue Jun 14, 2024 · 8 comments

Comments

@mkarg
Copy link
Contributor

mkarg commented Jun 14, 2024

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

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

jamezp commented Jun 14, 2024

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

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

mkarg commented Jun 14, 2024

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:

  • (a) Can I rely upon the fact the the original object IS updated?
  • (b) Can I rely upon the fact the the original object IS NOT updated?

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

mkarg commented Jun 14, 2024

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

jamezp commented Jun 14, 2024

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

mkarg commented Jun 14, 2024

@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:

  • IMHO examples are not authoritative, while the spec itself and the "normal" Javadocs are.
  • Neither the example nor the general rule above it say anything about property(), so in theory property() MAY return a copy but keep the original object pristine.
  • The @return does not mandate that the same object MUST get returned, but only "some" modified object.

I understand that you all share the vision:

  • register() and property() MUST modify the original object

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

jamezp commented Jun 15, 2024

I agree that being explicit with the documentation makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants