hudi-agent commented on code in PR #19784:
URL: https://github.com/apache/hudi/pull/19784#discussion_r3927644087
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -132,26 +135,77 @@ public static Stream<String>
getTopLevelClassesInClasspath(Class<?> clazz) {
ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
String packageName = clazz.getPackage().getName();
String path = packageName.replace('.', '/');
- Enumeration<URL> resources = null;
try {
- resources = classLoader.getResources(path);
+ return Collections.list(classLoader.getResources(path)).stream()
+ .flatMap(resource -> findClasses(resource, packageName).stream());
} catch (IOException e) {
log.error("Unable to fetch Resources in package {}", packageName, e);
+ return Stream.empty();
}
- List<File> directories = new ArrayList<>();
- while (Objects.requireNonNull(resources).hasMoreElements()) {
- URL resource = resources.nextElement();
+ }
+
+ /**
+ * Finds all top-level classes in {@code packageName} reachable from a
package resource URL.
+ *
+ * <p>Supports both {@code file:} URLs (exploded class directories) and
{@code jar:} URLs
+ * (JAR-backed class loaders, e.g. the shaded bundles whose {@code Main}
calls this method).
+ * JAR-backed resources must be scanned through the JAR entries rather than
{@code new File(uri)},
+ * which does not accept {@code jar:} URIs.
+ *
+ * @param resource the package resource URL
+ * @param packageName the package whose classes should be discovered
+ * @return the classes found in {@code packageName} and its subpackages, or
an empty list
+ */
+ private static List<String> findClasses(URL resource, String packageName) {
+ if ("jar".equals(resource.getProtocol())) {
try {
- directories.add(new File(resource.toURI()));
- } catch (URISyntaxException e) {
- log.error("Unable to get URI for {}", resource, e);
+ JarURLConnection connection = (JarURLConnection)
resource.openConnection();
+ try (JarFile jarFile = connection.getJarFile()) {
+ return findClassesInJar(jarFile, packageName);
+ }
+ } catch (IOException e) {
+ log.error("Unable to read JAR resource {} for package {}", resource,
packageName, e);
+ return Collections.emptyList();
}
}
Review Comment:
🤖 The old pipeline filtered out nulls
(`.map(toDirectory).filter(Objects::nonNull)`) before calling `findClasses`.
Here `toDirectory(resource)` can still return `null` (its own javadoc says "or
null if the URI is malformed"), and it's now passed straight into
`findClasses(File, ...)`, which dereferences it via `directory.exists()`. Could
you guard against the null so a malformed `file:` URI skips the resource like
before rather than NPE-ing?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -132,26 +135,77 @@ public static Stream<String>
getTopLevelClassesInClasspath(Class<?> clazz) {
ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
String packageName = clazz.getPackage().getName();
String path = packageName.replace('.', '/');
- Enumeration<URL> resources = null;
try {
- resources = classLoader.getResources(path);
+ return Collections.list(classLoader.getResources(path)).stream()
+ .flatMap(resource -> findClasses(resource, packageName).stream());
} catch (IOException e) {
log.error("Unable to fetch Resources in package {}", packageName, e);
+ return Stream.empty();
}
- List<File> directories = new ArrayList<>();
- while (Objects.requireNonNull(resources).hasMoreElements()) {
- URL resource = resources.nextElement();
+ }
+
+ /**
+ * Finds all top-level classes in {@code packageName} reachable from a
package resource URL.
+ *
+ * <p>Supports both {@code file:} URLs (exploded class directories) and
{@code jar:} URLs
+ * (JAR-backed class loaders, e.g. the shaded bundles whose {@code Main}
calls this method).
+ * JAR-backed resources must be scanned through the JAR entries rather than
{@code new File(uri)},
+ * which does not accept {@code jar:} URIs.
+ *
+ * @param resource the package resource URL
+ * @param packageName the package whose classes should be discovered
+ * @return the classes found in {@code packageName} and its subpackages, or
an empty list
+ */
+ private static List<String> findClasses(URL resource, String packageName) {
+ if ("jar".equals(resource.getProtocol())) {
try {
- directories.add(new File(resource.toURI()));
- } catch (URISyntaxException e) {
- log.error("Unable to get URI for {}", resource, e);
+ JarURLConnection connection = (JarURLConnection)
resource.openConnection();
Review Comment:
🤖 `connection.getJarFile()` with the default `useCaches=true` returns a
JVM-cached `JarFile`, and closing it via try-with-resources can leave a
subsequent `getJarFile()` for the same jar handing back a closed instance
(`IllegalStateException: zip file closed` on `entries()`). Each production
`Main` only calls this once per process so it's likely fine today, but if this
is ever invoked more than once per JVM (or `getResources` returns two entries
for the same jar), it could bite. Would `connection.setUseCaches(false)` before
`getJarFile()` be safer so we truly own the handle we close?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
