utafrali commented on code in PR #8382:
URL: https://github.com/apache/hop/pull/8382#discussion_r4013957789
##########
core/src/main/java/org/apache/hop/core/plugins/HopURLClassLoader.java:
##########
@@ -108,8 +108,20 @@ protected Class<?> loadClassFromThisLoader(String arg0,
boolean arg1)
return clz;
}
+ /**
+ * Packages a plugin must share with the core classloader so objects can
cross the boundary (Hop
+ * core types, the Jackson streaming/databind API, SLF4J). Jackson *modules*
are deliberately not
+ * listed: plugins bring their own — e.g. jackson-module-scala is compiled
per Scala version, so
+ * the Beam engine (Scala 2.12) and the native Spark engine (Scala 2.13)
each need the copy in
+ * their own lib, and a parent-first lookup of the wrong one fails at
runtime with "no Creators"
+ * when Spark deserializes its own Scala classes.
+ */
private static final String[] SYSTEM_PARENT_FIRST_PACKAGES = {
Review Comment:
The narrowing from `com.fasterxml.jackson.` to the three specific
sub-packages is the right call for the Scala-module problem, but it also flips
`com.fasterxml.jackson.dataformat.*` and `com.fasterxml.jackson.datatype.*`
from parent-first to plugin-first. If a plugin bundles (say)
`jackson-datatype-jsr310` and passes a `ZonedDateTime`-annotated object across
the plugin boundary to core's `ObjectMapper`, you can get a
`ClassCastException` because the two classloaders will have loaded different
`JavaTimeModule` instances. This is unlikely in practice right now, but worth a
sentence in the Javadoc so future contributors understand the scope of the
decision: not just `jackson-module-*` is excluded, but all Jackson packages
beyond `core`, `databind`, and `annotation`.
##########
core/src/test/java/org/apache/hop/core/plugins/HopURLClassLoaderTest.java:
##########
@@ -77,6 +78,13 @@ void testSystemParentFirstClasses() throws Exception {
Class<?> hopJsonClass =
loader.loadClass("org.apache.hop.core.json.HopJson");
assertEquals(org.apache.hop.core.json.HopJson.class, hopJsonClass);
assertEquals(getClass().getClassLoader(), hopJsonClass.getClassLoader());
+
+ // Jackson modules are not shared: a plugin's own (e.g. Scala-version
specific) copy must win
+
assertFalse(loader.isParentFirst("com.fasterxml.jackson.module.scala.DefaultScalaModule"));
+
assertFalse(loader.isParentFirst("com.fasterxml.jackson.datatype.jsr310.JavaTimeModule"));
Review Comment:
The `assertFalse` for `com.fasterxml.jackson.datatype.jsr310.JavaTimeModule`
covers a module that is not in the codebase's test classpath and therefore
exercises `isParentFirst` in isolation rather than actually loading the class.
That is fine and intentional here, but consider adding a brief comment like `//
isParentFirst check only — the class need not be on the classpath` so future
readers don't wonder why there is no corresponding `loadClass` assertion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
