refactor: move Maven utility to a separate package (#1193)

#1045 Considering that we want to support native Maven registry, we need `MergeMavenParents` from `internal/manifest` in `internal/resolution/client`, however `internal/manifest` imports `internal/resolution/client` for `DependencyClient` for dependency resolution, and this causes an import cycle. This PR moves the Maven utility in `internal/manifest` to a separate package `internal/utility/maven`.
google · Aug 22, 2024 · 35b36a3 · 35b36a3
1 parent 8bbcb62
commit 35b36a3
Show file tree

Hide file tree

Showing 10 changed files with 244 additions and 216 deletions.
diff --git a/cmd/osv-scanner/__snapshots__/main_test.snap b/cmd/osv-scanner/__snapshots__/main_test.snap
@@ -463,6 +463,7 @@ Scanned <rootdir>/fixtures/sbom-insecure/postgres-stretch.cdx.xml as CycloneDX S
 | https://osv.dev/GHSA-v95c-p5hm-xq8f | 6.0  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
 | https://osv.dev/GO-2022-0274        |      |           |                                |                                    |                                                 |
 | https://osv.dev/GHSA-f3fp-gc8g-vw66 | 5.9  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
+| https://osv.dev/GO-2022-0452        |      |           |                                |                                    |                                                 |
 | https://osv.dev/GHSA-g2j6-57v7-gm8c | 6.1  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |
 | https://osv.dev/GO-2023-1683        |      |           |                                |                                    |                                                 |
 | https://osv.dev/GHSA-m8cg-xc2p-r3fc | 2.5  | Go        | github.com/opencontainers/runc | v1.0.1                             | fixtures/sbom-insecure/postgres-stretch.cdx.xml |

diff --git a/internal/manifest/maven.go b/internal/manifest/maven.go
@@ -3,9 +3,7 @@ package manifest
 import (
 	"context"
 	"encoding/xml"
-	"errors"
 	"fmt"
-	"os"
 	"path/filepath"
 
 	"deps.dev/util/maven"
@@ -15,17 +13,11 @@ import (
 	"github.com/google/osv-scanner/internal/resolution/client"
 	"github.com/google/osv-scanner/internal/resolution/datasource"
 	"github.com/google/osv-scanner/internal/resolution/util"
+	mavenutil "github.com/google/osv-scanner/internal/utility/maven"
 	"github.com/google/osv-scanner/pkg/lockfile"
 	"golang.org/x/exp/maps"
 )
 
-const (
-	OriginManagement = "management"
-	OriginParent     = "parent"
-	OriginPlugin     = "plugin"
-	OriginProfile    = "profile"
-)
-
 type MavenResolverExtractor struct {
 	client.DependencyClient
 	datasource.MavenRegistryAPIClient
@@ -43,7 +35,7 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 		return []lockfile.PackageDetails{}, fmt.Errorf("could not extract from %s: %w", f.Path(), err)
 	}
 	// Merging parents data by parsing local parent pom.xml or fetching from upstream.
-	if err := MergeMavenParents(ctx, e.MavenRegistryAPIClient, &project, project.Parent, 1, f.Path(), true); err != nil {
+	if err := mavenutil.MergeParents(ctx, e.MavenRegistryAPIClient, &project, project.Parent, 1, f.Path(), true); err != nil {
 		return []lockfile.PackageDetails{}, fmt.Errorf("failed to merge parents: %w", err)
 	}
 	// Process the dependencies:
@@ -53,7 +45,7 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 	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 := MergeMavenParents(ctx, e.MavenRegistryAPIClient, &result, root, 0, f.Path(), false); err != nil {
+		if err := mavenutil.MergeParents(ctx, e.MavenRegistryAPIClient, &result, root, 0, f.Path(), false); err != nil {
 			return maven.DependencyManagement{}, err
 		}
 
@@ -97,7 +89,7 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 				VersionType: resolve.Requirement,
 				Version:     string(d.Version),
 			},
-			Type: resolve.MavenDepType(d, OriginManagement),
+			Type: resolve.MavenDepType(d, mavenutil.OriginManagement),
 		}
 	}
 	overrideClient.AddVersion(root, reqs)
@@ -133,112 +125,6 @@ func (e MavenResolverExtractor) Extract(f lockfile.DepFile) ([]lockfile.PackageD
 	return maps.Values(details), nil
 }
 
-// MaxParent sets a limit on the number of parents to avoid indefinite loop.
-const MaxParent = 100
-
-// MergeMavenParents 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 MergeMavenParents(ctx context.Context, mavenClient datasource.MavenRegistryAPIClient, result *maven.Project, current maven.Parent, start int, path string, allowLocal bool) 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
-		parentFound := false
-		if parentPath := MavenParentPOMPath(currentPath, string(current.RelativePath)); allowLocal && parentPath != "" {
-			currentPath = parentPath
-			f, err := os.Open(parentPath)
-			if err != nil {
-				return fmt.Errorf("failed to open parent file %s: %w", parentPath, err)
-			}
-			if err := xml.NewDecoder(f).Decode(&proj); err != nil {
-				return fmt.Errorf("failed to unmarshal project: %w", err)
-			}
-			if MavenProjectKey(proj) == current.ProjectKey && proj.Packaging == "pom" {
-				// Only mark parent is found when the identifiers and packaging are exptected.
-				parentFound = true
-			}
-		}
-		if !parentFound {
-			// 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 = mavenClient.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)
-			}
-			if MavenProjectKey(proj) != current.ProjectKey {
-				// The identifiers in parent does not match what we want.
-				return fmt.Errorf("parent identifiers mismatch: %v, expect %v", proj.ProjectKey, current.ProjectKey)
-			}
-		}
-		// Empty JDK and ActivationOS indicates merging the default profiles.
-		if err := proj.MergeProfiles("", maven.ActivationOS{}); err != nil {
-			return err
-		}
-		result.MergeParent(proj)
-		current = proj.Parent
-	}
-	// Interpolate the project to resolve the properties.
-	return result.Interpolate()
-}
-
-// MavenProjectKey returns a project key with empty groupId/version
-// filled by corresponding fields in parent.
-func MavenProjectKey(proj maven.Project) maven.ProjectKey {
-	if proj.GroupID == "" {
-		proj.GroupID = proj.Parent.GroupID
-	}
-	if proj.Version == "" {
-		proj.Version = proj.Parent.Version
-	}
-
-	return proj.ProjectKey
-}
-
-// Maven looks for the parent POM first in 'relativePath',
-// then the local repository '../pom.xml',
-// and lastly in the remote repo.
-func MavenParentPOMPath(currentPath, relativePath string) string {
-	if relativePath == "" {
-		relativePath = "../pom.xml"
-	}
-	path := filepath.Join(filepath.Dir(currentPath), relativePath)
-	if info, err := os.Stat(path); err == nil {
-		if !info.IsDir() {
-			return path
-		}
-		// Current path is a directory, so look for pom.xml in the directory.
-		path = filepath.Join(path, "pom.xml")
-		if _, err := os.Stat(path); err == nil {
-			return path
-		}
-	}
-
-	return ""
-}
-
 func ParseMavenWithResolver(depClient client.DependencyClient, mavenClient datasource.MavenRegistryAPIClient, pathToLockfile string) ([]lockfile.PackageDetails, error) {
 	f, err := lockfile.OpenLocalDepFile(pathToLockfile)
 	if err != nil {

diff --git a/internal/manifest/maven_test.go b/internal/manifest/maven_test.go
@@ -2,8 +2,6 @@ package manifest_test
 
 import (
 	"io/fs"
-	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/google/osv-scanner/internal/manifest"
@@ -360,65 +358,3 @@ func TestParseMavenWithResolver_Transitive(t *testing.T) {
 		},
 	})
 }
