Skip to content

Conversation

@rli
Copy link
Contributor

@rli rli commented Oct 21, 2024

Jackson's class resolver, if not defined on its TypeFactory, first tries to use Thread#getContextClassLoader of the current thread to attempt to load the requested type. On failure it will then attempt to use its own classloader. Since by design the default classloader is not defined, the first load attempt will fail (since the context classloader is the root IDE classpath) and fall back to using the classloader that loaded Jackson.

Effectively, this means that if we want to avoid setting the type classloader for every Jackson ObjectMapper instance, we must always have Jackson in the plugin classloader, rather than using the version bundled by the IDE so that it can find the classes it needs.

At the moment we only include Jackson in plugin-core, but it introduces a question on what happens if a leaf plugin (i.e. plugin-amazonq/plugin-toolkit) loads Jackson through core. Will Jackson successfully resolve classes correctly? Inspecting the conetents of v3.20, when we fixed our last Jackson issue, the answer seems to be yes.

Interestingly, though this change brings our packaging back to v3.19, we do not seem to be facing the same deserialization issue on 2023.3 as last time (#4732)

In Jackson 2.16.x, the Kotlin module uses reflection on the class annotations to determine if it should step in to deserialize a Kotlin class.
However in Jackson 2.17.x, they switched to the standard isAnnotationPresent call, which ultimately performs an identity check on the annotation.
In <241, the plugin classloader for the Q plugin INCORRECTLY loads the Kotlin metadata class from the Kotlin plugin, which results in isKotlinClass returning false (because the identity check fails if loaded from different classloaders) and then we get the cryptic cannot deserialize from Object value (no delegate- or property-based Creator) from the KotlinModule noop-ing.

And as always, unit testing of classloader issues is not possible at the moment because we are not set up for E2E tests and the classloader in headless tests differs from that of a running IDE.

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

Jackson's class resolver, if not defined on its `TypeFactory`, first tries to use `Thread#getContextClassLoader` of the current thread to attempt to load the requested type. On failure it will then attempt to use its own classloader.
Since by design the default classloader is not defined, the first load attempt will fail (since the context classloader is the root IDE classpath) and fall back to using the classloader that loaded Jackson.

Effectively, this means that if we want to avoid setting type classloader for every Jackson ObjectMapper instance, we must always have Jackson in the plugin classloader, rather than using the version bundled by the IDE so that it can find the classes it needs.

At the moment we only include Jackson in plugin-core, but it introduces a question on what happens if a leaf plugin (i.e. plugin-amazonq/plugin-toolkit) loads Jackson through core. Will Jackson successfully resolve classes correctly?
Inspecting the conetents of v3.20, when we fixed our last Jackson issue, the answer seems to be yes.

Interestingly, though this change brings our packaging back to v3.19, we do not seem to be facing the same deserialization issue on 2023.3 as last time
> In Jackson 2.16.x, the Kotlin module uses reflection on the class annotations to determine if it should step in to deserialize a Kotlin class.
> However in Jackson 2.17.x, they switched to the standard isAnnotationPresent call, which ultimately performs an identity check on the annotation.
> In <241, the plugin classloader for the Q plugin INCORRECTLY loads the Kotlin metadata class from the Kotlin plugin, which results in isKotlinClass returning false (because the identity check fails if loaded from different classloaders) and then we get the cryptic cannot deserialize from Object value (no delegate- or property-based Creator) from the KotlinModule noop-ing.

And as always, unit testing of classloader issues is not possible at the moment because we are not set up for E2E tests and the classloader in headless tests differs from that of a running IDE.
@rli rli requested a review from a team as a code owner October 21, 2024 22:50
@github-actions
Copy link

Qodana Community for JVM

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

@rli rli merged commit f331fb6 into main Oct 22, 2024
16 of 17 checks passed
@rli rli deleted the rli/jackson-classloader-pt2 branch October 22, 2024 00:22
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.

2 participants