Replace hardcoded classnames and reflection with ServiceLoader

[bdemers]

@jaapcoomans I'm dusting this off, sorry for the delay #458

I love the FakeServiceDescriptorClassLoader trick!

TODO:

• Deprecate the Classes util class? (not sure if this is possible given the last two remaining usages of this class:
  • OrgJsonSerializer re: java.lang.NoClassDefFoundError: org.json.JSONString #388
  • RuntimeEnvironment (checking if Bouncy Castle is available)
• merge in pending changes of Fix split package issue in extensions/jackson and extensions/orgjson #488 (rename Serializer classes)

IMHO, that first item does NOT block this issue, but I'd like to hear if anyone has any thoughts on the topic.

[coveralls]

Coverage remained the same at 100.0% when pulling 7037d64 on service-loader into bf7e300 on master.

[lhazlewood]

Originally I was very on-board for this PR and the idea of ServiceLoader for JJWT's internal implementations.

However, I don't see the need for it for any of JJWT's internal implementations that shouldn't be modified or overridden by users (e.g. spec-compliant implementations).

Please see other comments regarding the SignatureAlgorithm interface change as well.

[bdemers]

@lhazlewood I've reduced the scope of this PR down to the CompressionCodecs and Serializer/Deserializer implementations.
We can take another look at the other changes after we make any breaking changes for 1.0

[lhazlewood]

@bdemers based on this, the DefaultCompressionCodecResolver also needs to be updated to resolve any/all codecs found in the classpath. But please ensure that JJWT's Deflate and Gzip instances always take precedence for for values of (case-insensitive) ZIP and DEF. (In other words, even if a user specifies a codec with name zip or gzip, JJWT's instances will still be used because they reflect the RFC standard).

This ensures that if providing a custom compression codec by dropping one in the classpath, you wouldn't have to manually configure a CustomCodecResolver on the parser (as is required today).

This change also requires us to update the JavaDoc for CompressionCodecResolver and DefaultCompressionCodecResolver to explain what happens by default, as well as update the README documentation here: https://github.com/jwtk/jjwt#compression

Also, IMO, this implies that we would probably have a .compressWith(String name) method that allows the user to specify the codec name, and we look it up with a new utility method in CompressionCodecs, e.g. CompressionCodecs.forName(String name). The lookup should be lazy IMO, and only assert that it actually exists during the build() method (and not when compressWith is called), per normal builder semantics (because compressWith might be called multiple times via framework configuration code, and only the last value matters).

Thoughts?

[bdemers]

Good call on the DefaultCompressionCodecResolver. Ensuring JJWT's codecs are used first gets a little sticky. We could add a package name check on the resolved Codec, but at that point, we could just use reflection to load the implementation (without the Service Loader)

I like the .compressWith(name) idea! The previous concerns about ensuring JJWT's codecs are used, makes me wonder if we should just worry about the Serializer/Deserializer for this release? (and pull the CompressionCodec out of this PR), I think i'm on the fence now.

?

[lhazlewood]

hrm - you've gone through all this work, it'd feel like taking an unnecessary step back IMO if we don't just finish these few extra things.

Ensuring JJWT's codecs are used first is pretty easy IMO, unless I'm missing something:

In DefaultCompressionCodecResolver, after line 62 but before line 64, you call CompressionCodecs.forName. If still null after that, the existing line 64 throws the exception as expected. No? What am I missing? 😅

[lhazlewood]

In fact, move lines 57-62 into CompressionCodecs.forName and both the resolver and the parser builder call that method. The JJWT-prioritization logic is localized into one place and works in both scenarios (AFAICT).

[bdemers]

DefaultCompressionCodecResolver (or anything in the impl module) isn't an issue. I was thinking about how ensuring the JJWT CompressionCodec implementations would affect the proposed changes to CompressionCodecs

CompressionCodec would need to revert back to using reflection to make sure CompressionCodecs.DEFLATE and DefaultCompressionCodecResolver.resolveCompressionCodec({"zip": "deflate"}) both return the same implementation/instance.

Since the library, itself wouldn't be taking advantage of loading it's CompressionCodec's this way, I was questioning if we should continue down this path, and whether this change would be making this extensible just for the sake of extensibility. (NOTE: I have zero data on how often custom implementations are used <insert grain of salt>)

Either way, adding an additional method to CompressionCodecResolver might be handy:

CompressionCodec resolveCompressionCodec(String name) throws CompressionException;

This could be used for your proposed . compressWith(name) method (service-loader or not).

Thinking out loud.

[lhazlewood]

I'm suggesting to keep the logic that you've added (public static final variables with static class init block). CompressionCodecs.GZIP and DEFLATE need to remain for backwards compatibility.

I'm proposing this (simplified for brevity - needs javadoc comments, etc):

private static final String DEFLATE_IMPL_CLASSNAME = "io.jsonwebtoken.impl.compression.DeflateCompressionCodec";
private static final String GZIP_IMPL_CLASSNAME = "io.jsonwebtoken.impl.compression.GzipCompressionCodec";

