Handle Maven parent relative path #1149
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1149 +/- ##
==========================================
+ Coverage 65.69% 65.70% +0.01%
==========================================
Files 164 164
Lines 13655 13682 +27
==========================================
+ Hits 8970 8990 +20
- Misses 4201 4206 +5
- Partials 484 486 +2 ☔ View full report in Codecov by Sentry. |
| // then the local repository '../pom.xml', | ||
| // and lastly in the remote repo. | ||
| func MavenParentPOMPath(currentPath, relativePath string) string { | ||
| if relativePath == "" { |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
this can be investigated - we need to make sure we are able to differentiate the two situations.
| // then the local repository '../pom.xml', | ||
| // and lastly in the remote repo. | ||
| func MavenParentPOMPath(currentPath, relativePath string) string { | ||
| if relativePath == "" { |
There was a problem hiding this comment.
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?
| proj = maven.Project{} | ||
| } | ||
| } | ||
| // proj being empty indicates that we are not able to find parent pom.xml locally. | ||
| if reflect.DeepEqual(proj, maven.Project{}) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
actually I considered parentFound so just changed the PR to use it
| if !info.IsDir() { | ||
| return path | ||
| } |
There was a problem hiding this comment.
Wondering what maven does if the relativePath points to a file that is not named pom.xml
There was a problem hiding this comment.
this is acceptable if the file exists and is a valid parent pom.xml, otherwise, maven tries to download from upstream.
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.xmlwill 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/manifestandinternal/resolution/manifestrequire basically the same logic for merging parent POM, I would like to refactor this in a following PR.