-
-func TestParentPOMPath(t *testing.T) {
-	t.Parallel()
-	dir, err := os.Getwd()
-	if err != nil {
-		t.Fatalf("failed to get current directory: %v", err)
-	}
-	tests := []struct {
-		currentPath, relativePath string
-		want                      string
-	}{
-		// fixtures
-		// |- maven
-		// |  |- my-app
-		// |  |  |- pom.xml
-		// |  |- parent
-		// |  |  |- pom.xml
-		// |- pom.xml
-		{
-			// Parent path is specified correctly.
-			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
-			relativePath: "../parent/pom.xml",
-			want:         filepath.Join(dir, "fixtures", "maven", "parent", "pom.xml"),
-		},
-		{
-			// Wrong file name is specified in relative path.
-			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
-			relativePath: "../parent/abc.xml",
-			want:         "",
-		},
-		{
-			// Wrong directory is specified in relative path.
-			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
-			relativePath: "../not-found/pom.xml",
-			want:         "",
-		},
-		{
-			// Only directory is specified.
-			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
-			relativePath: "../parent",
-			want:         filepath.Join(dir, "fixtures", "maven", "parent", "pom.xml"),
-		},
-		{
-			// Parent relative path is default to '../pom.xml'.
-			currentPath:  filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"),
-			relativePath: "",
-			want:         filepath.Join(dir, "fixtures", "maven", "pom.xml"),
-		},
-		{
-			// No pom.xml is found even in the default path.
-			currentPath:  filepath.Join(dir, "fixtures", "maven", "pom.xml"),
-			relativePath: "",
-			want:         "",
-		},
-	}
-	for _, test := range tests {
-		got := manifest.MavenParentPOMPath(test.currentPath, test.relativePath)
-		if got != test.want {
-			t.Errorf("parentPOMPath(%s, %s): got %s, want %s", test.currentPath, test.relativePath, got, test.want)
-		}
-	}
-}
diff --git a/internal/remediation/override.go b/internal/remediation/override.go
@@ -8,12 +8,12 @@ import (
 
 	"deps.dev/util/resolve"
 	"deps.dev/util/resolve/dep"
-	"github.com/google/osv-scanner/internal/manifest"
 	"github.com/google/osv-scanner/internal/remediation/upgrade"
 	"github.com/google/osv-scanner/internal/resolution"
 	"github.com/google/osv-scanner/internal/resolution/client"
-	resolutionmanifest "github.com/google/osv-scanner/internal/resolution/manifest"
+	"github.com/google/osv-scanner/internal/resolution/manifest"
 	"github.com/google/osv-scanner/internal/resolution/util"
+	"github.com/google/osv-scanner/internal/utility/maven"
 	"github.com/google/osv-scanner/internal/utility/vulns"
 )
 
@@ -79,9 +79,9 @@ func ComputeOverridePatches(ctx context.Context, cl client.ResolutionClient, res
 		// CalculateDiff does not compute override manifest patches correctly, manually fill it out.
 		// TODO: CalculateDiff maybe should not be reconstructing patches.
 		// Refactor CalculateDiff, Relaxer, Override to make patches in a more sane way.
-		diff.Deps = make([]resolutionmanifest.DependencyPatch, len(res.patches))
+		diff.Deps = make([]manifest.DependencyPatch, len(res.patches))
 		for i, p := range res.patches {
-			diff.Deps[i] = resolutionmanifest.DependencyPatch{
+			diff.Deps[i] = manifest.DependencyPatch{
 				Pkg:          p.PackageKey,
 				Type:         dep.Type{},
 				OrigRequire:  "", // Using empty original to signal this is an override patch
@@ -280,9 +280,9 @@ func getVersionsGreater(ctx context.Context, cl client.DependencyClient, vk reso
 }
 
 // patchManifest applies the overridePatches to the manifest in-memory. Returns a copy of the manifest that has been patched.
-func patchManifest(patches []overridePatch, m resolutionmanifest.Manifest) (resolutionmanifest.Manifest, error) {
+func patchManifest(patches []overridePatch, m manifest.Manifest) (manifest.Manifest, error) {
 	if m.System() != resolve.Maven {
-		return resolutionmanifest.Manifest{}, errors.New("unsupported ecosystem")
+		return manifest.Manifest{}, errors.New("unsupported ecosystem")
 	}
 
 	// TODO: The overridePatch does not have an artifact's type or classifier, which is part of what uniquely identifies them.
@@ -301,7 +301,7 @@ func patchManifest(patches []overridePatch, m resolutionmanifest.Manifest) (reso
 				continue
 			}
 			origin, hasOrigin := r.Type.GetAttr(dep.MavenDependencyOrigin)
-			if !hasOrigin || origin == manifest.OriginManagement {
+			if !hasOrigin || origin == maven.OriginManagement {
 				found = true
 				r.Version = p.NewVersion
 				patched.Requirements[i] = r
@@ -317,7 +317,7 @@ func patchManifest(patches []overridePatch, m resolutionmanifest.Manifest) (reso
 					VersionType: resolve.Requirement,
 				},
 			}
-			newReq.Type.AddAttr(dep.MavenDependencyOrigin, manifest.OriginManagement)
+			newReq.Type.AddAttr(dep.MavenDependencyOrigin, maven.OriginManagement)
 			patched.Requirements = append(patched.Requirements, newReq)
 		}
 	}