deepakpanda93 commented on code in PR #19624:
URL: https://github.com/apache/hudi/pull/19624#discussion_r3967561660
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -130,19 +135,71 @@ 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) {
+ String entryPrefix = packageName.replace('.', '/') + '/';
+ try {
+ JarURLConnection connection = (JarURLConnection)
resource.openConnection();
+ // Without this the JarFile is cached and shared, and closing it below
would break other readers.
+ connection.setUseCaches(false);
Review Comment:
Applied.
`testGetTopLevelClassesInClasspathFromJarLeavesASharedJarFileUsable` opens the
jar through the shared cache first, runs the scan, then reads an entry back:
```java
JarURLConnection shared = (JarURLConnection) entry.openConnection();
shared.setUseCaches(true);
JarFile sharedJar = shared.getJarFile();
try {
withContextClassLoaderOver(() ->
getTopLevelClassesInClasspath(ReflectionUtils.class).count(), jar);
assertDoesNotThrow(() -> sharedJar.getEntry(dir + "Alpha.class"),
"the scan must not close a JarFile held open through the JVM-wide
cache");
} finally {
sharedJar.close();
}
```
Confirmed by the same mutation you ran — deleting `setUseCaches(false)` now
fails, with exactly the error you predicted:
```
AssertionFailedError: the scan must not close a JarFile held open through
the JVM-wide cache
==> Unexpected exception thrown: java.lang.IllegalStateException: zip file
closed
```
Good catch that the line was unpinned; it was doing real work with nothing
holding it in place.
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -130,19 +135,71 @@ 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);
Review Comment:
Applied — it is the same class of throw this PR removes, so it belongs here
rather than in a follow-up.
```java
File[] files = directory.listFiles();
if (files == null) {
// Null for an unreadable directory, or for a package path that is a
regular file. Skipping it
// keeps one bad classpath entry from failing the whole scan, as the jar
branch above does.
log.warn("Unable to list {}, skipping it", directory);
return classes;
}
```
Test is the deterministic fixture you suggested:
`testGetTopLevelClassesInClasspathWhenThePackagePathIsAFile` writes a regular
file where the package directory would be, so `listFiles()` returns null with
no chmod involved. Restoring `Objects.requireNonNull(files)` makes it fail with
`NullPointerException`, so the guard is pinned.
##########
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
+ void testGetTopLevelClassesInClasspathFromDirectory(@TempDir Path tempDir)
throws Exception {
+ String scanned = ReflectionUtils.class.getPackage().getName();
+ Path root = tempDir.resolve("classes");
+ Path pkgDir = root.resolve(scanned.replace('.', File.separatorChar));
+ Files.createDirectories(pkgDir.resolve("nested"));
+ Files.write(pkgDir.resolve("Alpha.class"), new byte[] {1});
+ Files.write(pkgDir.resolve("Beta.class"), new byte[] {1});
+ Files.write(pkgDir.resolve("nested").resolve("Gamma.class"), new byte[]
{1});
+ Files.write(pkgDir.resolve("notaclass.txt"), new byte[] {1});
+
+ List<String> classes = withContextClassLoaderOver(root, () ->
+
getTopLevelClassesInClasspath(ReflectionUtils.class).collect(Collectors.toList()));
+
+ assertEquals(
+ Arrays.asList(scanned + ".Alpha", scanned + ".Beta", scanned +
".nested.Gamma"),
+ classes.stream().sorted().collect(Collectors.toList()),
+ "a directory entry must yield the classes of the package and its
subpackages, and nothing else");
+ }
+
+ /**
+ * Classes packaged in a jar are reached over the "jar" protocol, whose URLs
are not hierarchical
+ * and so cannot be turned into a {@link File}. Every caller of this method
is a bundle
+ * Main class, which is exactly the packaged case.
+ * <p>
+ * The jar is built under the scanned class's own package, and the loader is
given no parent, so
+ * the only resource found for that package is the one written here.
+ */
+ @Test
+ void testGetTopLevelClassesInClasspathFromJar(@TempDir Path tempDir) throws
Exception {
Review Comment:
Applied — you are right that my change silently vacated that arm.
```java
@ValueSource(strings = {"jar:file:/unused.jar!/org/apache/hudi/common/util",
"file:/invalid path",
"http://example.invalid/org/apache/hudi/common/util"})
```
`new File(URI)` rejects the third with `URI scheme is not "file"`, so
`toDirectory`'s `IllegalArgumentException` arm is reached again. The
parameterized test now runs 3 cases and the suite is 12/12.
##########
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
+ void testGetTopLevelClassesInClasspathFromDirectory(@TempDir Path tempDir)
throws Exception {
+ String scanned = ReflectionUtils.class.getPackage().getName();
+ Path root = tempDir.resolve("classes");
+ Path pkgDir = root.resolve(scanned.replace('.', File.separatorChar));
+ Files.createDirectories(pkgDir.resolve("nested"));
+ Files.write(pkgDir.resolve("Alpha.class"), new byte[] {1});
+ Files.write(pkgDir.resolve("Beta.class"), new byte[] {1});
+ Files.write(pkgDir.resolve("nested").resolve("Gamma.class"), new byte[]
{1});
+ Files.write(pkgDir.resolve("notaclass.txt"), new byte[] {1});
+
+ List<String> classes = withContextClassLoaderOver(root, () ->
+
getTopLevelClassesInClasspath(ReflectionUtils.class).collect(Collectors.toList()));
+
+ assertEquals(
+ Arrays.asList(scanned + ".Alpha", scanned + ".Beta", scanned +
".nested.Gamma"),
+ classes.stream().sorted().collect(Collectors.toList()),
+ "a directory entry must yield the classes of the package and its
subpackages, and nothing else");
+ }
+
+ /**
+ * Classes packaged in a jar are reached over the "jar" protocol, whose URLs
are not hierarchical
+ * and so cannot be turned into a {@link File}. Every caller of this method
is a bundle
+ * Main class, which is exactly the packaged case.
+ * <p>
+ * The jar is built under the scanned class's own package, and the loader is
given no parent, so
+ * the only resource found for that package is the one written here.
+ */
+ @Test
+ void testGetTopLevelClassesInClasspathFromJar(@TempDir Path tempDir) throws
Exception {
+ String scanned = ReflectionUtils.class.getPackage().getName();
+ String dir = scanned.replace('.', '/') + "/";
+ Path jar = tempDir.resolve("classes.jar");
+ writeJar(jar,
+ dir,
+ dir + "Alpha.class",
+ dir + "Beta.class",
+ dir + "nested/",
+ dir + "nested/Gamma.class",
+ dir + "notaclass.txt",
+ "org/example/other/Delta.class");
+
+ List<String> classes = withContextClassLoaderOver(jar, () ->
+
getTopLevelClassesInClasspath(ReflectionUtils.class).collect(Collectors.toList()));
+
+ assertEquals(
+ Arrays.asList(scanned + ".Alpha", scanned + ".Beta", scanned +
".nested.Gamma"),
+ classes.stream().sorted().collect(Collectors.toList()),
+ "a jar entry must yield the classes of the package and its
subpackages, and nothing else");
+ }
+
+ @Test
+ void testGetTopLevelClassesInClasspathForClassesWithoutAPackage() {
+ // Arrays have no package, which used to dereference null.
+ assertEquals(0, getTopLevelClassesInClasspath(String[].class).count());
+ assertEquals(0, getTopLevelClassesInClasspath(int[].class).count());
+ }
+
+ /** Writes a jar holding the given entry names; names ending in "/" become
directory entries. */
+ private static void writeJar(Path jar, String... entryNames) throws
IOException {
+ try (JarOutputStream out = new
JarOutputStream(Files.newOutputStream(jar))) {
+ for (String entryName : entryNames) {
+ out.putNextEntry(new JarEntry(entryName));
+ if (!entryName.endsWith("/")) {
+ // Content is irrelevant: the scan reads entry names, never the
bytecode.
+ out.write(new byte[] {1, 2, 3});
+ }
+ out.closeEntry();
+ }
+ }
+ }
+
+ /**
+ * Runs the supplier with the thread context class loader reading only from
the given classpath
+ * root, which may be a jar or an exploded directory. The loader is given no
parent so the scan
+ * sees nothing else.
+ */
+ private static <T> T withContextClassLoaderOver(Path root, Supplier<T>
supplier) throws IOException {
Review Comment:
Applied. `testGetTopLevelClassesInClasspathUnionsEveryClasspathEntry` puts
`FromJar.class` in a jar and `FromDirectory.class` in a directory, both under
the scanned package, and asserts both come back — so the `flatMap` union across
`getResources` hits is now asserted, and it covers both protocols in one test.
The helper takes varargs roots as you suggested; the two single-root tests
just pass one.
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -130,19 +135,71 @@ 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) {
+ String entryPrefix = packageName.replace('.', '/') + '/';
+ try {
+ JarURLConnection connection = (JarURLConnection)
resource.openConnection();
Review Comment:
Applied. Worth doing even though it needs a custom handler to reach, because
the whole point of this change is that one bad classpath entry should not fail
the scan — a new throw path would work against that.
```java
URLConnection connection = resource.openConnection();
if (!(connection instanceof JarURLConnection)) {
log.warn("Skipping classpath entry {}, {} is not a JarURLConnection",
resource, connection.getClass());
return Stream.empty();
}
```
Same log-and-skip outcome master had for these entries.
--
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]
