jwtk / jjwt

Java JWT: JSON Web Token for Java and Android
Apache License 2.0
10.33k stars 1.34k forks source link

Unable to find implementation classes in OSGi #578

Closed cziegeler closed 3 years ago

cziegeler commented 4 years ago

At least since the removal of the service loader for loading implementations, the library does not work in OSGi anymore. The reason is that the api bundle does not have access to classes from the impl bundle as class loading across bundles only works if those classes are exported (and imported). I'm not sure how to solve this; clearly it would be wrong to export implementation classes and import them in the api. Creating a single bundle with api and impl would work, with the expense of creating another artifact.

This is the exception I get with latest 0.11.1 :

io.jsonwebtoken.lang.UnknownClassException: Unable to load class named [io.jsonwebtoken.impl.DefaultJwtBuilder] from the thread context, current, or system/application ClassLoaders. All heuristics have been exhausted. Class could not be found. Have you remembered to include the jjwt-impl.jar in your runtime classpath? at io.jsonwebtoken.lang.Classes.forName(Classes.java:92) at io.jsonwebtoken.lang.Classes.newInstance(Classes.java:136) at io.jsonwebtoken.Jwts.builder(Jwts.java:141) at de.osoco.cziegeler.test.jjwt.Test.start(Test.java:12)

lhazlewood commented 4 years ago

@cziegeler thanks for the issue - I really appreciate it.

Some questions:

Did this problem happen to you when using 0.10.x or earlier?

Could it be that we're not exporting the OSGi metadata correctly? This is the only thing we have for maintaining OSGi bundle data:

https://github.com/jwtk/jjwt/blob/c09deaa5f31fb7b3a8184daec4228269942c77a4/pom.xml#L485-L503

(This is in <build><plugins> to ensure it is enabled for all children modules automatically).

Thoughts? cc @bdemers

bdemers commented 4 years ago

Hi @cziegeler! Any chance you can create a simple example project to reproduce the problem? My assumption is that we just need configure the OSGI metadata correctly

cziegeler commented 4 years ago

@lhazlewood Yes, this happended with the 10.x version as well; but I only tested with the latest 10.x release. @bdemers The OSGi metadata is correct - the api jar exports everything and the impl jar does not export anything - this is how it should be. But due to that the api does not find the impl. I have some ideas on how to fix it, I'll try them out and then report back here

cziegeler commented 4 years ago

Just some more information: the first official release supporting OSGi is 0.11.0 (which has the same problem). The easiest solution is to make the implementation bundles fragments and therefore make them visible to the api bundle. In this case, it works exactly as it does outside of OSGi. I've created a PR for this: #580

cziegeler commented 4 years ago

I've also created a simple sample project which currently references 0.12.0-SNAPSHOT and can be used to reproduce the problem: jjwt-test

bdemers commented 4 years ago

Thanks @cziegeler! There is also a recommendation from the OSGi alliance here: https://blog.osgi.org/2013/02/javautilserviceloader-in-osgi.html?m=1

It’s a little older at this point, but any thoughts on that technique?

cziegeler commented 4 years ago

@bdemers There are currently two problems - first one is that the API bundle is not finding the classes from the implementation bundle using class loading, second one is that the API bundle is not finding services from the implementation or extension bundles when service loader is used. The serviceloader approach mentioned there helps with the second problem only, but then requires additional code that creates a (unnecessary) dependency to OSGi. Using OSGi fragments is a common technique for solving these issues, especially if you want to keep your code OSGi free.

cziegeler commented 4 years ago

@bdemers / @lhazlewood Any chance to get that patch applied? Today it's impossible to use these libraries out of the box in OSGi; with the patch this is finally doable

demetriuzz commented 4 years ago

Good day to all!

For version 0.11.1 in OSGi environment (on the example Karaf 4.2.8) got a similar problem.

I use instructions <Embed-Dependency> different ways, but nothing helped. There was still an exception: io.jsonwebtoken.lang.UnknownClassException

So I switched to another dependency: com.auth0/java-jwt/3.10.1 And use Instructions: <Embed-Dependency>java-jwt;inline=true</Embed-Dependency>

cziegeler commented 4 years ago

Right, we use a similar repackaging solution at the moment as well - which is basically taking all the different libraries (api, impl, serializer) and putting them in a single jar. With the proposed patch this extra work downstream can be avoided

