deepakpanda93 commented on code in PR #19624:
URL: https://github.com/apache/hudi/pull/19624#discussion_r3968210653
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -130,19 +135,91 @@ public static Object loadClass(String clazz, Object...
constructorArgs) {
*/
public static Stream<String> getTopLevelClassesInClasspath(Class<?> clazz) {
ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
- String packageName = clazz.getPackage().getName();
+ // Arrays and primitives have no package, and Class#getPackage is also
null when the class was
+ // loaded by a loader that defines no package for it.
+ Package pkg = clazz.getPackage();
+ if (pkg == null) {
+ return Stream.empty();
+ }
+ String packageName = pkg.getName();
String path = packageName.replace('.', '/');
try {
return Collections.list(classLoader.getResources(path)).stream()
- .map(ReflectionUtils::toDirectory)
- .filter(Objects::nonNull)
- .flatMap(directory -> findClasses(directory, packageName).stream());
+ .flatMap(resource -> classNamesIn(resource, packageName));
} catch (IOException e) {
log.error("Unable to fetch Resources in package {}", packageName, e);
return Stream.empty();
}
}
+ /**
+ * Class names under a single classpath entry for the package, whether that
entry is an exploded
+ * directory or a jar.
+ *
+ * <p>A jar entry cannot go through {@link #toDirectory}: a {@code jar:} URL
is non-hierarchical,
+ * so {@code new File(uri)} throws and the entry would be dropped. Every
bundle {@code Main} class
+ * runs from inside a shaded jar, so that path has to be read through the
jar connection instead.
+ *
+ * @param resource a classpath entry holding the package
+ * @param packageName the package being scanned
+ * @return class names found under that entry, empty if it cannot be read
+ */
+ private static Stream<String> classNamesIn(URL resource, String packageName)
{
+ if ("jar".equals(resource.getProtocol())) {
+ return classNamesInJar(resource, packageName);
+ }
+ File directory = toDirectory(resource);
+ return directory == null ? Stream.empty() : findClasses(directory,
packageName).stream();
+ }
+
+ /**
+ * Class names under the package inside a jar, read through the jar
connection.
+ *
+ * @param resource a {@code jar:} classpath entry holding the package
+ * @param packageName the package being scanned
+ * @return class names found in that jar, empty if the jar cannot be read
+ */
+ private static Stream<String> classNamesInJar(URL resource, String
packageName) {
+ try {
+ URLConnection connection = resource.openConnection();
+ if (!(connection instanceof JarURLConnection)) {
+ // A jar: URL served by a non-JDK stream handler. Skip it rather than
let the cast throw,
+ // since this method exists to stop such an entry from failing the
whole scan.
+ log.warn("Skipping classpath entry {}, {} is not a JarURLConnection",
resource, connection.getClass());
+ return Stream.empty();
+ }
+ JarURLConnection jarConnection = (JarURLConnection) connection;
+ // Without this the JarFile is cached and shared JVM-wide, and closing
it below would leave
+ // any reader that opened the same jar first with
"IllegalStateException: zip file closed".
+ jarConnection.setUseCaches(false);
+ // The loader resolves the package to this entry, which on a
multi-release jar is
+ // META-INF/versions/N/<pkg>/ rather than <pkg>/. Anchoring to it keeps
those classes visible
+ // and keeps the reported names rooted at packageName, the way
findClasses does.
+ String entryPrefix = jarConnection.getEntryName();
Review Comment:
Reverted to the package-derived prefix, and thanks for catching your own
suggestion — you are right, and it is worse than what it replaced.
I reproduced it on JDK 17 before reverting. A jar with `Alpha` and
`BaseOnly` in the base directory and `Alpha` and `VersionedOnly` under
`META-INF/versions/9/`, loaded through a `URLClassLoader`:
```
resolved URL = jar:file:/tmp/mr.jar!/META-INF/versions/9/org/example/pkg/
getEntryName() = META-INF/versions/9/org/example/pkg/
prefix from getEntryName() -> [org.example.pkg.Alpha,
org.example.pkg.VersionedOnly]
prefix from packageName -> [org.example.pkg.Alpha,
org.example.pkg.BaseOnly]
```
So `BaseOnly` is dropped, exactly as you describe, and which classes come
back depends on the JDK. Losing base-only classes is a worse failure than not
seeing versioned ones, and JDK-dependent output is worse than either.
The code is back to:
```java
String entryPrefix = packageName.replace('.', '/') + '/';
```
The null guard and the slash normalisation went with it, since both only
existed to make `getEntryName()` safe. The comment above it now records why the
package-derived anchor is the deliberate choice rather than an oversight, so
this does not get "fixed" again later.
I did not add the multi-release test. Pinning the entry-name behaviour would
mean pinning something JDK-dependent, and the package-derived prefix is what
the surrounding `findClasses` already assumes. Happy to add a multi-release
fixture as a separate change if you think the base-directory behaviour is worth
locking down on its own.
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -130,19 +135,91 @@ public static Object loadClass(String clazz, Object...
constructorArgs) {
*/
public static Stream<String> getTopLevelClassesInClasspath(Class<?> clazz) {
ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
- String packageName = clazz.getPackage().getName();
+ // Arrays and primitives have no package, and Class#getPackage is also
null when the class was
+ // loaded by a loader that defines no package for it.
+ Package pkg = clazz.getPackage();
+ if (pkg == null) {
+ return Stream.empty();
+ }
+ String packageName = pkg.getName();
String path = packageName.replace('.', '/');
try {
return Collections.list(classLoader.getResources(path)).stream()
- .map(ReflectionUtils::toDirectory)
- .filter(Objects::nonNull)
- .flatMap(directory -> findClasses(directory, packageName).stream());
+ .flatMap(resource -> classNamesIn(resource, packageName));
} catch (IOException e) {
log.error("Unable to fetch Resources in package {}", packageName, e);
return Stream.empty();
}
}
+ /**
+ * Class names under a single classpath entry for the package, whether that
entry is an exploded
+ * directory or a jar.
+ *
+ * <p>A jar entry cannot go through {@link #toDirectory}: a {@code jar:} URL
is non-hierarchical,
+ * so {@code new File(uri)} throws and the entry would be dropped. Every
bundle {@code Main} class
+ * runs from inside a shaded jar, so that path has to be read through the
jar connection instead.
+ *
+ * @param resource a classpath entry holding the package
+ * @param packageName the package being scanned
+ * @return class names found under that entry, empty if it cannot be read
+ */
+ private static Stream<String> classNamesIn(URL resource, String packageName)
{
+ if ("jar".equals(resource.getProtocol())) {
+ return classNamesInJar(resource, packageName);
+ }
+ File directory = toDirectory(resource);
+ return directory == null ? Stream.empty() : findClasses(directory,
packageName).stream();
+ }
+
+ /**
+ * Class names under the package inside a jar, read through the jar
connection.
+ *
+ * @param resource a {@code jar:} classpath entry holding the package
+ * @param packageName the package being scanned
+ * @return class names found in that jar, empty if the jar cannot be read
+ */
+ private static Stream<String> classNamesInJar(URL resource, String
packageName) {
+ try {
+ URLConnection connection = resource.openConnection();
+ if (!(connection instanceof JarURLConnection)) {
Review Comment:
Added, using the shape you described.
`testGetTopLevelClassesInClasspathSkipsAJarUrlThatIsNotAJarConnection` builds
the URL with a handler that hands back a plain `URLConnection` and feeds it
through the same anonymous-`ClassLoader` pattern as `SkipsInvalidResources`:
```java
URL notAJarConnection =
new URL(null, "jar:file:/unused.jar!/org/apache/hudi/common/util",
plainHandler);
```
and asserts the real directory entry still resolves, so the bad entry is
skipped rather than failing the scan.
Confirmed by mutation — removing the guard now fails, where it used to leave
all 12 green:
```
ClassCastException: class ...TestReflectionUtils$3$1 cannot be cast to class
java.net.JarURLConnection
```
You were also right that the body's "each fix is pinned by a test that fails
without it" did not hold while this guard was unpinned. It does now, and the
table in the description lists this case with the others. Suite is 13/13.
##########
hudi-common/src/test/java/org/apache/hudi/common/util/TestReflectionUtils.java:
##########
@@ -101,4 +113,96 @@ public Enumeration<URL> getResources(String name) throws
IOException {
Thread.currentThread().setContextClassLoader(original);
}
}
+
+ /**
+ * An exploded directory on the classpath, reached over the "file" protocol.
Built explicitly
+ * rather than relying on the test classpath, because under Maven the
modules are jars and this
+ * branch would never be entered.
+ */
+ @Test
Review Comment:
Agreed, leaving it. No follow-up unless it comes up again — happy to do the
whole-file move separately if anyone wants `TestReflectionUtils` sitting next
to `TestStringUtils` and `TestValidationUtils` in hudi-io.
--
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]
