deepakpanda93 commented on code in PR #19624:
URL: https://github.com/apache/hudi/pull/19624#discussion_r3967574876
##########
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:
Applied. The prefix now comes from the connection rather than being derived
from `packageName`:
```java
String entryPrefix = jarConnection.getEntryName();
...
.filter(name -> name.startsWith(prefix) && name.endsWith(CLASS_FILE_SUFFIX))
.map(name -> packageName + '.'
+ name.substring(prefix.length(), name.length() -
CLASS_FILE_SUFFIX.length()).replace('/', '.'))
```
Names are anchored at `packageName` the way `findClasses` does, so a
versioned entry reports the same class name a non-versioned one would. Two
guards came with it: `getEntryName()` is null for a `jar:` URL naming no entry,
and it has no trailing slash when the loader hands back the bare package path,
so it is normalised before use.
Taking it despite no Hudi bundle being multi-release today, because deriving
the prefix from the connection is simply the correct anchor — the old form
happened to agree with it only in the non-versioned case.
##########
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:
Applied, using your snippet:
```java
} else if (file.getName().endsWith(CLASS_FILE_SUFFIX)) {
classes.add(packageName + '.'
+ file.getName().substring(0, file.getName().length() -
CLASS_FILE_SUFFIX.length()));
}
```
Both branches read alike now, and the magic `6` is gone.
##########
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:
Applied — you are right that the second assertion was a duplicate of the
first branch, so "primitives" was untested:
```java
assertEquals(0, getTopLevelClassesInClasspath(String[].class).count());
assertEquals(0, getTopLevelClassesInClasspath(int.class).count());
```
An array and a genuine primitive now, and the comment says so.
##########
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:
Applied, using your wording. You are right about the mechanism — surefire
does put `hudi-common/target/classes` on as a directory, which is exactly why
the pre-existing `SkipsInvalidResources` test passes on master, so my stated
reason was wrong. The fixture earns its place by pinning an exact set, which is
what the javadoc now says.
##########
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:
Applied. Split into `withContextClassLoader(ClassLoader, Supplier<T>)` and a
`Path...` overload that builds the `URLClassLoader` and delegates to it, and
the two pre-existing tests at :84 and :102 now use the former instead of
open-coding the save/restore. One copy of that `finally` in the file now.
##########
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:
Not doing this one in this PR, though I think you are right about where the
tests belong.
Moving only the three new tests would split coverage of one class across two
modules: `TestReflectionUtils` in `hudi-common` would keep `testIsSubClass`,
`testGetMethod` and the two tests #19784 added, while `hudi-io` held the other
three. Someone changing `getTopLevelClassesInClasspath` would then have two
files to find. Moving the whole file is the coherent version of the change, but
that is a rename of a file #19784 just touched, and it would bury this diff.
Happy to do the whole-file move as a follow-up if you would like it — it is
a clean standalone change, and `hudi-io` already having `TestStringUtils` and
`TestValidationUtils` in that package is a good argument for it.
--
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]