hlavki commented 4 years ago

I don't think that Fragment-Host is good solution. It's workaround to badly written API. Really don't understand why there is need to separate API jar from IMPL jar. More complex projects like jackson hasn't this problem.

bdemers commented 4 years ago

@hlavki what would you recommend instead?

I also wouldn’t say separating the interfaces from the implementation packaging is a bad practice. It’s commonly found in many libraries. IIRC OSGi has build in support for this, but the vanilla Java/Android world doesn’t, and mechanisms like ServiceLoader are used.

hlavki commented 4 years ago

Of course, I am not talking about API/Impl separation. It's generally good approach used almost everywhere. And yes, OSGi supports it too. What I am talking about is linking implementation classes to API classes using strings e.g.:

public static JwtBuilder builder() {
        return Classes.newInstance("io.jsonwebtoken.impl.DefaultJwtBuilder");
}

vs. e.g. jackson's module system:

return new ObjectMapper()
                 .registerModule(new JavaTimeModule());

You can have DefaultJwtBuilder that implements JwtBuilder interface, but you don't need to instantiate it using java reflection API and also you don't need separated jar file.

lexsoto commented 4 years ago

Hello, I am having this exact same issue with version 0.11.2. Was this fix included in 0.11.2?

bdemers commented 4 years ago

@lexsoto it was.

Can you tell us about your environment and classloader? (and which jars do you have on your classpath?)

lexsoto commented 4 years ago

Thanks, @bdemers,

It is not working for me with Karaf 4.2.9, unless I specify the Serializer explicitly, as in:

Jwts.builder().serializeToJsonWith(new JacksonSerializer<>())

bdemers commented 4 years ago

@lexsoto can you include your stacktrace, and the list of JJWT modules that you are using?

cziegeler commented 4 years ago

@lexsoto It works for me with the JacksonSerializer if I include these bundles: "io.jsonwebtoken:jjwt-jackson:0.11.2", "io.jsonwebtoken:jjwt-impl:0.11.2", "io.jsonwebtoken:jjwt-api:0.11.2", "com.fasterxml.jackson.core:jackson-core:2.9.10", "com.fasterxml.jackson.core:jackson-annotations:2.9.10", "com.fasterxml.jackson.core:jackson-databind:2.9.10.4"

(Not testing in Karaf though, but that shouldn't be a difference)

lexsoto commented 4 years ago

These are the bundles I am loading (Karaf 4.2.9):

154 │ Active   │  40 │ 0.11.2                │ JJWT :: API, Fragments: 155, 156
155 │ Resolved │  40 │ 0.11.2                │ JJWT :: Impl, Hosts: 154
156 │ Resolved │  40 │ 0.11.2                │ JJWT :: Extensions :: Jackson, Hosts: 154
50 │ Active   │  10 │ 2.10.1                │ Jackson-annotations
 51 │ Active   │  10 │ 2.10.1                │ Jackson-core
 52 │ Active   │  10 │ 2.10.1                │ jackson-databind

I am also seeing this other variation of the same issue, but during validation:

java.util.ServiceConfigurationError: io.jsonwebtoken.CompressionCodec: Provider io.jsonwebtoken.impl.compression.DeflateCompressionCodec not found
    at java.util.ServiceLoader.fail(ServiceLoader.java:588) ~[?:?]
    at java.util.ServiceLoader$LazyClassPathLookupIterator.nextProviderClass(ServiceLoader.java:1211) ~[?:?]
    at java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1220) ~[?:?]
    at java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1264) ~[?:?]
    at java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1299) ~[?:?]
    at java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1384) ~[?:?]
    at io.jsonwebtoken.impl.lang.Services.loadAll(Services.java:80) ~[!/:0.11.2]
    at io.jsonwebtoken.impl.lang.Services.loadAll(Services.java:68) ~[!/:0.11.2]
    at io.jsonwebtoken.impl.compression.DefaultCompressionCodecResolver.<init>(DefaultCompressionCodecResolver.java:60) ~[!/:0.11.2]
    at io.jsonwebtoken.impl.DefaultJwtParserBuilder.<init>(DefaultJwtParserBuilder.java:58) ~[!/:0.11.2]
    at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:?]
    at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:?]
    at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:?]
    at java.lang.reflect.Constructor.newInstance(Constructor.java:490) ~[?:?]
    at java.lang.Class.newInstance(Class.java:584) ~[?:?]
    at io.jsonwebtoken.lang.Classes.newInstance(Classes.java:156) ~[!/:0.11.2]
    at io.jsonwebtoken.lang.Classes.newInstance(Classes.java:136) ~[!/:0.11.2]
    at io.jsonwebtoken.Jwts.parserBuilder(Jwts.java:130) ~[!/:0.11.2]
