Add io.Reader variants to lockfile package
#176
Comments
|
Hey @picatz, thanks for the suggestion! Indeed this is something we'd like to have. We've also been discussing internally with some other potential users of this library around some other refactorings to make this package more usable generally (that includes this). We'll hopefully have something ready to share for input soon :) |
|
fwiw I've made a start on this - patch is attached; main thing I'm still doing is figuring out how to make the JSON/YAML/TOML/XML parsers play nice with an Am pausing for now in favor of #81, but will hopefully be able to have it done in a couple of weeks. patch |
|
Ok so I think I've got a path forward for supporting JSON/YAML/TOML/XML parsers with However, I think #183 should be discussed/landed first so am waiting on that before continuing. |
|
Have opened #189 😅 |
|
Thanks @G-Rath! Let's use these to start discussions, but I'd maybe recommend not doing too much further work on these until we understand all the requirements :) Hopefully in the next few weeks -- we have some potential internal consumers as well who will have input. |
|
@oliverchang sure, though my PR covers exactly what @picatz has asked for, and that I think is a no brainer. Combined with supporting "diagnostics" in my other PR (which allows the parsers to return any extra arbitrary data folks might want), I'm not sure what uses cases couldn't be met without further internal changes 🤷♂️ Still, happy to wait until you've had those internal conversation 🙂 |
e.g. We may want e.g. Thanks again for working on this! |
|
I'm not sure why you'd want to do that though when none of the lockfile parsers need that? So you'd just be restricting their input, and you should still be able to pass in a My understanding is that |
Again, part of our internal conversations :) (and this is just one example). Let's circle back one we have more details / clarity. |
|
Sorry for the slow replies here. That said, we should start defining interfaces for the parsers here to make the namespace a bit cleaner + make it clearer what exactly needs to be implemented for a particular lockfile format. Something like: interface Parser {
Parse(r io.Reader) []PackageDetails
}We should still keep the individual @G-Rath @another-rex WDYT? |
|
Are you meaning like: fwiw, #260 has made things a bit tricker - we now also need to expose a way for parsers to read "another file"; we shouldn't make this change without resolving this too, because it's important for supporting container scanning. |
Ah good callout. It seems like to fully support this we will need some kind of lightweight filesystem abstraction as well then. As you mention this does seem to tie in closely to container scanning as well. |
|
Pushing this thread along a bit again. How about something along the lines of: interface FSAccessor {
// path should be relative to the main input path.
Get(path string) io.ReaderCloser
}
type Input struct {
Reader io.Reader // the main input
FS FSAccessor // optional, but this is required in some cases for certain ecosystems. An error should be returned by the Parser if this is required during parsing and not provided.
}
interface Parser {
// Heuristic to check if a given path/filename is supported by the parser.
ShouldParse(path string) bool
Parse(input Input) ([]PackageDetails, error)
}Having the main input be a struct also helps with future extensibility without breaking compatibility. |
|
|
|
I'm thinking a mix between the two options, something like this: // ParsableFile is an abstraction for a file that has been opened for parsing
// to create a Lockfile, and that knows how to open other ParsableFiles
// relative to itself.
type ParsableFile interface {
io.ReadCloser
// if string is a relative path, open relative to the current file
// otherwise open the file at the absolute path
Open(string) (ParsableFile, error)
Path() string
}
type Parser interface {
// Heuristic to check if a given path/filename is supported by the parser.
ShouldParse(path string) bool
Parse(input ParsableFile) ([]PackageDetails, error)
}This allows the implementer to also support specifying files on absolute paths. |
|
The interface we are planning on going with: var ErrNotSupported = errors.New("this file does not support opening files")
// DependenciesFile is an abstraction for a file that has been opened for extraction, and that knows how to open other DependenciesFile relative to itself.
type DependenciesFile interface {
io.ReadCloser
// if string is a relative path, open relative to the current file
// otherwise open the file at the absolute path
//
// If the DependenciesFile does not implement Open, Extractor should
// record the failure to open the file, and return the partial list
// of PackageDetails, in addition to ErrNotSupported error
Open(string) (DependenciesFile, error)
Path() string
}
type Extractor interface {
// Heuristic to check if a given path/filename is supported by the parser.
ShouldExtract(path string) bool
Extract(input DependenciesFile) ([]PackageDetails, error)
}This allows a lot of flexibility to the implementer for how to open files, including opening with custom fs.FS implementation. |
|
A final update to the design, the main change is to separate out DependenciesFile with one that has to be closed, and one that doesn't. This helps prevent hard to catch not closing/double close errors when using the interface. var ErrNotSupported = errors.New("this file does not support opening files")
// DepFile is an abstraction for a file that has been opened for extraction, and that knows how to open other DependenciesFile relative to itself.
type DepFile interface {
io.Reader
// if string is a relative path, open relative to the current file
// otherwise open the file at the absolute path
//
// If the DependenciesFile does not implement Open, Extractor should
// record the failure to open the file, and return the partial list
// of PackageDetails, in addition to ErrNotSupported error
Open(string) (NestedDepFile, error)
Path() string
}
// NestedDepFile is an abstraction for a file that has been opened while extracting another file, and would need to be closed.
type NestedDepFile interface {
io.Closer
DepFile
}
type Extractor interface {
// Heuristic to check if a given path/filename is supported by the parser.
ShouldExtract(path string) bool
Extract(input DepFile) ([]PackageDetails, error)
}Both // A LocalFile represents a file that exists on the local filesystem.
type LocalFile struct {
io.ReadCloser
path string
}
func (f LocalFile) Open(path string) (NestedParsableFile, error) {
...
}
func (f LocalFile) Path() string { return f.path }
var _ ParsableFile = LocalFile{}
var _ NestedParsableFile = LocalFile{} |
At this time, the
lockfilepackage's external functions only accept a path name, so it expects a file to be available on disk. For my use case, this isn't always true or ideal. Preferably, there would be an option to use a path name and anio.Reader.@cmaritan's work in #164 with
ParseApkInstalledFromReaderserves as a good foundation to be expanded upon:osv-scanner/pkg/lockfile/apk-installed.go
Line 74 in a418625
osv-scanner/pkg/lockfile/apk-installed.go
Line 84 in a418625
The text was updated successfully, but these errors were encountered: