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

Enable "jakarta.json.provider" system property to work in JPMS apps. Provide explicit API to create JsonProvider instance #129

Closed
cdivilly opened this issue May 17, 2024 · 3 comments · Fixed by #130

Comments

@cdivilly
Copy link

cdivilly commented May 17, 2024

There are a number of cases where an application may need to explicitly instantiate the Parsson jakarta.json.spi.JsonProvider instance and where using JsonProvider.provider() ServiceLoader based approach is not sufficient:

  • Due to a complicated chain of dependencies, there ends up being multiple Jakarta JSON providers available at runtime. The application needs to control/configure exactly which provider is used and when a given provider is instantiated.
  • The ServiceLoader mechanism which powers JsonProvider.provider() has known performance side-effects and may need to be avoided.

the addition of the JSONP_PROVIDER_FACTORY system property in JEE10 somewhat addresses this need.

However, this system property based solution does not work in JPMS enabled applications at present. For example if I have code like the following:

package org.example;

import jakarta.json.spi.JsonProvider;

public class JpmsExample {

    public static void main(String... args) {
        final var providerName = "org.eclipse.parsson.JsonProviderImpl";
        System.setProperty(JsonProvider.JSONP_PROVIDER_FACTORY, providerName);
        final var provider = JsonProvider.provider();
        assert provider.getClass().getName().equals(providerName);
    }
}

It will fail when running in a JPMS enabled runtime:

Exception in thread "main" jakarta.json.JsonException: Provider org.eclipse.parsson.JsonProviderImpl could not be instantiated: java.lang.IllegalAccessException: class jakarta.json.spi.JsonProvider (in module jakarta.json) cannot access class org.eclipse.parsson.JsonProviderImpl (in module org.eclipse.parsson) because module org.eclipse.parsson does not export org.eclipse.parsson to module jakarta.json
	at [email protected]/jakarta.json.spi.JsonProvider.newInstance(JsonProvider.java:163)
	at [email protected]/jakarta.json.spi.JsonProvider.provider(JsonProvider.java:115)
	at org.example/org.example.JpmsExample.main(JpmsExample.java:16)
Caused by: java.lang.IllegalAccessException: class jakarta.json.spi.JsonProvider (in module jakarta.json) cannot access class org.eclipse.parsson.JsonProviderImpl (in module org.eclipse.parsson) because module org.eclipse.parsson does not export org.eclipse.parsson to module jakarta.json
	at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:394)
	at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:714)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:495)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
	at [email protected]/jakarta.json.spi.JsonProvider.newInstance(JsonProvider.java:158)
	... 2 more

This could be addressed if Parsson opened the org.eclipse.parsson package to jakarta.json:

module org.eclipse.parsson {
    requires transitive jakarta.json;
    exports org.eclipse.parsson.api;
 
    opens org.eclipse.parsson to jakarta.json; // enable jakarta.json instantiate org.eclipse.parsson.JsonProviderImpl
 
    provides jakarta.json.spi.JsonProvider with org.eclipse.parsson.JsonProviderImpl;
}

I think it would also be beneficial if Parsson provided a proprietary API to instantiate a JsonProvider:

package org.eclipse.parsson.api;
 
public abstract class Parsson {

   private Parsson() {}

    public static JsonProvider newProvider() {
        return new org.eclipse.parsson.JsonProviderImpl();
    } 
}

This would give clients full control over how and when to instantiate the provider, similar to the capability non JPMS enabled applications enjoy to do new org.eclipse.parsson.JsonProviderImpl() wherever/whenever they need.

@cdivilly cdivilly changed the title Provide explicit API to create JsonProvider instance Enable "jakarta.json.provider" system property to work in JPMS apps. Provide explicit API to create JsonProvider instance May 17, 2024
@cdivilly
Copy link
Author

An alternative to introducing the proposed Parsson.newProvider() API would be to export the org.eclipse.parsson package:

module org.eclipse.parsson {
    requires transitive jakarta.json;
    exports org.eclipse.parsson;
    exports org.eclipse.parsson.api;
 
    provides jakarta.json.spi.JsonProvider with org.eclipse.parsson.JsonProviderImpl;
}

This would restore the API surface that non JPMS enabled applications see, so would minimize compatibility issues for non JPMS enabled applications that are in the process of migrating to JPMS.

This might be the most pragmatic solution.

jbescos added a commit to jbescos/parsson that referenced this issue May 20, 2024
…ovide explicit API to create JsonProvider instance eclipse-ee4j#129

Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos
Copy link
Member

jbescos commented May 20, 2024

Using a system property to define the expected implementation could have concurrent issues. For example with 2 threads:

  • Thread1: sets implementation A as system property
  • Thread2: sets implementation B as system property
  • Thread1: obtains instance of implementation B. This is unexpected, because it wants implementation A.

ServiceLoader is more correct approach, but you will need to avoid creating new unnecessary instances for the performance issues.

However, we should fix the module visibility issue.

Thank you for reporting it.

lukasj pushed a commit that referenced this issue May 20, 2024
…ovide explicit API to create JsonProvider instance #129

Signed-off-by: Jorge Bescos Gascon <[email protected]>
@lukasj
Copy link
Member

lukasj commented Jun 7, 2024

fixed by #130

@lukasj lukasj closed this as completed Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants