fix(matchUpstreamMavenPackages): Only search Maven if the metadata POM artifact ID and group ID are missing. #2547
Conversation
…M artifact ID and group ID are missing. Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev>
…tion test demonstrating skipped searches Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev>
| // The config file (./test-fixtures/config/matcher_integration_test_config.yaml) enables the maven search | ||
| // Additionally, increased the rate-limit to 500ms to avoid further 403 Forbiddens from Maven | ||
| // This integration test takes approximately 400s | ||
| func TestMavenSearch_MatchUpstreamMavenPackages(t *testing.T) { |
There was a problem hiding this comment.
Thi test is more like a CLI test (running a compiled version of grype) which isn't ideal. I think a small refactor would make this much easier to write a unit test for and the test code smaller/easier to reason about -- by decoupling the decision to search by sha or not into another function:
func (m *Matcher) shouldSearchMavenBySha(p pkg.Package) (bool, []string) {
digests := []string{}
if metadata, ok := p.Metadata.(pkg.JavaMetadata); ok {
if metadata.PomArtifactID == "" || metadata.PomGroupID == "" {
for _, digest := range metadata.ArchiveDigests {
if digest.Algorithm == "sha1" {
digests = append(digests, digest.Value)
}
}
}
}
return len(digests) > 0, digests
}or something like this. Now the test can assert the correctness of the decision without mocks or needing to test the running times between two configurations.
Can you refactor this approach or another that improves the testability?
There was a problem hiding this comment.
Hey @wagoodman, thank you for the quick review!!
And yes, I agree, it was a bit messy for that integration test. I like your proposal for the refactor, I'll give that a go today. Thanks.
…determine if we need to search Maven Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev>
Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev>
|
I've updated the PR to now include the following changes based on @wagoodman’s suggestion above:
|
grype/matcher/java/matcher.go
Outdated
| } | ||
| // If we need to search Maven but no valid SHA-1 digests exist, return an error | ||
| if len(digests) == 0 { | ||
| return true, nil, fmt.Errorf("missing SHA-1 digest; cannot search Maven for package %s", p.Name) |
There was a problem hiding this comment.
I don't think we want to bail early / return an error if we have no digest nor pom info. I think we should still let the default "skipping maven search" still occur. If that's true, then this function won't need an error returned.
There was a problem hiding this comment.
Okay, that sounds good to me. I've updated the code :)
…MavenBySha Signed-off-by: tdunlap607 <trevor.dunlap@chainguard.dev>
|
Hey @wagoodman, just a friendly check-in to see if any other changes are required for this PR. Thanks! |
Alex is out this week - Thanks for the changes @tdunlap607 this LGTM I've run the internal checks and will enable auto merge in a second. |
Summary
This PR addresses issue #2216: Should only check maven central if pom info is missing. The goal is to reduce the number of Maven searches, to help reduce the possibility of rate limits (Maven Error 403 Forbidden).
Fixes #2216
Changes
Updated matchUpstreamMavenPackages to first check for
metadata.PomArtifactIDandmetadata.PomGroupId.Testing
TestMatcherJava_matchUpstreamMavenPackageunit test to account for the new logic. This included added data to grype/matcher/java/matcher_mocks_test.goTestMavenSearch_MatchUpstreamMavenPackages) comparing these updates to Grype v0.90.0, confirming that when POM data is available, we can skip Maven searches while maintaining the same results. An additional config test file was added undergrype/matcher/java/test-fixtures/config/matcher_integration_test_config.yamlto enable Maven searches for the integration test.Here’s an example output from the integration test:
Happy to chat more or make additional changes! :)
cc @luhring