-
Notifications
You must be signed in to change notification settings - Fork 598
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
Introduce new format pattern + port json processing #550
Conversation
Benchmark Test ResultsBenchmark results from the latest changes vs base branch
|
syft/format/format.go
Outdated
return f.decoder(reader) | ||
} | ||
|
||
func (f Format) Detect(b []byte) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From @luhring : could this be Validate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and if so, probably adjust the signature to be consistent with our new Validator
concept (e.g. return an error
))
syft/format/format.go
Outdated
} | ||
|
||
if err := f.validator(bytes.NewReader(b)); err != nil { | ||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From @spiffcs : maybe log debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging here probably won't be helpful since we're expecting validation errors (which could get noisy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve based off changes highlighted in our zoom meeting.
The TODO left seem to be larger enough PR that they should not be included here.
) | ||
|
||
// Encode takes all SBOM elements and a format option and encodes an SBOM document. | ||
// TODO: encapsulate input data into common sbom document object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want me to keep track of the TODO still found in this PR and break them into separate smaller issues to solve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! (pending resolution of the other threads of thought already discussed)
Excellent work!! 👏
|
||
import "io" | ||
|
||
type Validator func(reader io.Reader) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: For future implementers, it could be nice to add a doc comment to explain what's intended to be implemented in a Validator. E.g., // A Validator assess the input to determine blah blah blah ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed -- let me follow up with some useful doc strings
return err | ||
} | ||
s.Target = payload | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: blank line above this line for consistency within the switch
return fmt.Errorf("unable to decode: %w", err) | ||
} | ||
|
||
// note: we accept al schema versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: al
-> all
internal/formats/formats.go
Outdated
|
||
func Identify(by []byte) (*format.Format, error) { | ||
for _, f := range All() { | ||
if f.Detect(by) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Potentially changing this to create a new reader, based on a previous comment)
internal/formats/formats_test.go
Outdated
) | ||
|
||
func TestIdentify(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: stray empty line
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
d11e487
to
139c2b8
Compare
* add new format pattern Signed-off-by: Alex Goodman <[email protected]> * add syftjson format Signed-off-by: Alex Goodman <[email protected]> * add internal formats helper Signed-off-by: Alex Goodman <[email protected]> * add SBOM encode/decode to lib API Signed-off-by: Alex Goodman <[email protected]> * remove json presenter + update presenter tests to use common utils Signed-off-by: Alex Goodman <[email protected]> * remove presenter format enum type + add formats shim in presenter helper Signed-off-by: Alex Goodman <[email protected]> * add MustCPE helper for tests Signed-off-by: Alex Goodman <[email protected]> * update usage of format enum Signed-off-by: Alex Goodman <[email protected]> * add test fixtures for encode/decode tests Signed-off-by: Alex Goodman <[email protected]> * fix integration test Signed-off-by: Alex Goodman <[email protected]> * migrate format detection to use reader Signed-off-by: Alex Goodman <[email protected]> * address review comments Signed-off-by: Alex Goodman <[email protected]>
Partially addresses anchore/grype#395
Today we have the
presenter
abstraction to write out internal SBOM results to a particular format (e.g. JSON, table, SPDX, CycloneDX). All model details about the format itself is contained under the presenter objects themselves. This abstraction was never meant to encapsulate handling intake (parsing) of these formats. As we add more formats it would be ideal to be able to parse these formats for use in other tools (such as syft). For this reason this PR adds a newformat
abstraction for specifying encoding/decoding details for a particular SBOM format (and the presenter can use format encoders to deal with presentation concerns).The first commit of this PR adds the new format abstractions:
type Encoder ...
: a function signature for encoding a given set of SBOM objects into bytes that are written to a writer.type Decoder ...
: a function signature for decoding an SBOM from a reader and returning SBOM objects.type Validator ...
: a function signature for observing the bytes of an SBOM document via a reader and returns any errors if the given document is not of a specific format.type Format ...
: ties together the above bits of functionality into a single object tied to a specificformat.Option
(with helper functions)This new pattern allows for:
This PR ports encoding from the syft presenters and decoding from the grype pkg.Provider implementation for the
syftjson
format.Additionally the encode/decode behavior has been exposed in the syft top-level API , bypassing the presenter abstraction for the typical user (where deferred encoding is not necessary).