Skip to content

Update Jackson (JSON 3.x), and use of java.net.http#74

Open
smocken78 wants to merge 14 commits into
FusionAuth:mainfrom
smocken78:modernize
Open

Update Jackson (JSON 3.x), and use of java.net.http#74
smocken78 wants to merge 14 commits into
FusionAuth:mainfrom
smocken78:modernize

Conversation

@smocken78

Copy link
Copy Markdown

Updated depencies from com.fasterxml.jackson to tools.jackson where possible and migrated from URLConnection to HttpClient.
From my own test and my usage of the package there were no problems, especially with the upgraded dependencies, but i had no usecase where needed to use HttpClient, so this should be tested.

@smocken78 smocken78 requested a review from a team as a code owner February 28, 2026 10:40
@robotdan

robotdan commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution @smocken78 !

Some feedback to improve your PR:

  • Remove .settings, .classpath and .project, these look to be from your local IDE config
  • Change 10000 to 10_000 to preserve readability of larger numbers.
  • Preserve existing copyright notices in files

Some open questions:

  • Is the upgrade of Jackson to account for a known vulnerability or performance benefits?
  • Is there a functional benefit to changing from HttpURLConnection to HttpClient?

Comments:

One of the reasons Jackson renamed the packages in version 3 is to allow for versions 2 and 3 to co-exist in the classpath. So I'm assuming this change isn't for compatibility for your usage - perhaps there is another driver?

Based upon your implementation - it does looks like you are already aware of some of the issues with HttpClient - and in particular, that each instance of this object builds a thread pool. This is a bit unfortunate because it requires to maintain an instance for re-use. And because object itself is AutoCloseable - ideally we'd be able to close it on shutdown. Although if the VM is shutting down anyway, it likely doesn't matter much if we ever call close() on this instance, but it would be ideal.

One option is when you hold a static reference to a closeable - is for the enclosing class could also implement AutoCloseable and it's close method would call close on the instance if non-null.

This gets a bit more complicated if you add the setHttpClient method into the mix since it would be common practice to expect the provider to close their own resources.

Creating one in a static field works, but does then does build a thread pool even if no methods are called. This instance could be lazily instantiated instead.

In theory there is nothing wrong with changing to HttpClient as long as performance is well tested and the same configuration options are used - but I'm assuming the prior implementation of HttpURLConnection was working ok already or did you run into some issues at runtime?

Thanks!

@robotdan

robotdan commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

A few additional findings from reviewing the diff:

module-info.java has bugs

The current PR has:

requires tools.jackson.databind.annotation;
requires tools.jackson.core;
requires com.fasterxml.jackson.databind;

This has two issues:

  1. tools.jackson.databind.annotation is a Java package inside the tools.jackson.databind module, not a separate module. There is no module by that name.
  2. com.fasterxml.jackson.databind is the Jackson 2.x module name. Should be tools.jackson.databind for 3.x.

It should be:

requires com.fasterxml.jackson.annotation;    // annotations (unchanged in 3.x)
requires tools.jackson.core;                  // jackson-core 3.x
requires tools.jackson.databind;              // jackson-databind 3.x

(Verified against the actual module-info.java in the Jackson 3.x repos: jackson-core, jackson-databind, jackson-annotations)

Breaking API changes

The retrieveKeysFromJWKS(HttpURLConnection) and retrieveKeysFromWellKnownConfiguration(HttpURLConnection) public overloads were removed. Consumers using those methods will break at compile time. Options to consider:

  1. Overload: Keep the HttpURLConnection signatures alongside the new HttpRequest.Builder ones (backwards compatible)
  2. Revert: Keep the existing API surface and only migrate internals
  3. Ship as-is with 7.0.0: Since the version is already bumped to 7.0.0, breaking changes are semver-permitted, but should be documented in release notes / changelog so consumers know what to migrate

Exception type change

The JWKS retrieval methods (retrieveKeysFromJWKS, retrieveKeysFromWellKnownConfiguration) changed from throwing JSONWebKeyBuilderException to JSONWebKeySetException. These are sibling types (both extend RuntimeException directly), so callers catching JSONWebKeyBuilderException around JWKS retrieval will silently miss exceptions after upgrading.

JSONWebKeySetException is arguably the more correct type for key set retrieval operations (vs building), but it is a breaking change for anyone catching the specific exception type. If shipping as 7.0.0, this should also be documented.

Code quality

  • Inconsistent indentation: Mixed tabs and spaces throughout, with varying nesting depth (particularly in pom.xml and AbstractHttpHelper.java). Several files are also missing trailing newlines.
  • Test line-ending normalization: Multiple tests now have .replace("\r\n", "\n") on both sides of assertions. This appears to be working around a platform-specific issue rather than addressing the root cause. Ideally the PEM encoding/decoding would produce consistent line endings regardless of platform, rather than normalizing in every test assertion.

@robotdan

robotdan commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

One more note on the breaking changes — this PR introduces several:

  1. Removed HttpURLConnection overloads (compile-time break for consumers)
  2. Exception type change from JSONWebKeyBuilderException to JSONWebKeySetException (silent runtime behavior change)
  3. Consumer<HttpURLConnection> signatures changed to Consumer<HttpRequest.Builder> (compile-time break)
  4. Jackson 3.x transitive dependency change (classpath/module impact on consumers)

Collectively these force a major version bump (6.x → 7.0.0). Generally, major version bumps to a library should be avoided unless driven by a security fix or architectural necessity — they impose upgrade cost on every downstream consumer. If the primary goal is to modernize internals (HttpClient, Jackson 3.x), it may be worth keeping the public API surface stable and only changing the implementation behind the existing method signatures. That way consumers get the benefits without a forced migration.

@voidmain

voidmain commented Mar 6, 2026

Copy link
Copy Markdown
Member

Adding on to the URLConnection versus HttpClient discussion. HttpClient is known to be significantly slower than URLConnection (https://bugs.openjdk.org/browse/JDK-8277519?page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel).

I don't think this is an issue because JWKS and other requests are sporadic. Plus, the round-trip latency should not cause this library or applications using it issues. However, we should just make sure that any performance issues are considered and documented as not a concern.

@smocken78

smocken78 commented Mar 8, 2026

Copy link
Copy Markdown
Author

Thanks for reviewing, i will look into the bugs.
The upgrade to jackson3 was mainly done, because i did not want to mix the use of jackson2 and jackson3 in my applications.

In case of HttpClient there is no functional benefit, the main reason why i changed HttpUrlConnection to HttpClient was that in my personal application tests HttpClient had a slightly better performance over URLConnection since i upgraded to Java 21.
Probably the use of HttpClient is only reasonable when using java >= 21.

Regarding the "Test line-ending normalization", yes this was done due to a plattform specifc error on some Windows machine. I will have a look into this. :(

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