github-actions[bot] commented on code in PR #41083:
URL: https://github.com/apache/beam/pull/41083#discussion_r4505764888
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/transforms/providers/StringCompiler.java:
##########
@@ -49,41 +56,209 @@
import javax.tools.SimpleJavaFileObject;
import javax.tools.StandardJavaFileManager;
import javax.tools.ToolProvider;
+import org.apache.beam.sdk.values.Row;
+import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.annotations.VisibleForTesting;
+import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Splitter;
import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Suppliers;
import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableList;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class StringCompiler {
+ private static final Logger LOG =
LoggerFactory.getLogger(StringCompiler.class);
+
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
private static final Supplier<String> classpathSupplier =
Suppliers.memoize(
() -> {
- List<String> cp = new ArrayList<>();
- cp.add(System.getProperty("java.class.path"));
- // Javac doesn't properly handle manifest classpath spec.
ClassLoader cl = StringCompiler.class.getClassLoader();
if (cl == null) {
cl = ClassLoader.getSystemClassLoader();
}
- if (cl instanceof URLClassLoader) {
- for (URL url : ((URLClassLoader) cl).getURLs()) {
- File file = new File(url.getFile());
- if (file.exists() && !file.isDirectory()) {
- try (ZipFile zipFile = new ZipFile(new File(url.getFile())))
{
- ZipEntry manifestEntry =
zipFile.getEntry("META-INF/MANIFEST.MF");
- if (manifestEntry != null) {
- Manifest manifest = new
Manifest(zipFile.getInputStream(manifestEntry));
-
cp.add(manifest.getMainAttributes().getValue(Attributes.Name.CLASS_PATH));
- }
- } catch (IOException exn) {
- throw new RuntimeException(exn);
+ List<String> cp = buildClasspath(cl,
System.getProperty("java.class.path"));
+ return String.join(File.pathSeparator, cp);
+ });
+
+ @VisibleForTesting
+ static List<String> buildClasspath(@Nullable ClassLoader cl, @Nullable
String classPathProperty) {
+ Set<String> cp = new LinkedHashSet<>();
+ Set<File> visitedJars = new HashSet<>();
+
+ if (classPathProperty != null) {
+ for (String entry :
+
Splitter.on(File.pathSeparator).omitEmptyStrings().split(classPathProperty)) {
+ File file = new File(entry);
+ if (file.exists()) {
+ cp.add(file.getAbsolutePath());
+ if (file.isFile() && isJarFile(file)) {
+ extractManifestClasspath(file, cp, visitedJars);
+ }
+ }
+ }
+ }
+
+ if (cl instanceof URLClassLoader) {
+ for (URL url : ((URLClassLoader) cl).getURLs()) {
+ try {
+ File file = new File(url.toURI());
+ if (file.exists()) {
+ cp.add(file.getAbsolutePath());
+ if (file.isFile() && isJarFile(file)) {
+ extractManifestClasspath(file, cp, visitedJars);
+ }
+ }
+ } catch (Exception e) {
+ File file = new File(url.getPath());
+ if (file.exists()) {
+ cp.add(file.getAbsolutePath());
+ }
+ }
+ }
+ }
+
+ addClassLocation(Row.class, cp, visitedJars);
+ addClassLocation(StringCompiler.class, cp, visitedJars);
+
+ if (cl != null) {
+ addResourceLocation(cl, "org/apache/beam/sdk/values/Row.class", cp,
visitedJars);
+ addResourceLocation(
+ cl,
+
"org/apache/beam/sdk/schemas/transforms/providers/StringCompiler.class",
+ cp,
+ visitedJars);
+ }
+
+ return new ArrayList<>(cp);
+ }
+
+ private static void extractManifestClasspath(
+ File jarFile, Set<String> cp, Set<File> visitedJars) {
+ File canonicalJar;
+ try {
+ canonicalJar = jarFile.getCanonicalFile();
+ } catch (IOException e) {
+ canonicalJar = jarFile.getAbsoluteFile();
+ }
+ if (!visitedJars.add(canonicalJar)) {
+ return;
+ }
+
+ try (ZipFile zipFile = new ZipFile(jarFile)) {
+ ZipEntry manifestEntry = zipFile.getEntry("META-INF/MANIFEST.MF");
+ if (manifestEntry != null) {
+ try (InputStream is = zipFile.getInputStream(manifestEntry)) {
+ if (is != null) {
+ Manifest manifest = new Manifest(is);
+ String classPathAttr =
+
manifest.getMainAttributes().getValue(Attributes.Name.CLASS_PATH);
+ if (classPathAttr != null) {
+ for (String part :
+
Splitter.onPattern("\\s+").omitEmptyStrings().split(classPathAttr)) {
+ File targetFile =
resolveManifestEntry(jarFile.getParentFile(), part);
+ if (targetFile != null && targetFile.exists()) {
+ cp.add(targetFile.getAbsolutePath());
+ if (targetFile.isFile() && isJarFile(targetFile)) {
+ extractManifestClasspath(targetFile, cp, visitedJars);
}
}
}
}
- return String.join(System.getProperty("path.separator"), cp);
- });
+ }
+ }
+ }
+ } catch (IOException exn) {
+ LOG.warn("Failed to read manifest from jar: {}", jarFile, exn);
+ }
+ }
+
+ private static @Nullable File resolveManifestEntry(@Nullable File parentDir,
String entry) {
+ if (entry.startsWith("file:")) {
+ try {
+ URI uri = URI.create(entry);
+ File f = new File(uri.getPath());
+ if (f.isAbsolute()) {
+ return f;
+ }
+ return parentDir != null ? new File(parentDir, f.getPath()) : f;
+ } catch (Exception e) {
+ String path = entry.substring("file:".length());
+ File f = new File(path);
+ if (f.isAbsolute()) {
+ return f;
+ }
+ return parentDir != null ? new File(parentDir, path) : f;
+ }
Review Comment:
**🟡 Medium**: Per the JAR File Specification, `Class-Path` manifest entries
are relative URLs (not raw file paths), so characters like spaces are
percent-encoded (e.g., `%20`). In the `else` branch, passing `entry` directly
to `new File(...)` without decoding or resolving via `URI` will fail to locate
files whose paths contain percent-encoded characters (`file.exists()` will
return `false`). Consider resolving relative entries via
`URI.create(entry).getPath()` (with fallback to `entry` on
`IllegalArgumentException`) so percent-encoded characters are decoded
consistently.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/transforms/providers/StringCompiler.java:
##########
@@ -49,41 +56,209 @@
import javax.tools.SimpleJavaFileObject;
import javax.tools.StandardJavaFileManager;
import javax.tools.ToolProvider;
+import org.apache.beam.sdk.values.Row;
+import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.annotations.VisibleForTesting;
+import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Splitter;
import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Suppliers;
import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableList;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class StringCompiler {
+ private static final Logger LOG =
LoggerFactory.getLogger(StringCompiler.class);
+
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
private static final Supplier<String> classpathSupplier =
Suppliers.memoize(
() -> {
- List<String> cp = new ArrayList<>();
- cp.add(System.getProperty("java.class.path"));
- // Javac doesn't properly handle manifest classpath spec.
ClassLoader cl = StringCompiler.class.getClassLoader();
if (cl == null) {
cl = ClassLoader.getSystemClassLoader();
}
- if (cl instanceof URLClassLoader) {
- for (URL url : ((URLClassLoader) cl).getURLs()) {
- File file = new File(url.getFile());
- if (file.exists() && !file.isDirectory()) {
- try (ZipFile zipFile = new ZipFile(new File(url.getFile())))
{
- ZipEntry manifestEntry =
zipFile.getEntry("META-INF/MANIFEST.MF");
- if (manifestEntry != null) {
- Manifest manifest = new
Manifest(zipFile.getInputStream(manifestEntry));
-
cp.add(manifest.getMainAttributes().getValue(Attributes.Name.CLASS_PATH));
- }
- } catch (IOException exn) {
- throw new RuntimeException(exn);
+ List<String> cp = buildClasspath(cl,
System.getProperty("java.class.path"));
+ return String.join(File.pathSeparator, cp);
+ });
+
+ @VisibleForTesting
+ static List<String> buildClasspath(@Nullable ClassLoader cl, @Nullable
String classPathProperty) {
+ Set<String> cp = new LinkedHashSet<>();
+ Set<File> visitedJars = new HashSet<>();
+
+ if (classPathProperty != null) {
+ for (String entry :
+
Splitter.on(File.pathSeparator).omitEmptyStrings().split(classPathProperty)) {
+ File file = new File(entry);
+ if (file.exists()) {
+ cp.add(file.getAbsolutePath());
+ if (file.isFile() && isJarFile(file)) {
+ extractManifestClasspath(file, cp, visitedJars);
+ }
+ }
+ }
+ }
+
+ if (cl instanceof URLClassLoader) {
+ for (URL url : ((URLClassLoader) cl).getURLs()) {
+ try {
+ File file = new File(url.toURI());
+ if (file.exists()) {
+ cp.add(file.getAbsolutePath());
+ if (file.isFile() && isJarFile(file)) {
+ extractManifestClasspath(file, cp, visitedJars);
+ }
+ }
+ } catch (Exception e) {
+ File file = new File(url.getPath());
+ if (file.exists()) {
+ cp.add(file.getAbsolutePath());
+ }
Review Comment:
**🟡 Medium**: In the fallback catch block for `URLClassLoader` URLs,
`extractManifestClasspath` is not called if `file` is a JAR file (unlike the
`try` block above on lines 108-110). If `url.toURI()` throws an exception
(e.g., due to unescaped characters in the URL) and falls back to
`url.getPath()`, any manifest `Class-Path` entries in that JAR will be skipped.
--
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]
