voonhous commented on code in PR #19624:
URL: https://github.com/apache/hudi/pull/19624#discussion_r3967273344
##########
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:
**minor:** Nothing pins this line: deleting it leaves all 8 tests green
(checked by mutation), yet without it the scan closes the JVM-wide cached
`JarFile`, and any reader that opened the same jar first then gets
`IllegalStateException: zip file closed` (reproduced on JDK 8/11/17/25). A
second scan does not catch it either, since the closed entry is evicted from
the cache. Not blocking, but could `testGetTopLevelClassesInClasspathFromJar`
open a `JarURLConnection` on the jar before the scan and assert
`shared.getEntry(dir + "Alpha.class")` is still readable afterwards?
##########
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:
**minor:** Small coverage regression: the `"jar:file:/unused.jar!/..."`
parameter of `testGetTopLevelClassesInClasspathSkipsInvalidResources` (:82)
used to land in `toDirectory`'s `IllegalArgumentException` arm and now routes
to `classNamesInJar`, so nothing in the suite reaches that arm any more. Not
blocking: could a third `@ValueSource` entry such as
`"http://example.invalid/org/apache/hudi/common/util"` be added? `new
File(URI)` rejects it with `URI scheme is not "file"`, which restores the arm.
##########
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:
**nit:** Every new test uses a single classpath root, so the `flatMap` union
over several `getResources` hits (the surefire shape:
`org.apache.hudi.common.util` is in the hudi-io jar and in
`hudi-common/target/classes` at once) works but is never asserted. Feel free to
ignore: could this helper take varargs roots so one test asserts that `{jar,
directory}` yields both sets?
##########
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('.', '/') + '/';
Review Comment:
**nit:** On a multi-release jar the class loader resolves the package URL to
`META-INF/versions/N/<pkg>/`, so a prefix derived from `packageName` never
matches the versioned entries and classes that exist only there are invisible.
No Hudi bundle is multi-release today, so feel free to ignore: could the prefix
come from `connection.getEntryName()` instead, with names built as `packageName
+ '.' + relativePath.replace('/', '.')` the way `findClasses` anchors them?
##########
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:
**nit:** A `jar:` URL served by a non-JDK handler makes this cast throw
`ClassCastException`, which the `IOException` catch below does not cover, so an
entry master logged and skipped now throws. Only reachable with a custom
`URLStreamHandlerFactory`, so feel free to ignore: could this be `URLConnection
conn = resource.openConnection()` plus an `instanceof JarURLConnection` check
that falls through to log-and-skip?
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -44,6 +48,7 @@
public class ReflectionUtils {
private static final Map<String, Class<?>> CLAZZ_CACHE = new
ConcurrentHashMap<>();
+ private static final String CLASS_FILE_SUFFIX = ".class";
Review Comment:
**nit:** This constant is only used by the jar branch; `findClasses`
(:234-235) still carries the `".class"` literal and the magic `6`. Feel free to
ignore, but could both branches read alike?
```java
} else if (file.getName().endsWith(CLASS_FILE_SUFFIX)) {
classes.add(packageName + '.' + file.getName().substring(0,
file.getName().length() - CLASS_FILE_SUFFIX.length()));
}
```
##########
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:
**minor:** `findClasses` still has one throw path: `File#listFiles()`
returns null for an unreadable package directory or for a package path that is
a regular file, and `Objects.requireNonNull(files)` at :231 lets that NPE
escape `getTopLevelClassesInClasspath`. Pre-existing, but it is the same class
of throw this PR removes elsewhere. Not blocking: could `findClasses` return an
empty list when `listFiles()` is null, with a "package path is a regular file"
fixture as the test (deterministic, no chmod needed)?
##########
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.
Review Comment:
**nit:** The stated reason is not quite right: surefire puts
`hudi-common/target/classes` on the test classpath as a directory (that is what
makes the pre-existing `SkipsInvalidResources` test pass on master), so a
real-classpath scan does enter this branch. The fixture still earns its keep
because it pins an exact set. Feel free to ignore:
```suggestion
* An exploded directory on the classpath, reached over the "file"
protocol. Built explicitly
* rather than scanning the test classpath, so the expected set is exact
and independent of
* whatever else surefire puts on the classpath.
```
##########
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:
**nit:** These three tests need nothing from hudi-common, but the class
under test lives in hudi-io, and
`hudi-io/src/test/java/org/apache/hudi/common/util/` already holds
`TestStringUtils` and `TestValidationUtils`. Feel free to ignore since #19784
set the precedent here: would it be worth putting the three new tests next to
those, so hudi-io's own suite covers the branch being fixed?
##########
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 {
+ ClassLoader original = Thread.currentThread().getContextClassLoader();
Review Comment:
**nit:** This is the third copy of the TCCL save/restore in the file (:84
and :102 open-code the same `original` / `finally` pair). Feel free to ignore:
could it be split into `withContextClassLoader(ClassLoader, Supplier<T>)` plus
this `Path` overload, so the two pre-existing tests can use it too?
##########
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());
Review Comment:
**nit:** `String[]` and `int[]` are both arrays, so the second assertion
hits the identical branch and the "primitives" half of the test name (and of
the PR body) is untested. `int.class.getPackage()` and
`void.class.getPackage()` are also null. Feel free to ignore:
```suggestion
assertEquals(0, getTopLevelClassesInClasspath(int.class).count());
```
--
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]
