jakartaee/rest

FactoryFinder does not respect the SystemProperty of factoryId

Opened this issue · 5 comments

stbischof commented

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:

rest/jaxrs-api/src/main/java/jakarta/ws/rs/client/FactoryFinder.java

Line 179 in 895716b

return loader.iterator().next();

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

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

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

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

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

It does look for the property in the jaxrs.properties, then the system property,

rest/jaxrs-api/src/main/java/jakarta/ws/rs/client/FactoryFinder.java

Lines 125 to 161 in 895716b

// try to read from $java.home/lib/jaxrs.properties
FileInputStream inputStream = null;
String configFile = null;
try {
String javah = System.getProperty("java.home");
configFile = javah + File.separator + "lib" + File.separator + "jaxrs.properties";
File f = new File(configFile);
if (f.exists()) {
Properties props = new Properties();
inputStream = new FileInputStream(f);
props.load(inputStream);
String factoryClassName = props.getProperty(factoryId);
return newInstance(factoryClassName, classLoader);
}
} catch (Exception ex) {
LOGGER.log(Level.FINER, "Failed to load service " + factoryId
+ " from $java.home/lib/jaxrs.properties", ex);
} finally {
if (inputStream != null) {
try {
inputStream.close();
} catch (IOException ex) {
LOGGER.log(Level.FINER, String.format("Error closing %s file.", configFile), ex);
}
}
}
// Use the system property
try {
String systemProp = System.getProperty(factoryId);
if (systemProp != null) {
return newInstance(systemProp, classLoader);
}
} catch (SecurityException se) {
LOGGER.log(Level.FINER, "Failed to load service " + factoryId
+ " from a system property", se);
}
. 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.

  • 👍1