private static final Map<String,CompressionCodec> CODECS;
public static final CompressionCodec DEFLATE;
public static final CompressionCodec GZIP;

static {
    CODECS = new HashMap<String,CompressionCodec>();
    Collection<CompressionCodec> loaded = Services.loadAll(CompressionCodec.class);
    for (CompressionCodec codec : loaded) {
        String className = codec.getClass().getName();
        if (DEFLATE_IMPL_CLASSNAME.equals(className)) {
          DEFLATE = codec;
        } else if (GZIP_IMPL_CLASSNAME.equals(className)) {
          GZIP=codec;
        }
        String name = codec.getName();
        // TODO: if !Strings.hasText(name) throw exception
       name = name.toUpperCase() //guarantee canonical for map storage and lookup
       CODECS.put(name, codec);
    }

    //TODO: if either DEFLATE or GZIP are null here, throw an IllegalStateException
    
    // Now ensure JJWT's GZIP and DEFLATE always take precedence for RFC-standard names:
    CODECS.put(DEFLATE.getName(), DEFLATE);
    CODECS.put(GZIP.getName(), GZIP);
}

public static final CompressionCodec forName(String name) {
  if (!Strings.hasText(name)) { ... throw exception ... }
  String name = name.toUpperCase(); //normalize for map lookup
  CompressionCodec codec = CODECS.get(name);
  if (codec == null) { throw new UnsupportedCodecException("There is no codec with algorithm name " + name + ": Ensure jjwt-impl.jar or your custom implementation .jar are in the classpath according to the documentation at https://github.com/jwtk/jjwt#whatever." }
}

[lhazlewood]

Hrm - the reflection angle is super easy too. Maybe a hybrid approach makes sense: do reflection for our internal implementations (no serviceloader metadata) and allow custom CompressionCodecs to be added to the classpath for parser lookup?

I like the reflection angle for our internal stuff actually, and serviceloader for plugins. I think that's super clean as we wouldn't conflict with any plugins, ever.

[lhazlewood]

So based on that last comment, how about this:

private static final String DEFLATE_IMPL_CLASSNAME = "io.jsonwebtoken.impl.compression.DeflateCompressionCodec";
private static final String GZIP_IMPL_CLASSNAME = "io.jsonwebtoken.impl.compression.GzipCompressionCodec";

private static final Map<String,CompressionCodec> CODECS;
public static final CompressionCodec DEFLATE;
public static final CompressionCodec GZIP;

static {
    CODECS = new HashMap<String,CompressionCodec>();
    Collection<CompressionCodec> loaded = Services.loadAll(CompressionCodec.class);
    for (CompressionCodec codec : loaded) {
        String name = codec.getName();
        // TODO: if !Strings.hasText(name) throw exception
       name = name.toUpperCase() //guarantee canonical for map storage and lookup
       CODECS.put(name, codec);
    }

    DEFLATE = Classes.newInstance(DEFLATE_IMPL_CLASSNAME);
    CODECS.put(DEFLATE.getName, DEFLATE);

    GZIP = Classes.newInstance(GZIP_IMPL_CLASSNAME);
    CODECS.put(GZIP.getName(), GZIP);
}

public static final CompressionCodec forName(String name) {
  if (!Strings.hasText(name)) { ... throw exception ... }
  String name = name.toUpperCase(); //normalize for map lookup
  CompressionCodec codec = CODECS.get(name);
  if (codec == null) { throw new UnsupportedCodecException("There is no codec with algorithm name " + name + ": Ensure jjwt-impl.jar or your custom implementation .jar are in the classpath according to the documentation at https://github.com/jwtk/jjwt#whatever." }
}

[bdemers]

@lhazlewood getting back to this.
I did something very similar to your suggestion, except I moved the logic into DefaultCompressionCodecResolver

[bdemers]

a couple more minor tweaks found in a quick self review

[skjolber]

So will this all work with JDK9+ modules now?

[bdemers]

@skjolber the big blocker for 9+ was the split package issue in: #488
(We are testing with Open JDK 8, 9, 10, 11 now too)

If you run into anything specific let us know!

We haven't added module-info's as we are still technically testing/building on 1.7
There is another issue covering this specifically, but we could add them starting with the 1.0 release, but it would mean that we would need to use Java 11 for building (and still target 1.8)
i.e. this: https://maven.apache.org/plugins/maven-compiler-plugin/examples/module-info.html

[lhazlewood]

Some more comments. I love seeing deleted code! 😄

[lhazlewood]

Nice stuff @bdemers !

I have just a teeny few nits remaining - only because I'm a little paranoid about any changes in the api/src/main location due to its exposure to (now) thousands of projects. Once they're done, we're good to merge!

[bdemers]

All refs to new UnavailableImplementationException() are from Servies.* and all refs to those method pass a Class directly in.