utafrali commented on code in PR #19784:
URL: https://github.com/apache/hudi/pull/19784#discussion_r3880297604
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -132,26 +132,30 @@ 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()
+ .map(ReflectionUtils::toDirectory)
+ .filter(Objects::nonNull)
+ .flatMap(directory -> findClasses(directory, 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();
- try {
- directories.add(new File(resource.toURI()));
- } catch (URISyntaxException e) {
- log.error("Unable to get URI for {}", resource, e);
- }
- }
- List<String> classes = new ArrayList<>();
- for (File directory : directories) {
- classes.addAll(findClasses(directory, packageName));
+ }
+
+ /**
+ * Converts a package resource {@link URL} to a {@link File} directory, or
{@code null} if the URI is malformed.
+ *
+ * @param resource the package resource URL
+ * @return the corresponding directory, or {@code null} on a malformed URI
+ */
+ private static File toDirectory(URL resource) {
+ try {
+ return new File(resource.toURI());
+ } catch (URISyntaxException e) {
Review Comment:
`new File(resource.toURI())` can throw `IllegalArgumentException` when the
URI scheme is not `file:` (e.g.
`jar:file:/path/to/bundle.jar!/org/apache/hudi/...` URLs returned by the class
loader when running from a fat JAR). Every bundle `Main` caller of this method
runs from a fat JAR, so `toDirectory` will throw an unchecked
`IllegalArgumentException` for all of them, bypassing the `null`-return/log
path entirely.
Since the PR's stated goal is graceful degradation, the catch should cover
both exceptions:
```java
} catch (URISyntaxException | IllegalArgumentException e) {
log.error("Unable to get URI for {}", resource, e);
return null;
}
```
This was a pre-existing bug in the original code, but it's the right time to
fix it.
##########
hudi-io/src/main/java/org/apache/hudi/common/util/ReflectionUtils.java:
##########
@@ -132,26 +132,30 @@ 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()
+ .map(ReflectionUtils::toDirectory)
+ .filter(Objects::nonNull)
+ .flatMap(directory -> findClasses(directory, packageName).stream());
} catch (IOException e) {
Review Comment:
The new `IOException` recovery path (returning `Stream.empty()`) has no test
coverage. A unit test using a custom `ClassLoader` that throws `IOException`
from `getResources` would confirm the method returns an empty stream rather
than propagating an exception. Given this was the main bug cited in the PR
description, a regression test would be valuable.
--
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]
