Merge parent projects for Maven pom.xml (#1019)

#531 This PR merges parent pom.xml by paring locally or fetching from upstream (Maven Central for now). When merging a parent pom.xml, only `pom` packaging is allowed. Once we fetch a parent from upstream, parsing from local is no longer allowed. The project is also interpolated to get rid of properties, and dependencies are also processed (dedup and import).
google · Jun 7, 2024 · 0993dba · 0993dba
1 parent 7f7f468
commit 0993dba
Show file tree

Hide file tree

Showing 5 changed files with 263 additions and 14 deletions.
diff --git a/internal/manifest/fixtures/maven/parent/pom.xml b/internal/manifest/fixtures/maven/parent/pom.xml
@@ -0,0 +1,19 @@
+<project>
+  <groupId>com.mycompany.app</groupId>
+  <artifactId>my-app</artifactId>
+  <version>1.0</version>
+
+  <parent>
+    <groupId>org.upstream</groupId>
+    <artifactId>parent-pom</artifactId>
+    <version>1.0</version>
+  </parent>
+
+  <dependencies>
+    <dependency>
+      <groupId>org.dave</groupId>
+      <artifactId>dave</artifactId>
+      <version>4.0.0</version>
+    </dependency>
+  </dependencies>
+</project>
diff --git a/internal/manifest/fixtures/maven/with-parent.xml b/internal/manifest/fixtures/maven/with-parent.xml
@@ -0,0 +1,54 @@
+<project>
+  <groupId>com.mycompany.app</groupId>
+  <artifactId>my-app</artifactId>
+  <version>1.0</version>
+
+  <parent>
+    <groupId>org.local</groupId>
+    <artifactId>parent-pom</artifactId>
+    <version>1.0</version>
+    <relativePath>./parent/pom.xml</relativePath>
+  </parent>
+
+  <properties>
+    <bob.version>2.0.0</bob.version>
+  </properties>
+
+  <dependencies>
+    <dependency>
+      <groupId>org.alice</groupId>
+      <artifactId>alice</artifactId>
+      <version>1.0.0</version>
+    </dependency>
+    <dependency>
+      <groupId>org.bob</groupId>
+      <artifactId>bob</artifactId>
+      <version>${bob.version}</version>
+    </dependency>
+    <dependency>
+      <groupId>org.chuck</groupId>
+      <artifactId>chuck</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.frank</groupId>
+      <artifactId>frank</artifactId>
+    </dependency>
+  </dependencies>
+
+  <dependencyManagement>
+    <dependencies>
+      <dependency>
+        <groupId>org.chuck</groupId>
+        <artifactId>chuck</artifactId>
+        <version>3.0.0</version>
+      </dependency>
+      <dependency>
+        <groupId>org.import</groupId>
+        <artifactId>import</artifactId>
+        <version>1.2.3</version>
+        <type>pom</type>
+        <scope>import</scope>
+      </dependency>
+    </dependencies>
+  </dependencyManagement>
+</project>
diff --git a/internal/manifest/fixtures/universe/basic-universe.yaml b/internal/manifest/fixtures/universe/basic-universe.yaml
@@ -7,15 +7,27 @@ schema: |
     4.1.42.Final
   junit:junit
     4.12
+  org.alice:alice
+    1.0.0
   org.apache.maven:maven-artifact
     1.0.0
+  org.bob:bob
+    2.0.0
+  org.chuck:chuck
+    3.0.0
+  org.dave:dave
+    4.0.0
   org.direct:alice
     1.0.0
       org.transitive:chuck@1.1.1
       org.transitive:dave@2.2.2
   org.direct:bob
     2.0.0
       org.transitive:eve@3.3.3
+  org.eve:eve
+    5.0.0
+  org.frank:frank
+    6.0.0
   org.mine:my.package
     2.3.4
   org.mine:mypackage

diff --git a/internal/manifest/maven.go b/internal/manifest/maven.go
@@ -3,21 +3,25 @@ package manifest
 import (
 	"context"
 	"encoding/xml"
+	"errors"
 	"fmt"
+	"os"
 	"path/filepath"
 
 	"deps.dev/util/maven"
 	"deps.dev/util/resolve"
 	"deps.dev/util/resolve/dep"
 	mavenresolve "deps.dev/util/resolve/maven"
 	"github.com/google/osv-scanner/internal/resolution/client"
+	"github.com/google/osv-scanner/internal/resolution/datasource"
 	"github.com/google/osv-scanner/internal/resolution/util"
 	"github.com/google/osv-scanner/pkg/lockfile"
 	"golang.org/x/exp/maps"
 )
 
 type MavenResolverExtractor struct {
 	client.DependencyClient
+	datasource.MavenRegistryAPIClient
 }
 
 func (e MavenResolverExtractor) ShouldExtract(path string) bool {
@@ -31,9 +35,23 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 	if err := xml.NewDecoder(f).Decode(&project); err != nil {
 		return []lockfile.PackageDetails{}, fmt.Errorf("could not extract from %s: %w", f.Path(), err)
 	}
-	if err := project.Interpolate(); err != nil {
-		return []lockfile.PackageDetails{}, fmt.Errorf("could not interpolate Maven project %s: %w", project.ProjectKey.Name(), err)
+	// Merging parents data by parsing local parent pom.xml or fetching from upstream.
+	if err := e.mergeParents(ctx, &project, project.Parent, 1, true, f.Path()); err != nil {
+		return []lockfile.PackageDetails{}, fmt.Errorf("failed to merge parents: %w", err)
 	}
+	// Process the dependencies:
+	//  - dedupe dependencies and dependency management
+	//  - import dependency management
+	//  - fill in missing dependency version requirement
+	project.ProcessDependencies(func(groupID, artifactID, version maven.String) (maven.DependencyManagement, error) {
+		root := maven.Parent{ProjectKey: maven.ProjectKey{GroupID: groupID, ArtifactID: artifactID, Version: version}}
+		var result maven.Project
+		if err := e.mergeParents(ctx, &result, root, 0, false, f.Path()); err != nil {
+			return maven.DependencyManagement{}, err
+		}
+
+		return result.DependencyManagement, nil
+	})
 
 	overrideClient := client.NewOverrideClient(e.DependencyClient)
 	resolver := mavenresolve.NewResolver(overrideClient)
@@ -95,12 +113,73 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 	return maps.Values(details), nil
 }
 
-func ParseMavenWithResolver(depClient client.DependencyClient, pathToLockfile string) ([]lockfile.PackageDetails, error) {
+// MaxParent sets a limit on the number of parents to avoid indefinite loop.
+const MaxParent = 100
+
+// mergeParents parses local accessible parent pom.xml or fetches it from
+// upstream, merges into root project, then interpolate the properties.
+// result holds the merged Maven project.
+// current holds the current parent project to merge.
+// start indicates the index of the current parent project, which is used to
+// check if the packaging has to be `pom`.
+// allowLocal indicates whether parsing local parent pom.xml is allowed.
+// path holds the path to the current pom.xml, which is used to compute the
+// relative path of parent.
+func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven.Project, current maven.Parent, start int, allowLocal bool, path string) error {
+	currentPath := path
+	visited := make(map[maven.ProjectKey]bool, MaxParent)
+	for n := start; n < MaxParent; n++ {
+		if current.GroupID == "" || current.ArtifactID == "" || current.Version == "" {
+			break
+		}
+		if visited[current.ProjectKey] {
+			// A cycle of parents is detected
+			return errors.New("a cycle of parents is detected")
+		}
+		visited[current.ProjectKey] = true
+
+		var proj maven.Project
+		if allowLocal && current.RelativePath != "" {
+			currentPath = filepath.Join(filepath.Dir(currentPath), string(current.RelativePath))
+			if filepath.Base(currentPath) != "pom.xml" {
+				// If the base is not pom.xml, this path is a directory but not a file.
+				currentPath = filepath.Join(currentPath, "pom.xml")
+			}
+			f, err := os.Open(currentPath)
+			if err != nil {
+				return fmt.Errorf("failed to open parent file %s: %w", current.RelativePath, err)
+			}
+			if err := xml.NewDecoder(f).Decode(&proj); err != nil {
+				return fmt.Errorf("failed to unmarshal project: %w", err)
+			}
+		} else {
+			// Once we fetch a parent pom.xml from upstream, we should not
+			// allow parsing parent pom.xml locally anymore.
+			allowLocal = false
+
+			var err error
+			proj, err = e.MavenRegistryAPIClient.GetProject(ctx, string(current.GroupID), string(current.ArtifactID), string(current.Version))
+			if err != nil {
+				return fmt.Errorf("failed to get Maven project %s:%s:%s: %w", current.GroupID, current.ArtifactID, current.Version, err)
+			}
+			if n > 0 && proj.Packaging != "pom" {
+				// A parent project should only be of "pom" packaging type.
+				return fmt.Errorf("invalid packaging for parent project %s", proj.Packaging)
+			}
+		}
+		result.MergeParent(proj)
+		current = proj.Parent
+	}
+	// Interpolate the project to resolve the properties.
+	return result.Interpolate()
+}
+
+func ParseMavenWithResolver(depClient client.DependencyClient, mavenClient datasource.MavenRegistryAPIClient, pathToLockfile string) ([]lockfile.PackageDetails, error) {
 	f, err := lockfile.OpenLocalDepFile(pathToLockfile)
 	if err != nil {
 		return []lockfile.PackageDetails{}, err
 	}
 	defer f.Close()
 
-	return MavenResolverExtractor{DependencyClient: depClient}.Extract(f)
+	return MavenResolverExtractor{DependencyClient: depClient, MavenRegistryAPIClient: mavenClient}.Extract(f)
 }