Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Maven parent relative path #1149

Merged
merged 4 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions internal/manifest/fixtures/maven/parent/pom.xml
Original file line number Diff line number Diff line change
@@ -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>
Expand Down
31 changes: 22 additions & 9 deletions internal/manifest/maven.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"os"
"path/filepath"
"reflect"

"deps.dev/util/maven"
"deps.dev/util/resolve"
Expand Down Expand Up @@ -153,20 +154,24 @@ 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)
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" {
// The identifiers or packaging in parent does not match what we want,
// mark proj as empty so parent can be fetched from upstream.
proj = maven.Project{}
}
}

// proj being empty indicates that we are not able to find parent pom.xml locally.
if reflect.DeepEqual(proj, maven.Project{}) {
// Once we fetch a parent pom.xml from upstream, we should not
// allow parsing parent pom.xml locally anymore.
allowLocal = false
Expand All @@ -180,6 +185,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
Expand Down
2 changes: 1 addition & 1 deletion internal/resolution/manifest/__snapshots__/maven_test.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
<groupId>org.upstream</groupId>
<artifactId>parent-pom</artifactId>
<version>1.2.3</version>
<relativePath>./mismatch.xml</relativePath>
</parent>

</project>
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
10 changes: 10 additions & 0 deletions internal/resolution/manifest/fixtures/maven/pom.xml
Original file line number Diff line number Diff line change
@@ -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>
48 changes: 39 additions & 9 deletions internal/resolution/manifest/maven.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io"
"os"
"path/filepath"
"reflect"
"slices"
"strings"

Expand Down Expand Up @@ -263,20 +264,23 @@ 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)
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" {
// The identifiers or packaging in parent does not match what we want,
// mark proj as empty so parent can be fetched from upstream.
proj = maven.Project{}
}
}
// proj being empty indicates that we are not able to find parent pom.xml locally.
if reflect.DeepEqual(proj, maven.Project{}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Not sure if I like this reflect.DeepEqual() here.
Could maybe instead use a pointer and check proj == nil or add a parentFound bool and check that?

Though, I'm fine to keep it as-is if you would prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I considered parentFound so just changed the PR to use it

// Once we fetch a parent pom.xml from upstream, we should not allow
// parsing parent pom.xml locally anymore.
allowLocal = false
Expand All @@ -289,6 +293,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 {
Expand All @@ -301,6 +309,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 == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is some caveat here: we are not able to tell whether the relative path is not set (should default to ../pom.xml) or set to an empty string (parent should be fetched from upstream).
but we still check if the parent pom.xml exists and its key matches what we want, so I think this might be fine...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there's a change that can be made in the deps.dev repo to set a default value of relativePath to ../pom.xml if it's unspecified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be investigated - we need to make sure we are able to differentiate the two situations.

relativePath = "../pom.xml"
}
path := filepath.Join(filepath.Dir(currentPath), relativePath)
if info, err := os.Stat(path); err == nil {
if !info.IsDir() {
return path
}
Comment on lines +319 to +321
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering what maven does if the relativePath points to a file that is not named pom.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is acceptable if the file exists and is a valid parent pom.xml, otherwise, maven tries to download from upstream.

// 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
Expand Down
68 changes: 65 additions & 3 deletions internal/resolution/manifest/maven_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
}