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

Refactor package scanning to produce packages instead of queries #614

Merged
merged 6 commits into from
Nov 1, 2023

Conversation

josieang
Copy link
Collaborator

this is in preparation for the license scanning feature. the queries are structured around making requests to the osv API, we also will want to make requests to the deps.dev api.
#501

osv.BatchedQuery

We are planning to create deps.dev queries from these packages. Rather
than use the osv.BatchedQuery type to construct deps.dev queries from,
create a new type Package that includes package information and source
information.
@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2023

Codecov Report

Merging #614 (6b658da) into main (b099238) will increase coverage by 0.03%.
The diff coverage is 74.56%.

@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
+ Coverage   80.30%   80.33%   +0.03%     
==========================================
  Files          78       78              
  Lines        5381     5406      +25     
==========================================
+ Hits         4321     4343      +22     
- Misses        885      887       +2     
- Partials      175      176       +1     
Files Coverage Δ
pkg/osvscanner/vulnerability_result.go 78.43% <92.30%> (+0.43%) ⬆️
pkg/osvscanner/osvscanner.go 60.63% <72.27%> (+3.01%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Thanks! Added some comments, mainly about whether we need the Package struct or if we can reuse the Query struct, but still follow the return pattern in this refactor rather than the original appending to a big slice.

@@ -505,6 +516,15 @@ func parseLockfilePath(lockfileElem string) (string, string) {
return splits[0], splits[1]
}

type Package struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make this private? (is there any public functions that take this struct in?) We want to reduce the amount of stuff made public to make future refactoring easier.

Also can you name it something like queryPackage to make it clear what it's purpose is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... now that I think about it, do we even need this intermediate type? It looks like what we're looking for is just the osv.Query type, but rather than being appended to a pointer being passed in, we return it as an array.

This should reduce the number of changes while still accomplishing what you want right?

Copy link
Collaborator Author

@josieang josieang Nov 1, 2023

Choose a reason for hiding this comment

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

I make a separate Package object because it doesn't make semantic sense to use an osv.Query object to create a depsdev query. While it might be possible to do so, I think it's a misuse of the meaning of the osv.Query object and we should be making osv and depsdev queries from an abstracted package object instead.

In the case where commit information and package information are available, it's valid to just construct an osv.Query that just contains just the commit [1]. in this case we wouldn't be able to construct a depsdev query from the osv query. I'm not sure whether this scenario exists yet, but I think it illustrates why it's cleaner to have a separate package object.

As discussed offline I'll make this a private type and submit it as is!

[1] https://github.com/google/osv-scanner/pull/614/files#diff-07423256a3c05c7b4f0b6147545f87ae156c5426f13527a5ef76f58afae8b022R375

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

LGTM!

@another-rex another-rex merged commit ac2897c into google:main Nov 1, 2023
@josieang josieang mentioned this pull request Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants