jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

FactoryFinder does not respect the SystemProperty of factoryId #1259

Open stbischof opened 2 months ago

stbischof commented 2 months ago

If the SystemProperty "jakarta.ws.rs.client.ClientBuilder" is set, the FactoryFinder must respect this value when searching for e.g. ClientBuilder using ServiceLoader. If set, all ClientBuilder without matching classname must be ignored.

Spec:

    /**
     * Name of the property identifying the {@link ClientBuilder} implementation to be returned from
     * {@link ClientBuilder#newBuilder()}.
     */
    public static final String JAXRS_DEFAULT_CLIENT_BUILDER_PROPERTY = "jakarta.ws.rs.client.ClientBuilder";
* @param factoryId the name of the factory to find, which is a system property.

 static <T> Object find(final String factoryId, final Class<T> service) throws ClassNotFoundException {

Code: https://github.com/jakartaee/rest/blob/895716b7f81636c2df7fc80706ecb5f6482ea979/jaxrs-api/src/main/java/jakarta/ws/rs/client/FactoryFinder.java#L179

Issue: The method FactoryFinder.findFirstService gets the factoryId but doer not use it.

Test:

System.setProperty(ClientBuilder.JAXRS_DEFAULT_CLIENT_BUILDER_PROPERTY, ClientBuilderImpl.class.getName());
ClientBuilder clientBuilder = ClientBuilder.newBuilder();
assertInstanceOf(ClientBuilderImpl.class, clientBuilder);

I can to a PR

jamezp commented 2 months ago

I don't see where it's defined that the system property must be used first. The property is checked, but it's the last option not the first.

stbischof commented 2 months ago

I do not say that is must be first step to Instanciate the SystemPropertys Class. I am saying, if the value is set to SystemProperty, then is must be respected.

IF SystemPropertys is set. When there is a ClientBuilders available over ServiceLoader, we should check against from SystemPropertys When there are multple ClientBuilders available over ServiceLoader, we should use the one from SystemPropertys

jansupol commented 2 months ago

I have never felt it had been meant to be a System property. I understand it to be a name of a META-INF/services/jakarta.ws.rs.client.ClientBuilder defined as a property instead of a magic string in the code. I'd see it as a wrong javadoc saying "property" instead of "service".

@stbischof Can you please describe a use-case of yours that would require the service name to be renamed via the System property? Have you got multiple Client implementations on the classpath?

stbischof commented 2 months ago

One of the cases is like that, what i found on GitHub. And how i expected how the Constanze in javadoc in meaned.

https://github.com/stjordanis/knime-rest/blob/2941f3a046f7c7b51578a07f3b19b89d5e343b5d/org.knime.rest/src/org/knime/rest/nodes/common/RestNodeModel.java#L286

https://github.com/Asbaharoon/oci-java-sdk/blob/36329d6392630c05d89dc2c0757da3400d63a543/bmc-examples/src/main/java/InstancePrincipalsAuthenticationDetailsProviderWithResteasyClientExample.java#L34

But. I see that the spec there is... Not very descriptive.

The usecase is to set an own ClientBuilder class. That you can modify the default. But with the current api you cant get this.

Also if there are several impls on classpath like in an OSGi Setup you need an Option to select the one you like to have.

jamezp commented 2 months ago

It does look for the property in the jaxrs.properties, then the system property, https://github.com/jakartaee/rest/blob/895716b7f81636c2df7fc80706ecb5f6482ea979/jaxrs-api/src/main/java/jakarta/ws/rs/client/FactoryFinder.java#L125-L161. However, this is done as a last resort.

In other words, it does work, but not as you're expecting. I'm not an OSGi expert by any means, but it seems like you'd need to have your service "higher" on the class path or remove the other implementation.