From 9c10e8cd3678ca4a05f2d8b41ae2a90262dea056 Mon Sep 17 00:00:00 2001 From: Xueqin Cui Date: Tue, 30 Jul 2024 17:56:45 +1000 Subject: [PATCH 1/2] Handle Maven parent relative path --- .../manifest/fixtures/maven/parent/pom.xml | 6 +- internal/manifest/maven.go | 31 ++++++--- .../manifest/__snapshots__/maven_test.snap | 2 +- .../fixtures/{ => maven/my-app}/pom.xml | 2 +- .../{ => maven}/no-dependency-management.xml | 0 .../maven/parent/grandparent/mismatch.xml | 11 +++ .../{ => maven}/parent/grandparent/pom.xml | 1 + .../fixtures/{ => maven}/parent/pom.xml | 2 +- .../manifest/fixtures/maven/pom.xml | 10 +++ internal/resolution/manifest/maven.go | 48 ++++++++++--- internal/resolution/manifest/maven_test.go | 68 ++++++++++++++++++- 11 files changed, 155 insertions(+), 26 deletions(-) rename internal/resolution/manifest/fixtures/{ => maven/my-app}/pom.xml (98%) rename internal/resolution/manifest/fixtures/{ => maven}/no-dependency-management.xml (100%) create mode 100644 internal/resolution/manifest/fixtures/maven/parent/grandparent/mismatch.xml rename internal/resolution/manifest/fixtures/{ => maven}/parent/grandparent/pom.xml (93%) rename internal/resolution/manifest/fixtures/{ => maven}/parent/pom.xml (95%) create mode 100644 internal/resolution/manifest/fixtures/maven/pom.xml diff --git a/internal/manifest/fixtures/maven/parent/pom.xml b/internal/manifest/fixtures/maven/parent/pom.xml index a673b2faea..3751df6be3 100644 --- a/internal/manifest/fixtures/maven/parent/pom.xml +++ b/internal/manifest/fixtures/maven/parent/pom.xml @@ -1,8 +1,10 @@ - com.mycompany.app - my-app + org.local + parent-pom 1.0 + pom + org.upstream parent-pom diff --git a/internal/manifest/maven.go b/internal/manifest/maven.go index 7f792d2525..0b9f8bbcb9 100644 --- a/internal/manifest/maven.go +++ b/internal/manifest/maven.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" "deps.dev/util/maven" "deps.dev/util/resolve" @@ -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 @@ -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 diff --git a/internal/resolution/manifest/__snapshots__/maven_test.snap b/internal/resolution/manifest/__snapshots__/maven_test.snap index 78ebbca6f1..62e9c38a90 100755 --- a/internal/resolution/manifest/__snapshots__/maven_test.snap +++ b/internal/resolution/manifest/__snapshots__/maven_test.snap @@ -18,7 +18,7 @@ org.parent parent-pom 1.1.1 - ./parent/pom.xml + ../parent/pom.xml diff --git a/internal/resolution/manifest/fixtures/pom.xml b/internal/resolution/manifest/fixtures/maven/my-app/pom.xml similarity index 98% rename from internal/resolution/manifest/fixtures/pom.xml rename to internal/resolution/manifest/fixtures/maven/my-app/pom.xml index e548db2657..ea58b481d5 100644 --- a/internal/resolution/manifest/fixtures/pom.xml +++ b/internal/resolution/manifest/fixtures/maven/my-app/pom.xml @@ -16,7 +16,7 @@ org.parent parent-pom 1.1.1 - ./parent/pom.xml + ../parent/pom.xml diff --git a/internal/resolution/manifest/fixtures/no-dependency-management.xml b/internal/resolution/manifest/fixtures/maven/no-dependency-management.xml similarity index 100% rename from internal/resolution/manifest/fixtures/no-dependency-management.xml rename to internal/resolution/manifest/fixtures/maven/no-dependency-management.xml diff --git a/internal/resolution/manifest/fixtures/maven/parent/grandparent/mismatch.xml b/internal/resolution/manifest/fixtures/maven/parent/grandparent/mismatch.xml new file mode 100644 index 0000000000..6b39ccaad5 --- /dev/null +++ b/internal/resolution/manifest/fixtures/maven/parent/grandparent/mismatch.xml @@ -0,0 +1,11 @@ + + + 4.0.0 + + org.upstream + wrong-parent + 1.1.1 + + pom + + diff --git a/internal/resolution/manifest/fixtures/parent/grandparent/pom.xml b/internal/resolution/manifest/fixtures/maven/parent/grandparent/pom.xml similarity index 93% rename from internal/resolution/manifest/fixtures/parent/grandparent/pom.xml rename to internal/resolution/manifest/fixtures/maven/parent/grandparent/pom.xml index f992aca3aa..e768820f58 100644 --- a/internal/resolution/manifest/fixtures/parent/grandparent/pom.xml +++ b/internal/resolution/manifest/fixtures/maven/parent/grandparent/pom.xml @@ -18,6 +18,7 @@ org.upstream parent-pom 1.2.3 + ./mismatch.xml diff --git a/internal/resolution/manifest/fixtures/parent/pom.xml b/internal/resolution/manifest/fixtures/maven/parent/pom.xml similarity index 95% rename from internal/resolution/manifest/fixtures/parent/pom.xml rename to internal/resolution/manifest/fixtures/maven/parent/pom.xml index 272ded1ffe..067663d23c 100644 --- a/internal/resolution/manifest/fixtures/parent/pom.xml +++ b/internal/resolution/manifest/fixtures/maven/parent/pom.xml @@ -16,7 +16,7 @@ org.grandparent - grandparentparent-pom + grandparent-pom 1.1.1 ./grandparent diff --git a/internal/resolution/manifest/fixtures/maven/pom.xml b/internal/resolution/manifest/fixtures/maven/pom.xml new file mode 100644 index 0000000000..cb678cdf43 --- /dev/null +++ b/internal/resolution/manifest/fixtures/maven/pom.xml @@ -0,0 +1,10 @@ + + + + 4.0.0 + + org.test + test + 1.0.0 + + diff --git a/internal/resolution/manifest/maven.go b/internal/resolution/manifest/maven.go index 3fe468d5b9..78b87b8b95 100644 --- a/internal/resolution/manifest/maven.go +++ b/internal/resolution/manifest/maven.go @@ -10,6 +10,7 @@ import ( "io" "os" "path/filepath" + "reflect" "slices" "strings" @@ -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{}) { // Once we fetch a parent pom.xml from upstream, we should not allow // parsing parent pom.xml locally anymore. allowLocal = false @@ -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 { @@ -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 == "" { + 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 diff --git a/internal/resolution/manifest/maven_test.go b/internal/resolution/manifest/maven_test.go index 338b5b9965..84f0c4fe0a 100644 --- a/internal/resolution/manifest/maven_test.go +++ b/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) + } + } +} From 9a85d488a9b151759c59e02e1d281b222d8dd4e0 Mon Sep 17 00:00:00 2001 From: Xueqin Cui Date: Thu, 1 Aug 2024 10:49:34 +1000 Subject: [PATCH 2/2] review comment --- internal/manifest/maven.go | 13 +++++-------- internal/resolution/manifest/maven.go | 12 +++++------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/internal/manifest/maven.go b/internal/manifest/maven.go index 0b9f8bbcb9..f1a39ce347 100644 --- a/internal/manifest/maven.go +++ b/internal/manifest/maven.go @@ -7,7 +7,6 @@ import ( "fmt" "os" "path/filepath" - "reflect" "deps.dev/util/maven" "deps.dev/util/resolve" @@ -154,6 +153,7 @@ func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven. visited[current.ProjectKey] = true var proj maven.Project + parentFound := false if parentPath := manifest.MavenParentPOMPath(currentPath, string(current.RelativePath)); allowLocal && parentPath != "" { currentPath = parentPath f, err := os.Open(parentPath) @@ -163,15 +163,12 @@ func (e MavenResolverExtractor) mergeParents(ctx context.Context, result *maven. if err := xml.NewDecoder(f).Decode(&proj); err != nil { return fmt.Errorf("failed to unmarshal project: %w", err) } - 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{} + if proj.ProjectKey == current.ProjectKey && proj.Packaging == "pom" { + // Only mark parent is found when the identifiers and packaging are exptected. + parentFound = true } } - - // proj being empty indicates that we are not able to find parent pom.xml locally. - if reflect.DeepEqual(proj, maven.Project{}) { + if !parentFound { // Once we fetch a parent pom.xml from upstream, we should not // allow parsing parent pom.xml locally anymore. allowLocal = false diff --git a/internal/resolution/manifest/maven.go b/internal/resolution/manifest/maven.go index 78b87b8b95..46a7c31676 100644 --- a/internal/resolution/manifest/maven.go +++ b/internal/resolution/manifest/maven.go @@ -10,7 +10,6 @@ import ( "io" "os" "path/filepath" - "reflect" "slices" "strings" @@ -264,6 +263,7 @@ func (m MavenManifestIO) mergeParents(ctx context.Context, result *maven.Project } 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) @@ -273,14 +273,12 @@ func (m MavenManifestIO) mergeParents(ctx context.Context, result *maven.Project if err := xml.NewDecoder(f).Decode(&proj); err != nil { return fmt.Errorf("failed to unmarshal project: %w", err) } - 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{} + if proj.ProjectKey == current.ProjectKey && proj.Packaging == "pom" { + // Only mark parent is found when the identifiers and packaging are exptected. + parentFound = true } } - // proj being empty indicates that we are not able to find parent pom.xml locally. - if reflect.DeepEqual(proj, maven.Project{}) { + if !parentFound { // Once we fetch a parent pom.xml from upstream, we should not allow // parsing parent pom.xml locally anymore. allowLocal = false