lexsoto commented 4 years ago

Even when I tried to provide my own Compression Codec Resolver it would fail. It is good that you are allowed to replace the default one with yours as in:

Jwts.parserBuilder().setCompressionCodecResolver(resolver)

But it is of no use when class DefaultJwtParserBuilder instantiates the DefaultCompressionCodecResolver :

private CompressionCodecResolver compressionCodecResolver = new DefaultCompressionCodecResolver();

Here is the relevant stack trace:

java.util.ServiceConfigurationError: io.jsonwebtoken.CompressionCodec: Provider io.jsonwebtoken.impl.compression.DeflateCompressionCodec not found
    at java.util.ServiceLoader.fail(ServiceLoader.java:588) ~[?:?]
    at java.util.ServiceLoader$LazyClassPathLookupIterator.nextProviderClass(ServiceLoader.java:1211) ~[?:?]
    at java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1220) ~[?:?]
    at java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1264) ~[?:?]
    at java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1299) ~[?:?]
    at java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1384) ~[?:?]
    at io.jsonwebtoken.impl.lang.Services.loadAll(Services.java:80) ~[!/:0.11.2]
    at io.jsonwebtoken.impl.lang.Services.loadAll(Services.java:68) ~[!/:0.11.2]
    at io.jsonwebtoken.impl.compression.DefaultCompressionCodecResolver.<init>(DefaultCompressionCodecResolver.java:60) ~[!/:0.11.2]
    at io.jsonwebtoken.impl.DefaultJwtParserBuilder.<init>(DefaultJwtParserBuilder.java:58) ~[!/:0.11.2]
    at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:?]
    at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:?]
    at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:?]
    at java.lang.reflect.Constructor.newInstance(Constructor.java:490) ~[?:?]
    at java.lang.Class.newInstance(Class.java:584) ~[?:?]
    at io.jsonwebtoken.lang.Classes.newInstance(Classes.java:156) ~[!/:0.11.2]
    at io.jsonwebtoken.lang.Classes.newInstance(Classes.java:136) ~[!/:0.11.2]
    at io.jsonwebtoken.Jwts.parserBuilder(Jwts.java:130) ~[!/:0.11.2]`

It would be nice if the DefaultCompressionCodecResolver is not instantiated automatically, so one can entirely replace it and avoid the errors caused by the use of Service Loaders, specifically in an OSGi environment. Here are some issues with Service Loader and OSGi:

https://blog.osgi.org/2013/02/javautilserviceloader-in-osgi.html

bdemers commented 4 years ago

@lexsoto that seems reasonable, we do something similar with the Deserializer https://github.com/jwtk/jjwt/blob/master/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java#L187-L192

Any chance you want to try to hack it up and see if it works for you?

lexsoto commented 4 years ago

I worked around the problem by doing this:

1- Shade the Json Web Token libraries:

                        <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-shade-plugin</artifactId>
                <configuration>
                    <minimizeJar>false</minimizeJar>
                    <createDependencyReducedPom>false</createDependencyReducedPom>
                    <transformers>
                        <transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer" />
                    </transformers>
                    <artifactSet>
                        <includes>
                            <include>io.jsonwebtoken:*</include>
                        </includes>
                    </artifactSet>
                    <filters>
                        <filter>
                            <artifact>io.jsonwebtoken:*</artifact>
                            <excludes>
                                <exclude>META-INF/MANIFEST.MF</exclude>
                            </excludes>
                        </filter>
                    </filters>
                </configuration>
                <executions>
                    <execution>
                        <phase>package</phase>
                        <goals>
                            <goal>shade</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>

2- Avoid importing JSON Web packages in the bundle headers because it is all now in your own Uber Jar.

Import-Package: !io.jsonwebtoken.*

3- Add bundle header to be able to find Json serializers (in my case Jackson):

DynamicImport-Package: com.fasterxml.jackson.*

4- Add bundle headers to automatically register services (CompressionCodec, Serializer, and Deserializer):

Require-Capability: osgi.extender;filter:="(osgi.extender=osgi.serviceloader.registrar)"
  Provide-Capability: \
        osgi.serviceloader;osgi.serviceloader=io.jsonwebtoken.CompressionCodec, \
        osgi.serviceloader;osgi.serviceloader=io.jsonwebtoken.io.Serializer, \
        osgi.serviceloader;osgi.serviceloader=io.jsonwebtoken.io.Deserializer, \
        osgi.service;objectClass=io.jsonwebtoken.io.Deserializer,\
        osgi.service;objectClass=io.jsonwebtoken.io.Serializer

5- Create OSGi friendly Compression Codec Resolver:

@Component(immediate = true)
  public class OsgiCompressionCodecResolver implements CompressionCodecResolver {

    @Reference(policyOption = ReferencePolicyOption.GREEDY)
    volatile private List<CompressionCodec> codecs;

    public OsgiCompressionCodecResolver() {}

    @Override
    public CompressionCodec resolveCompressionCodec(Header header) {
        Assert.notNull(header, "header cannot be null.");
        String alg = header.getCompressionAlgorithm();
        if (alg == null) return null;
        return byName(alg);
    }

    private CompressionCodec byName(String name) {
        for (CompressionCodec c : codecs) {
            if (name.equals(c.getAlgorithmName())) {
                return c;
            }
        }
        throw new CompressionException(String.format("Compression codec '%s' not found.", name));
    }
}

6- Put it all together in your client OSGi component:

import io.jsonwebtoken.io.Deserializer;
import io.jsonwebtoken.io.Serializer;

@Component
public class MyClient {
       @Reference
    private Deserializer<Map<String, ?>> deserializer;
    @Reference
    private Serializer<Map<String, ?>> serializer;
    @Reference
    private CompressionCodecResolver resolver;

...
     Jwts.parserBuilder()
    .deserializeJsonWith(deserializer)
    .setCompressionCodecResolver(resolver)
...
    Jwts.builder()
    .serializeToJsonWith(serializer)

Not exactly simple but it works, avoiding the Service Lookup and using OSGi lookup instead.

Now, I think the shading can be removed if DefaultJwtParserBuilder is changed to not automatically instantiate the DefaultCompressionCodecResolver, then an OSGi Compression Codec Resolver like the one I pasted, can be provided for those who want to use it. The headers I added in step 4, would be nice to have out of the box so that the OSGi framwork registers the services automatically.

bdemers commented 4 years ago

@lexsoto can you take a look at #611 and see if that makes your life any easier?

I'm not a regular OSGi user, is there something better we can do in our ServiceLoader logic to make it play better with OSGi environments? Do OSGi runtimes ignore the ServiceLoader metadata? If we defined a module-info (and added the services there) would that help? (granted doing that in a < Java 9 way is a PITA)

lhazlewood commented 4 years ago

Above @lexsoto referenced this:

https://blog.osgi.org/2013/02/javautilserviceloader-in-osgi.html

That article indicates how, via metadata only (META-INF/services/io.jsonwebtoken.CompressionCodec and META-INF/MANIFEST.MF files), OSGi environments can emulate ServiceLoader functionality.

Since we control both the 'provider' .jars (the Jackson, Gson, etc implementations) and the 'consumer' jar (the impl module that creates DefaultCompressionCodecResolver), can't we just solve this with metadata and without code changes?

@lexsoto and @cziegeler - would either of you be able to help us set up Karaf in an OsgiIT integration test and test this somehow so we can ensure this stuff works on every build?

Unless we have an automated repeatable solution, this problem is likely to surface repeatedly.

lexsoto commented 4 years ago

@bdemers Yes, #611 helps.

Since we control both the 'provider' .jars (the Jackson, Gson, etc implementations) and the 'consumer' jar (the impl module that creates DefaultCompressionCodecResolver), can't we just solve this with metadata and without code changes?

Metadata will definitely help, but the code change in #611 is necessary, in my opinion, since without it, the logic loading the services is executed eagerly, even when our intent is to provide our own Compression Codec Resolver; besides, I don't think that change affects non OSGi users either.

The referenced article explains the nuances with service loader and class loaders, but I am afraid the issue I encountered is more related to the use of Fragment Bundles. I am not sure fragment bundles are the best choice here, regular bundles with the metadata to let the OSGi framework register the services automatically would be cleaner IMO.

Providing an OsgiCompressionCodecResolver, like the one I pasted before would be nice as well.

About providing IT, I could set something up, but I am not very familiar with this project and how to contribute, etc. but I could do it in my repo and you guys can take it from there.

bdemers commented 4 years ago

About providing IT, I could set something up, but I am not very familiar with this project and how to contribute, etc. but I could do it in my repo and you guys can take it from there.

That is a great starting point!

lhazlewood commented 4 years ago

Metadata will definitely help, but the code change in #611 is necessary, in my opinion, since without it, the logic loading the services is executed eagerly, even when our intent is to provide our own Compression Codec Resolver

I agree, this is necessary (to load lazily), but it doesn't address the underlying issue that discovery is not working in an OSGi environment:

Providing an OsgiCompressionCodecResolver, like the one I pasted before would be nice as well.

My point was that if we got the MANIFEST.MF records correct per the blog article, you shouldn't need to have an OsgiCompressionCodecResolver at all. That you had to create one (and a shade .jar) at all is the problem IMO, and the ideal solution would be to address that instead of having OSGi-specific classes/annotations/etc.

About providing IT, I could set something up, but I am not very familiar with this project and how to contribute, etc. but I could do it in my repo and you guys can take it from there.

That would be so helpful! We'd really appreciate it 😅

What I think we're hoping for is something that sets up Karaf in an OsgiIT test class and allows us a 'hook' where we can put JJWT specific code that will run in an OSGi environment. If you (or @cziegeler, or anyone else) could get that part working for us and provide a line like //JJWT stuff goes here, that would allow us to take it from there.

lhazlewood commented 4 years ago

Reopening - 901048a was only part of the solution.

bdemers commented 4 years ago

Thanks @lhazlewood! I forgot that that linked PR would close the issue automatically

cziegeler commented 4 years ago

@bdemers Regarding your question about "module-info" , apart from the Java 9+ problem, it wouldn't help as OSGi is not using these things out of the box. Now, granted fragments are not the best solution in the world, but the other option would be proper metadata and usage of service loader - my impression was that this project moved away from that solution - and I assume there were good reasons for it. We can make it work with service loader in OSGi; as mentioned in one of the pointers it requires an additional bundle (the service loader mediator) at runtime and the additional OSGi metadata.

bdemers commented 4 years ago

@cziegeler If you can help write an OSGi integration test that would go a long way to fixing this problem.

Is the needed metadata something we can just add to: https://github.com/jwtk/jjwt/blob/master/impl/bnd.bnd

lexsoto commented 4 years ago

I worked on an integration test for this using Karaf, you can find it here:

https://github.com/lexsoto/jwt-karaf-itest

Feel free to copy and adapt.

A funny thing happened, though, and it is that I expected to fail, but it is working! Of course this is a very simple test, my real project is a lot more complex, and it was not working there.
I will spend more time figuring out what makes it break, in other words, why does it work in the simple integration test and not in my actual project. Thanks for the help, sorry for the noise.

cziegeler commented 4 years ago

@bdemers Yes, the metadata can be added to the bnd files - the basics of the test provided by @lexsoto looks good to me.

bdemers commented 4 years ago

@lexsoto no worries! Your project might have more complex classloading, which is what we would like make sure we capture in the test.

Keep us posted!

lexsoto commented 4 years ago

Finally I was able to reproduce the error with a minimal project.

Please check: https://github.com/lexsoto/jwt-karaf-itest

This is new version is a multi module project. The issue shows up when invoking from a Servlet. See servlet in the client subproject.

I hope this helps in solving this issue. Thanks!

bdemers commented 4 years ago

Great! Is it possible to mimic the classloading in the test? (i'm not sure how karaf does this)

lexsoto commented 4 years ago

I am not sure how to, but it may be possible; however, I suspect it may be more related to threads, because when I tried different bundles (each has its own class loader), I could not reproduce; only when JJWT is invoked in the context of a Servlet (which runs in a web server thread (Jetty) is when I was able to reproduce.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale due to inactivity for 60 or more days. It will be closed in 7 days if no further activity occurs.

stale[bot] commented 3 years ago

Closed due to inactivity.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale due to inactivity for 60 or more days. It will be closed in 7 days if no further activity occurs.

stale[bot] commented 3 years ago

Closed due to inactivity.