Repository: google/osv-scanner
Pull request #345 (2023-04-13 22:36:46)

Refactor reporter to interfaces
The reporter has been refactored to be an interface with various implementations. Based on the idea from Michael here: https://github.com/google/osv-scanner/pull/341#discussion_r1164891532 

Want to get some feedback on whether this is worth changing or not.

I'm also not sure if we should have the different reporter types public here, or make them private and have the users of the library provide their own implementations of Reporter.


Patch:
diff --git a/cmd/osv-scanner/main.go b/cmd/osv-scanner/main.go
index c0c0df1..4eb96a1 100644
--- a/cmd/osv-scanner/main.go
+++ b/cmd/osv-scanner/main.go
@@ -20,10 +20,10 @@ var (
 )
 
 func run(args []string, stdout, stderr io.Writer) int {
-	var r *reporter.Reporter
+	var r reporter.Reporter
 
 	cli.VersionPrinter = func(ctx *cli.Context) {
-		r = reporter.NewReporter(ctx.App.Writer, ctx.App.ErrWriter, "")
+		r = reporter.NewTableReporter(stdout, stderr, false)
 		r.PrintText(fmt.Sprintf("osv-scanner version: %s\ncommit: %s\nbuilt at: %s\n", ctx.App.Version, commit, date))
 	}
 
@@ -67,7 +67,7 @@ func run(args []string, stdout, stderr io.Writer) int {
 					switch s {
 					case
 						"table",
-						"json",
+						"json", //nolint:goconst
 						"markdown":
 						return nil
 					}
@@ -109,7 +109,16 @@ func run(args []string, stdout, stderr io.Writer) int {
 				format = "json"
 			}
 
-			r = reporter.NewReporter(stdout, stderr, format)
+			switch format {
+			case "json":
+				r = reporter.NewJSONReporter(stdout, stderr)
+			case "table":
+				r = reporter.NewTableReporter(stdout, stderr, false)
+			case "markdown":
+				r = reporter.NewTableReporter(stdout, stderr, true)
+			default:
+				return fmt.Errorf("%v is not a valid format", format)
+			}
 
 			vulnResult, err := osvscanner.DoScan(osvscanner.ScannerActions{
 				LockfilePaths:            context.StringSlice("lockfile"),
@@ -133,7 +142,7 @@ func run(args []string, stdout, stderr io.Writer) int {
 
 	if err := app.Run(args); err != nil {
 		if r == nil {
-			r = reporter.NewReporter(stdout, stderr, "")
+			r = reporter.NewTableReporter(stdout, stderr, false)
 		}
 		if errors.Is(err, osvscanner.VulnerabilitiesFoundErr) {
 			return 1
diff --git a/internal/sourceanalysis/go.go b/internal/sourceanalysis/go.go
index 88ae0d5..59288a9 100644
--- a/internal/sourceanalysis/go.go
+++ b/internal/sourceanalysis/go.go
@@ -11,7 +11,7 @@ import (
 	"golang.org/x/vuln/exp/govulncheck"
 )
 
-func goAnalysis(r *reporter.Reporter, pkgs []models.PackageVulns, source models.SourceInfo) {
+func goAnalysis(r reporter.Reporter, pkgs []models.PackageVulns, source models.SourceInfo) {
 	vulns, vulnsByID := vulnsFromAllPkgs(pkgs)
 	res, err := govulncheckshim.RunGoVulnCheck(filepath.Dir(source.Path), vulns)
 	if err != nil {
diff --git a/internal/sourceanalysis/sourceanalysis.go b/internal/sourceanalysis/sourceanalysis.go
index 885a2d0..d6112fd 100644
--- a/internal/sourceanalysis/sourceanalysis.go
+++ b/internal/sourceanalysis/sourceanalysis.go
@@ -25,7 +25,7 @@ func vulnsFromAllPkgs(pkgs []models.PackageVulns) ([]models.Vulnerability, map[s
 }
 
 // Run runs the language specific analyzers on the code given packages and source info
-func Run(r *reporter.Reporter, source models.SourceInfo, pkgs []models.PackageVulns) {
+func Run(r reporter.Reporter, source models.SourceInfo, pkgs []models.PackageVulns) {
 	// GoVulnCheck
 	if source.Type == "lockfile" && filepath.Base(source.Path) == "go.mod" {
 		goAnalysis(r, pkgs, source)
diff --git a/pkg/config/config.go b/pkg/config/config.go
index cf076d1..6eaab8f 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -63,7 +63,7 @@ func (c *ConfigManager) UseOverride(configPath string) error {
 }
 
 // Attempts to get the config
-func (c *ConfigManager) Get(r *reporter.Reporter, targetPath string) Config {
+func (c *ConfigManager) Get(r reporter.Reporter, targetPath string) Config {
 	if c.OverrideConfig != nil {
 		return *c.OverrideConfig
 	}
diff --git a/pkg/osvscanner/osvscanner.go b/pkg/osvscanner/osvscanner.go
index a982bad..28f9dd8 100644
--- a/pkg/osvscanner/osvscanner.go
+++ b/pkg/osvscanner/osvscanner.go
@@ -52,7 +52,7 @@ var OnlyUncalledVulnerabilitiesFoundErr = errors.New("only uncalled vulnerabilit
 //   - Any lockfiles with scanLockfile
 //   - Any SBOM files with scanSBOMFile
 //   - Any git repositories with scanGit
-func scanDir(r *reporter.Reporter, query *osv.BatchedQuery, dir string, skipGit bool, recursive bool, useGitIgnore bool) error {
+func scanDir(r reporter.Reporter, query *osv.BatchedQuery, dir string, skipGit bool, recursive bool, useGitIgnore bool) error {
 	var ignoreMatcher *gitIgnoreMatcher
 	if useGitIgnore {
 		var err error
@@ -180,7 +180,7 @@ func (m *gitIgnoreMatcher) match(absPath string, isDir bool) (bool, error) {
 
 // scanLockfile will load, identify, and parse the lockfile path passed in, and add the dependencies specified
 // within to `query`
-func scanLockfile(r *reporter.Reporter, query *osv.BatchedQuery, path string, parseAs string) error {
+func scanLockfile(r reporter.Reporter, query *osv.BatchedQuery, path string, parseAs string) error {
 	var err error
 	var parsedLockfile lockfile.Lockfile
 
@@ -221,7 +221,7 @@ func scanLockfile(r *reporter.Reporter, query *osv.BatchedQuery, path string, pa
 
 // scanSBOMFile will load, identify, and parse the SBOM path passed in, and add the dependencies specified
 // within to `query`
-func scanSBOMFile(r *reporter.Reporter, query *osv.BatchedQuery, path string, fromFSScan bool) error {
+func scanSBOMFile(r reporter.Reporter, query *osv.BatchedQuery, path string, fromFSScan bool) error {
 	var errs []error
 	for _, provider := range sbom.Providers {
 		if fromFSScan && !provider.MatchesRecognizedFileNames(path) {
@@ -312,7 +312,7 @@ func getCommitSHA(repoDir string) (string, error) {
 }
 
 // Scan git repository. Expects repoDir to end with /
-func scanGit(r *reporter.Reporter, query *osv.BatchedQuery, repoDir string) error {
+func scanGit(r reporter.Reporter, query *osv.BatchedQuery, repoDir string) error {
 	commit, err := getCommitSHA(repoDir)
 	if err != nil {
 		return err
@@ -333,7 +333,7 @@ func scanGitCommit(query *osv.BatchedQuery, commit string, source string) error
 	return nil
 }
 
-func scanDebianDocker(r *reporter.Reporter, query *osv.BatchedQuery, dockerImageName string) error {
+func scanDebianDocker(r reporter.Reporter, query *osv.BatchedQuery, dockerImageName string) error {
 	cmd := exec.Command("docker", "run", "--rm", "--entrypoint", "/usr/bin/dpkg-query", dockerImageName, "-f", "${Package}###${Version}\\n", "-W")
 	stdout, err := cmd.StdoutPipe()
 
@@ -381,7 +381,7 @@ func scanDebianDocker(r *reporter.Reporter, query *osv.BatchedQuery, dockerImage
 }
 
 // Filters results according to config, preserving order. Returns total number of vulnerabilities removed.
-func filterResults(r *reporter.Reporter, results *models.VulnerabilityResults, configManager *config.ConfigManager) int {
+func filterResults(r reporter.Reporter, results *models.VulnerabilityResults, configManager *config.ConfigManager) int {
 	removedCount := 0
 	newResults := []models.PackageSource{} // Want 0 vulnerabilities to show in JSON as an empty list, not null.
 	for _, pkgSrc := range results.Results {
@@ -407,7 +407,7 @@ func filterResults(r *reporter.Reporter, results *models.VulnerabilityResults, c
 }
 
 // Filters package-grouped vulnerabilities according to config, preserving ordering. Returns filtered package vulnerabilities.
-func filterPackageVulns(r *reporter.Reporter, pkgVulns models.PackageVulns, configToUse config.Config) models.PackageVulns {
+func filterPackageVulns(r reporter.Reporter, pkgVulns models.PackageVulns, configToUse config.Config) models.PackageVulns {
 	ignoredVulns := map[string]struct{}{}
 	// Iterate over groups first to remove all aliases of ignored vulnerabilities.
 	var newGroups []models.GroupInfo
@@ -464,9 +464,9 @@ func parseLockfilePath(lockfileElem string) (string, string) {
 }
 
 // Perform osv scanner action, with optional reporter to output information
-func DoScan(actions ScannerActions, r *reporter.Reporter) (models.VulnerabilityResults, error) {
+func DoScan(actions ScannerActions, r reporter.Reporter) (models.VulnerabilityResults, error) {
 	if r == nil {
-		r = reporter.NewVoidReporter()
+		r = &reporter.VoidReporter{}
 	}
 
 	configManager := config.ConfigManager{
diff --git a/pkg/osvscanner/vulnerability_result.go b/pkg/osvscanner/vulnerability_result.go
index a477a5f..4a4260a 100644
--- a/pkg/osvscanner/vulnerability_result.go
+++ b/pkg/osvscanner/vulnerability_result.go
@@ -13,7 +13,7 @@ import (
 
 // groupResponseBySource converts raw OSV API response into structured vulnerability information
 // grouped by source location.
-func groupResponseBySource(r *reporter.Reporter, query osv.BatchedQuery, resp *osv.HydratedBatchedResponse, callAnalysis bool) models.VulnerabilityResults {
+func groupResponseBySource(r reporter.Reporter, query osv.BatchedQuery, resp *osv.HydratedBatchedResponse, callAnalysis bool) models.VulnerabilityResults {
 	output := models.VulnerabilityResults{
 		Results: []models.PackageSource{},
 	}
diff --git a/pkg/reporter/json_reporter.go b/pkg/reporter/json_reporter.go
new file mode 100644
index 0000000..f47785f
--- /dev/null
+++ b/pkg/reporter/json_reporter.go
@@ -0,0 +1,41 @@
+package reporter
+
+import (
+	"fmt"
+	"io"
+
+	"github.com/google/osv-scanner/internal/output"
+	"github.com/google/osv-scanner/pkg/models"
+)
+
+type JSONReporter struct {
+	hasPrintedError bool
+	stdout          io.Writer
+	stderr          io.Writer
+}
+
+func NewJSONReporter(stdout io.Writer, stderr io.Writer) *JSONReporter {
+	return &JSONReporter{
+		stdout:          stdout,
+		stderr:          stderr,
+		hasPrintedError: false,
+	}
+}
+
+func (r *JSONReporter) PrintError(msg string) {
+	fmt.Fprint(r.stderr, msg)
+	r.hasPrintedError = true
+}
+
+func (r *JSONReporter) HasPrintedError() bool {
+	return r.hasPrintedError
+}
+
+func (r *JSONReporter) PrintText(msg string) {
+	// Print non json text to stderr
+	fmt.Fprint(r.stderr, msg)
+}
+
+func (r *JSONReporter) PrintResult(vulnResult *models.VulnerabilityResults) error {
+	return output.PrintJSONResults(vulnResult, r.stdout)
+}
diff --git a/pkg/reporter/reporter.go b/pkg/reporter/reporter.go
index 0cf7171..d4f5e75 100644
--- a/pkg/reporter/reporter.go
+++ b/pkg/reporter/reporter.go
@@ -1,72 +1,19 @@
 package reporter
 
 import (
-	"fmt"
-	"io"
-	"strings"
-
-	"github.com/google/osv-scanner/internal/output"
 	"github.com/google/osv-scanner/pkg/models"
 )
 
-type Reporter struct {
-	stdout          io.Writer
-	stderr          io.Writer
-	format          string
-	hasPrintedError bool
-}
-
-func NewReporter(stdout io.Writer, stderr io.Writer, format string) *Reporter {
-	return &Reporter{
-		stdout: stdout,
-		stderr: stderr,
-		format: format,
-	}
-}
-
-// NewVoidReporter creates a reporter that doesn't report to anywhere
-func NewVoidReporter() *Reporter {
-	stdout := new(strings.Builder)
-	stderr := new(strings.Builder)
-
-	return NewReporter(stdout, stderr, "")
-}
-
-// PrintError writes the given message to stderr, regardless of if the reporter
-// is outputting as JSON or not
-func (r *Reporter) PrintError(msg string) {
-	fmt.Fprint(r.stderr, msg)
-	r.hasPrintedError = true
-}
-
-func (r *Reporter) HasPrintedError() bool {
-	return r.hasPrintedError
-}
-
-// PrintText writes the given message to stdout, _unless_ the reporter is set
-// to output as JSON, in which case it writes the message to stderr.
-//
-// This should be used for content that should always be outputted, but that
-// should not be captured when piping if outputting JSON.
-func (r *Reporter) PrintText(msg string) {
-	target := r.stdout
-
-	if r.format == "json" {
-		target = r.stderr
-	}
-
-	fmt.Fprint(target, msg)
-}
-
-func (r *Reporter) PrintResult(vulnResult *models.VulnerabilityResults) error {
-	switch r.format {
-	case "json":
-		return output.PrintJSONResults(vulnResult, r.stdout)
-	case "markdown":
-		output.PrintMarkdownTableResults(vulnResult, r.stdout)
-	case "table":
-		output.PrintTableResults(vulnResult, r.stdout)
-	}
-
-	return nil
+type Reporter interface {
+	// PrintError writes the given message to stderr, regardless of if the reporter
+	// is outputting as JSON or not
+	PrintError(msg string)
+	HasPrintedError() bool
+	// PrintText writes the given message to stdout, _unless_ the reporter is set
+	// to output as JSON, in which case it writes the message to stderr.
+	//
+	// This should be used for content that should always be outputted, but that
+	// should not be captured when piping if outputting JSON.
+	PrintText(msg string)
+	PrintResult(vulnResult *models.VulnerabilityResults) error
 }
diff --git a/pkg/reporter/table_reporter.go b/pkg/reporter/table_reporter.go
new file mode 100644
index 0000000..7857129
--- /dev/null
+++ b/pkg/reporter/table_reporter.go
@@ -0,0 +1,48 @@
+package reporter
+
+import (
+	"fmt"
+	"io"
+
+	"github.com/google/osv-scanner/internal/output"
+	"github.com/google/osv-scanner/pkg/models"
+)
+
+type TableReporter struct {
+	hasPrintedError bool
+	stdout          io.Writer
+	stderr          io.Writer
+	markdown        bool
+}
+
+func NewTableReporter(stdout io.Writer, stderr io.Writer, markdown bool) *TableReporter {
+	return &TableReporter{
+		stdout:          stdout,
+		stderr:          stderr,
+		hasPrintedError: false,
+		markdown:        markdown,
+	}
+}
+
+func (r *TableReporter) PrintError(msg string) {
+	fmt.Fprint(r.stderr, msg)
+	r.hasPrintedError = true
+}
+
+func (r *TableReporter) HasPrintedError() bool {
+	return r.hasPrintedError
+}
+
+func (r *TableReporter) PrintText(msg string) {
+	fmt.Fprint(r.stdout, msg)
+}
+
+func (r *TableReporter) PrintResult(vulnResult *models.VulnerabilityResults) error {
+	if r.markdown {
+		output.PrintMarkdownTableResults(vulnResult, r.stdout)
+	} else {
+		output.PrintTableResults(vulnResult, r.stdout)
+	}
+
+	return nil
+}
diff --git a/pkg/reporter/void_reporter.go b/pkg/reporter/void_reporter.go
new file mode 100644
index 0000000..c90f455
--- /dev/null
+++ b/pkg/reporter/void_reporter.go
@@ -0,0 +1,22 @@
+package reporter
+
+import "github.com/google/osv-scanner/pkg/models"
+
+type VoidReporter struct {
+	hasPrintedError bool
+}
+
+func (r *VoidReporter) HasPrintedError() bool {
+	return r.hasPrintedError
+}
+
+func (r *VoidReporter) PrintText(msg string) {
+}
+
+func (r *VoidReporter) PrintResult(vulnResult *models.VulnerabilityResults) error {
+	return nil
+}
+
+func (r *VoidReporter) PrintError(msg string) {
+	r.hasPrintedError = true
+}


Test patch:
diff --git a/pkg/osvscanner/osvscanner_internal_test.go b/pkg/osvscanner/osvscanner_internal_test.go
index f45e38d..a539245 100644
--- a/pkg/osvscanner/osvscanner_internal_test.go
+++ b/pkg/osvscanner/osvscanner_internal_test.go
@@ -51,7 +51,7 @@ func Test_filterResults(t *testing.T) {
 		tt := tt // Reinitialize for t.Parallel()
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			r := reporter.NewVoidReporter()
+			r := &reporter.VoidReporter{}
 			// ConfigManager looks for osv-scanner.toml in the source path.
 			// Sources in the test input should point to files/folders in the text fixture folder for this to work correctly.
 			configManager := config.ConfigManager{
diff --git a/pkg/osvscanner/vulnerability_result_internal_test.go b/pkg/osvscanner/vulnerability_result_internal_test.go
index 212b5ff..36b789e 100644
--- a/pkg/osvscanner/vulnerability_result_internal_test.go
+++ b/pkg/osvscanner/vulnerability_result_internal_test.go
@@ -12,7 +12,7 @@ import (
 func Test_groupResponseBySource(t *testing.T) {
 	t.Parallel()
 	type args struct {
-		r            *reporter.Reporter
+		r            reporter.Reporter
 		query        osv.BatchedQuery
 		resp         *osv.HydratedBatchedResponse
 		callAnalysis bool