Handle Maven parent relative path (#1149)

When Maven looks for the parent POM, it first looks up the specified
relative path, then look for the default relative path which is
`../pom.xml`, and lastly in the remote repository. If only a directory
is specified in relative path, `pom.xml` will be looked automatically.

Reference:
https://maven.apache.org/ref/3.9.8/maven-model/maven.html#parent

Currently, OSV-Scanner only do some steps above, this PR corrects this.

Also, considering both `internal/manifest` and
`internal/resolution/manifest` require basically the same logic for
merging parent POM, I would like to refactor this in a following PR.

internal/manifest/fixtures/maven/parent/pom.xml

@@ -1,8 +1,10 @@
 <project>
-  <groupId>com.mycompany.app</groupId>
-  <artifactId>my-app</artifactId>
+  <groupId>org.local</groupId>
+  <artifactId>parent-pom</artifactId>
   <version>1.0</version>
 
+  <packaging>pom</packaging>
+
   <parent>
     <groupId>org.upstream</groupId>
     <artifactId>parent-pom</artifactId>

internal/manifest/maven.go

@@ -153,20 +153,22 @@ func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven.
 		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)
+		parentFound := false
+		if parentPath := manifest.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", current.RelativePath, err)
+				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)
 			}
-		} else {
+			if proj.ProjectKey == 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
@@ -180,6 +182,14 @@ func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven.
 				// A parent project should only be of "pom" packaging type.
 				return fmt.Errorf("invalid packaging for parent project %s", proj.Packaging)
 			}
+			if proj.ProjectKey != 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 := result.MergeProfiles("", maven.ActivationOS{}); err != nil {
+			return err
 		}
 		result.MergeParent(proj)
 		current = proj.Parent

internal/resolution/manifest/__snapshots__/maven_test.snap

@@ -18,7 +18,7 @@
     <groupId>org.parent</groupId>
     <artifactId>parent-pom</artifactId>
     <version>1.1.1</version>
-    <relativePath>./parent/pom.xml</relativePath>
+    <relativePath>../parent/pom.xml</relativePath>
   </parent>
 
   <properties>

internal/resolution/manifest/fixtures/pom.xml → internal/resolution/manifest/fixtures/maven/my-app/pom.xml

@@ -16,7 +16,7 @@
     <groupId>org.parent</groupId>
     <artifactId>parent-pom</artifactId>
     <version>1.1.1</version>
-    <relativePath>./parent/pom.xml</relativePath>
+    <relativePath>../parent/pom.xml</relativePath>
   </parent>
 
   <properties>

internal/resolution/manifest/fixtures/maven/parent/grandparent/mismatch.xml

@@ -0,0 +1,11 @@
+<project>
+
+  <modelVersion>4.0.0</modelVersion>
+
+  <groupId>org.upstream</groupId>
+  <artifactId>wrong-parent</artifactId>
+  <version>1.1.1</version>
+
+  <packaging>pom</packaging>
+
+</project>

internal/resolution/manifest/fixtures/parent/grandparent/pom.xml → internal/resolution/manifest/fixtures/maven/parent/grandparent/pom.xml

@@ -18,6 +18,7 @@
     <groupId>org.upstream</groupId>
     <artifactId>parent-pom</artifactId>
     <version>1.2.3</version>
+    <relativePath>./mismatch.xml</relativePath>
   </parent>
 
 </project>

internal/resolution/manifest/fixtures/parent/pom.xml → internal/resolution/manifest/fixtures/maven/parent/pom.xml

@@ -16,7 +16,7 @@
 
   <parent>
     <groupId>org.grandparent</groupId>
-    <artifactId>grandparentparent-pom</artifactId>
+    <artifactId>grandparent-pom</artifactId>
     <version>1.1.1</version>
     <relativePath>./grandparent</relativePath>
   </parent>

internal/resolution/manifest/fixtures/maven/pom.xml

@@ -0,0 +1,10 @@
+<!-- For testing parent relative path -->
+<project>
+
+  <modelVersion>4.0.0</modelVersion>
+
+  <groupId>org.test</groupId>
+  <artifactId>test</artifactId>
+  <version>1.0.0</version>
+
+</project>

internal/resolution/manifest/maven.go

@@ -263,20 +263,22 @@ func (m MavenManifestIO) mergeParents(ctx context.Context, result *maven.Project
 		}
 		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)
+		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", current.RelativePath, err)
+				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)
 			}
-		} else {
+			if proj.ProjectKey == 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
@@ -289,6 +291,10 @@ func (m MavenManifestIO) mergeParents(ctx context.Context, result *maven.Project
 				// A parent project should only be of "pom" packaging type.
 				return fmt.Errorf("invalid packaging for parent project %s", proj.Packaging)
 			}
+			if proj.ProjectKey != 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 := result.MergeProfiles("", maven.ActivationOS{}); err != nil {
@@ -301,6 +307,28 @@ func (m MavenManifestIO) mergeParents(ctx context.Context, result *maven.Project
 	return result.Interpolate()
 }
 
+// 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 ""
+}
+
 // For dependencies in profiles and plugins, we use origin to indicate where they are from.
 // The origin is in the format prefix@identifier[@postfix] (where @ is the separator):
 //   - prefix indicates it is from profile or plugin

internal/resolution/manifest/maven_test.go

@@ -103,7 +103,7 @@ func TestMavenRead(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to get current directory: %v", err)
 	}
-	df, err := lockfile.OpenLocalDepFile(filepath.Join(dir, "fixtures", "pom.xml"))
+	df, err := lockfile.OpenLocalDepFile(filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"))
 	if err != nil {
 		t.Fatalf("failed to open file: %v", err)
 	}
@@ -382,7 +382,7 @@ func TestMavenWrite(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to get current directory: %v", err)
 	}
-	df, err := lockfile.OpenLocalDepFile(filepath.Join(dir, "fixtures", "pom.xml"))
+	df, err := lockfile.OpenLocalDepFile(filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"))
 	if err != nil {
 		t.Fatalf("fail to open file: %v", err)
 	}
@@ -492,7 +492,7 @@ func TestMavenWriteDM(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to get current directory: %v", err)
 	}
-	df, err := lockfile.OpenLocalDepFile(filepath.Join(dir, "fixtures", "no-dependency-management.xml"))
+	df, err := lockfile.OpenLocalDepFile(filepath.Join(dir, "fixtures", "maven", "no-dependency-management.xml"))
 	if err != nil {
 		t.Fatalf("fail to open file: %v", err)
 	}
@@ -868,3 +868,65 @@ func TestGeneratePropertyPatches(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 := 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)
+		}
+	}
+}