fix: in some cases, try to use pom info to guess name and version to top level jar #2080
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
All tests pass, but hacky. Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Benchmark Test ResultsBenchmark results from the latest changes vs base branch |
wagoodman
reviewed
Aug 31, 2023
| pomMatches := j.fileManifest.GlobMatch(pomXMLGlob) | ||
| var pomPropertiesObject pkg.PomProperties | ||
| var pomProjectObject pkg.PomProject | ||
| if len(pomPropertyMatches) == 1 && len(pomMatches) == 1 { |
There was a problem hiding this comment.
it's possible that we could have only a pom.xml and no properties, I think we should account for that as well (allow for 0 to 1 pom properties)
wagoodman
reviewed
Aug 31, 2023
|
|
||
| func TestWarCatalogedCorrectlyIfRenamed(t *testing.T) { | ||
| // install hudson-war@2.2.1 and renames the file to `/hudson.war` | ||
| // TODO: is this better expressed in syft/pkg/cataloger/java/archive_parser_test.go? |
There was a problem hiding this comment.
agreed, I'm not against having this integration test at all, so consider this comment non-blocking, however, testing specific business logic should be pushed as low as possible test-level-wise (unit is preferred for these kinds of assertions). We do have some regression tests at the integration level to test against the specific asset where we found the regression, so keeping this test here is ok.
There was a problem hiding this comment.
Thanks for the suggestion! I think I'll leave this here for now; it was reported as an end to end regression, and the parser was still finding the correct package, it was just also finding an incorrect extra package, so the end-to-end flow seems a good place of the test.
I'll push a commit to remove the TODO.
wagoodman
reviewed
Aug 31, 2023
| return &pkg.Package{ | ||
| Name: selectName(manifest, j.fileInfo), | ||
| Version: selectVersion(manifest, j.fileInfo), | ||
| // TODO: maybe select name should just have a pom properties in it? |
There was a problem hiding this comment.
or maybe rename selectName to something more specific like selectNameFromManifest (same for the version function) -- the upside is that you can write a selectNameAndVersion wrapper that is called here and pushes some of this lower level logic to another function, and the existing tests for selectName don't really need to get modified from a logic sense.
There was a problem hiding this comment.
Thanks for the suggestion! I think I'll do this as a follow up.
Signed-off-by: Will Murphy <will.murphy@anchore.com>
wagoodman
approved these changes
Aug 31, 2023
Signed-off-by: Will Murphy <will.murphy@anchore.com>
This was referenced Sep 1, 2023
Merged
This was referenced Sep 12, 2023
GijsCalis
pushed a commit
to GijsCalis/syft
that referenced
this pull request
Feb 19, 2024
…top level jar (anchore#2080) Otherwise, small renames like 'hudson-war-2.2.1.war' to 'hudson.war', would cause syft to incorrectly catolog the archive. Signed-off-by: Will Murphy <will.murphy@anchore.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the following changes in the java archive parser's
discoverMainPackage()method:if there is exactly one pom.xml and pom.properties in the jar and if the filename is a prefix of the artifact ID on the pom.properties, prefer, in order, for name and version:
Definitely open to feedback and refactoring here, but wanted to show a working fix before spending too much time making it look great.
Fixes #2077