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

Replace hardcoded classnames and reflection with ServiceLoader #458

Closed
wants to merge 6 commits into from

Conversation

jaapcoomans
Copy link
Contributor

Currently there are several dependencies on class names between modules that go in the reverse direction of the actual dependency. This is because hardcoded class names and reflection are being used. By using ServiceLoader instead of the current setup, the dependency points in the right direction at all source levels. This makes jjwt-api fully independent and agnostic of jjwt-impl and jjwt-impl fully independent and agnostic of jjwt-jackson and jjwt-orgjson.

It is quite a big change, but I believe it's worth it. It makes it easier to develop API's, implementations and extensions separately. Currently many reported issues are blocked by the fact that jjwt is used on both Android (JDK 1.7) and regular Java (JDK 8+). This PR still is fully JDK 1.7 compliant, yet at the same is a first step to supporting JPMS.

…sses.

By using ServiceLoader the hardcoded dependency of implementation classes becomes obsolete, so that the API will be truly independent from the implementation. Also this approach paves the way for migration to JPMS modules, as these also leverage the ServiceLoader API.
@coveralls
Copy link

coveralls commented Jun 26, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling c66d1cd on jaapcoomans:service-loader into 86b6096 on jwtk:master.

@jaapcoomans
Copy link
Contributor Author

Strange... the builds for JDK7 and JDK10 failed with the following error:

Unresolveable build extension: Plugin org.apache.felix:maven-bundle-plugin:3.3.0 or one of its dependencies could not be resolved: The following artifacts could not be resolved: org.apache.felix:maven-bundle-plugin:jar:3.3.0, biz.aQute.bnd:biz.aQute.bndlib:jar:3.3.0, org.easymock:easymock:jar:3.4, org.objenesis:objenesis:jar:2.2, org.apache.felix:org.apache.felix.utils:jar:1.6.0, org.apache.maven:maven-plugin-parameter-documenter:jar:2.2.0: Could not transfer artifact org.apache.felix:maven-bundle-plugin:jar:3.3.0 from/to central (https://repo.maven.apache.org/maven2): Access denied to: https://repo.maven.apache.org/maven2/org/apache/felix/maven-bundle-plugin/3.3.0/maven-bundle-plugin-3.3.0.jar , ReasonPhrase:Forbidden.

Neither the JDK8 or JDK9 builds failed for this reason, while they're using the exact same plugin. Also, when I manually look up all artifacts mentioned in above error message I can all download them from Maven central. Must have been a glitch or something.

Will close and re-open to trigger a fresh build.

@jaapcoomans jaapcoomans reopened this Jun 26, 2019
@jaapcoomans
Copy link
Contributor Author

So this time the JDK7 and 10 builds succeeded, but now the JDK9 build failed for the same reason. That's eerily flaky. I unfortunately can't connect the error nor the flakiness with the changes in the PR.

@jaapcoomans
Copy link
Contributor Author

Turns out more people are experiencing similar issues with Travis and Maven Central: https://travis-ci.community/t/continuous-maven-repo-403/3908. Apparently Travis is currently investigating.

@jaapcoomans
Copy link
Contributor Author

An issue was also reported at Sonatype: https://issues.sonatype.org/browse/MVNCENTRAL-4985. According to the comments on that issue several more IP's are now whitelisted. Will trigger a build again to see if this resolved the issue.

@jaapcoomans jaapcoomans reopened this Jun 27, 2019
@jaapcoomans
Copy link
Contributor Author

According to the Travis incident report the issue is now resolved. https://www.traviscistatus.com/incidents/mm020jn4nzcn

@jaapcoomans jaapcoomans reopened this Jun 28, 2019
@lhazlewood
Copy link
Contributor

@jaapcoomans I just wanted to let you know I haven't forgotten about this - but it is substantial and I need to dig in to it a little further. For example, for semantic versioning reasons, we cannot move existing classes or interfaces into other packages - they must remain in the packages where they currently are, and it's not yet clear to me if your PR re-arranges packages yet.

Also, things in the api module are strictly to be used by end-users of JJWT and should not expose anything they wouldn't use. The Services class looks like it might be such a thing that shouldn't be exposed (but again, I haven't dug in to it enough). I'll have more feedback as soon as I can look into it.

Thanks!

@lhazlewood
Copy link
Contributor

P.S. I do want to say that this PR does look promising! I like the idea of the ServiceLoader concept!

@lhazlewood
Copy link
Contributor

Also, @jaapcoomans have you tested this on Android? Does ServiceLoader and .jar metadata work without any issues on Android?

@lhazlewood
Copy link
Contributor

Looks like it has been available in Android since Service Level 9, aka "Gingerbread":

https://developer.android.com/reference/java/util/ServiceLoader

@jaapcoomans
Copy link
Contributor Author

jaapcoomans commented Jul 13, 2019

Hi @lhazlewood, thanks for the constructive feedback :) I must admit I haven't tested on Android, as I have no idea how to do that. Is support starting from Gingerbread sufficient in your opinion?

I took the backwards compatibility requirement into account. If I'm not mistaken the PR does not break clients of the api module.

@lhazlewood
Copy link
Contributor

I think that because Gingerbread onwards has support for ServiceLoader, we should be ok. I'll hopefully be able to add PR review comments soon. Thanks!

@bdemers
Copy link
Member

bdemers commented Sep 24, 2019

Resolved conflicts and continued work in #496

Thanks again @jaapcoomans !!

bdemers added a commit that referenced this pull request Oct 1, 2019
bdemers added a commit that referenced this pull request Oct 17, 2019
bdemers added a commit that referenced this pull request Oct 17, 2019
bdemers added a commit that referenced this pull request Oct 17, 2019
bdemers added a commit that referenced this pull request Oct 23, 2019
bdemers added a commit that referenced this pull request Oct 24, 2019
Replace hardcoded class names and reflection with ServiceLoader

Fixes: #458
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 this pull request may close these issues.

4 participants