Repository: ossf/package-analysis (language: python)

Issue:
Analysed package is not saved with correct name
The upload of the analyzed package tarball implemented in #739 and #778 doesn't use the correct name to save to the bucket.

It is supposed to involve the package name and version, followed by the hash, but the name used contains just the hash preceeded by `"-"`. It seems like `rs.generatePath(pkg)` in https://github.com/ossf/package-analysis/blob/af891961604f4288a64073c352e9431a24d84a18/internal/resultstore/resultstore.go#L139 is generating / returning an empty path, for some reason


Discussion / hints:

Fix patch:
diff --git a/cmd/analyze/main.go b/cmd/analyze/main.go
index 1633d1aa..70a1bb0a 100644
--- a/cmd/analyze/main.go
+++ b/cmd/analyze/main.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"flag"
 	"fmt"
-	"net/url"
 	"os"
 	"strings"
 
@@ -45,35 +44,21 @@ var (
 		"list of analysis modes to run, separated by commas. Use -list-modes to see available options")
 )
 
-func parseBucketPath(path string) (string, string) {
-	parsed, err := url.Parse(path)
-	if err != nil {
-		log.Panic("Failed to parse bucket path",
-			"path", path)
-	}
-
-	return parsed.Scheme + "://" + parsed.Host, parsed.Path
-}
-
 func makeResultStores() worker.ResultStores {
 	rs := worker.ResultStores{}
 
 	if *dynamicUpload != "" {
-		bucket, path := parseBucketPath(*dynamicUpload)
-		rs.DynamicAnalysis = resultstore.New(bucket, resultstore.BasePath(path))
+		rs.DynamicAnalysis = resultstore.New(*dynamicUpload)
 	}
 	if *staticUpload != "" {
-		bucket, path := parseBucketPath(*staticUpload)
-		rs.StaticAnalysis = resultstore.New(bucket, resultstore.BasePath(path))
+		rs.StaticAnalysis = resultstore.New(*staticUpload)
 	}
 	if *uploadFileWriteInfo != "" {
-		bucket, path := parseBucketPath(*uploadFileWriteInfo)
-		rs.FileWrites = resultstore.New(bucket, resultstore.BasePath(path))
+		rs.FileWrites = resultstore.New(*uploadFileWriteInfo)
 	}
 
 	if *uploadAnalyzedPkg != "" {
-		bucket, path := parseBucketPath(*uploadAnalyzedPkg)
-		rs.AnalyzedPackage = resultstore.New(bucket, resultstore.BasePath(path))
+		rs.AnalyzedPackage = resultstore.New(*uploadAnalyzedPkg)
 	}
 
 	return rs
diff --git a/go.mod b/go.mod
index e6419de0..4bb4be64 100644
--- a/go.mod
+++ b/go.mod
@@ -10,7 +10,7 @@ require (
 	github.com/package-url/packageurl-go v0.1.1
 	github.com/texttheater/golang-levenshtein/levenshtein v0.0.0-20200805054039-cae8b0eaed6c
 	go.uber.org/zap v1.25.0
-	gocloud.dev v0.33.0
+	gocloud.dev v0.33.1-0.20230808164326-dee61a596af0
 	gocloud.dev/pubsub/kafkapubsub v0.33.0
 	golang.org/x/exp v0.0.0-20230321023759-10a507213a29
 	google.golang.org/api v0.136.0
diff --git a/go.sum b/go.sum
index a2595781..422a360c 100644
--- a/go.sum
+++ b/go.sum
@@ -420,8 +420,8 @@ go.uber.org/zap v1.10.0/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
 go.uber.org/zap v1.25.0 h1:4Hvk6GtkucQ790dqmj7l1eEnRdKm3k3ZUrUMS2d5+5c=
 go.uber.org/zap v1.25.0/go.mod h1:JIAUzQIH94IC4fOJQm7gMmBJP5k7wQfdcnYdPoEXJYk=
 gocloud.dev v0.22.0/go.mod h1:z3jKIQ0Es9LALVZFQ3wOvwqAsSLq1R5c/2RdmghDucw=
-gocloud.dev v0.33.0 h1:ET5z49jm1+eUhY5BkuGk2d7czfgGeXKd4vtg1Jcg9OQ=
-gocloud.dev v0.33.0/go.mod h1:z6W8qorjrfM09H8t1MDk8KLPj3Xi26aFBzDKAHWIgLU=
+gocloud.dev v0.33.1-0.20230808164326-dee61a596af0 h1://FYsd3KnVdEJOVT6JaWvAKvUjvKnDnnovQnuwc33Ec=
+gocloud.dev v0.33.1-0.20230808164326-dee61a596af0/go.mod h1:z6W8qorjrfM09H8t1MDk8KLPj3Xi26aFBzDKAHWIgLU=
 gocloud.dev/pubsub/kafkapubsub v0.22.0/go.mod h1:71PFhLtS1ymaBd1BEj6B5m4v8pFmLQ3GPT1O9XwDtwM=
 gocloud.dev/pubsub/kafkapubsub v0.33.0 h1:cSE9mmkisZ3/s01SIun6mXJJNwxMA/6XvyozlwgQQWo=
 gocloud.dev/pubsub/kafkapubsub v0.33.0/go.mod h1:UfHC/CNXpZTHW3pMJgjaAnm4Hz4vB7N18A4K81rKoV4=
diff --git a/internal/resultstore/resultstore.go b/internal/resultstore/resultstore.go
index a6be8296..d9a8a957 100644
--- a/internal/resultstore/resultstore.go
+++ b/internal/resultstore/resultstore.go
@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"io"
+	"net/url"
 	"os"
 	"path"
 	"time"
@@ -21,8 +22,8 @@ import (
 )
 
 type ResultStore struct {
-	bucket        string
-	basePath      string
+	bucket        *url.URL
+	keyPrefix     string
 	constructPath bool
 }
 
@@ -39,15 +40,34 @@ func ConstructPath() Option {
 	return option(func(rs *ResultStore) { rs.constructPath = true })
 }
 
-// BasePath sets the base path used while saving files to storage.
-func BasePath(base string) Option {
-	return option(func(rs *ResultStore) { rs.basePath = base })
-}
-
+// New creates a new ResultStore instance with the given bucket URL and options.
+// If the bucket URL is invalid, a nil pointer is returned.
 func New(bucket string, options ...Option) *ResultStore {
+	bucketURL, err := url.Parse(bucket)
+	if err != nil {
+		return nil
+	}
+
+	var pathPrefix string
+
+	if bucketURL.Scheme == "file" {
+		// https://github.com/google/go-cloud/issues/3294
+		params := bucketURL.Query()
+		params.Set("no_tmp_dir", "true")
+		bucketURL.RawQuery = params.Encode()
+	} else {
+		// for non-file schemes, we need to separate the path because OpenBucket
+		// doesn't support turning the path into a key prefix
+		pathPrefix = bucketURL.Path
+		bucketURL.Path = ""
+		bucketURL.RawPath = ""
+	}
+
 	rs := &ResultStore{
-		bucket: bucket,
+		bucket:    bucketURL,
+		keyPrefix: pathPrefix,
 	}
+
 	for _, o := range options {
 		o.set(rs)
 	}
@@ -55,40 +75,47 @@ func New(bucket string, options ...Option) *ResultStore {
 }
 
 func (rs *ResultStore) String() string {
-	s := rs.bucket + "/" + rs.basePath
+	// label when bucket path is constructed from package name
 	if rs.constructPath {
-		s += "+"
+		return rs.bucket.JoinPath(rs.keyPrefix, "<dynamic path>").String()
 	}
-	return s
+
+	return rs.bucket.JoinPath(rs.keyPrefix).String()
 }
 
-func (rs *ResultStore) generatePath(p Pkg) string {
-	uploadPath := rs.basePath
-	if rs.constructPath {
-		uploadPath = path.Join(uploadPath, p.EcosystemName(), p.Name())
+// generateKey creates an identifier key to store an object with.
+// If p is non-nil and the ResultStore was constructed with
+// the ConstructPath() option, then the base key will be prefixed
+// with the ecosystem and name of the given package (in that order).
+// Otherwise, the basename is returned.
+func (rs *ResultStore) generateKey(p Pkg, baseKey string) string {
+	if p != nil && rs.constructPath {
+		return path.Join(rs.keyPrefix, p.EcosystemName(), p.Name(), baseKey)
 	}
-	return uploadPath
+	return path.Join(rs.keyPrefix, baseKey)
+}
+
+func (rs *ResultStore) openBucket(ctx context.Context) (*blob.Bucket, error) {
+	return blob.OpenBucket(ctx, rs.bucket.String())
 }
 
-func (rs *ResultStore) SaveTempFilesToZip(ctx context.Context, p Pkg, fileName string, tempFileNames []string) error {
-	bkt, err := blob.OpenBucket(ctx, rs.bucket)
+func (rs *ResultStore) SaveTempFilesToZip(ctx context.Context, p Pkg, zipName string, tempFileNames []string) error {
+	bkt, err := rs.openBucket(ctx)
 	if err != nil {
 		return err
 	}
 	defer bkt.Close()
 
-	uploadPath := path.Join(rs.generatePath(p), fileName+".zip")
-	log.Info("Uploading results",
-		"bucket", rs.bucket,
-		"path", uploadPath)
+	uploadPath := rs.generateKey(p, zipName+".zip")
+	log.Info("Uploading results", "bucket", rs.bucket.String(), "path", uploadPath)
 
-	w, err := bkt.NewWriter(ctx, uploadPath, nil)
+	bucketWriter, err := bkt.NewWriter(ctx, uploadPath, nil)
 	if err != nil {
 		return err
 	}
-	defer w.Close()
+	defer bucketWriter.Close()
 
-	zipWriter := zip.NewWriter(w)
+	zipWriter := zip.NewWriter(bucketWriter)
 	defer zipWriter.Close()
 
 	for _, fileName := range tempFileNames {
@@ -130,14 +157,14 @@ func (rs *ResultStore) SaveAnalyzedPackage(ctx context.Context, manager *pkgmana
 		return err
 	}
 
-	bkt, err := blob.OpenBucket(ctx, rs.bucket)
+	bkt, err := rs.openBucket(ctx)
 	if err != nil {
 		return err
 	}
 	defer bkt.Close()
 
-	uploadPath := rs.generatePath(pkg) + "-" + hash
-	log.Info("Uploading analyzed package", "bucket", rs.bucket, "path", uploadPath)
+	uploadPath := rs.generateKey(pkg, pkg.Version()+"-"+hash)
+	log.Info("Uploading analyzed package", "bucket", rs.bucket.String(), "path", uploadPath)
 
 	f, err := os.Open(archivePath)
 	if err != nil {
@@ -185,16 +212,14 @@ func (rs *ResultStore) SaveWithFilename(ctx context.Context, p Pkg, filename str
 		return err
 	}
 
-	bkt, err := blob.OpenBucket(ctx, rs.bucket)
+	bkt, err := rs.openBucket(ctx)
 	if err != nil {
 		return err
 	}
 	defer bkt.Close()
 
-	uploadPath := path.Join(rs.generatePath(p), filename)
-	log.Info("Uploading results",
-		"bucket", rs.bucket,
-		"path", uploadPath)
+	uploadPath := rs.generateKey(p, filename)
+	log.Info("Uploading results", "bucket", rs.bucket.String(), "path", uploadPath)
 
 	w, err := bkt.NewWriter(ctx, uploadPath, nil)
 	if err != nil {
@@ -210,29 +235,15 @@ func (rs *ResultStore) SaveWithFilename(ctx context.Context, p Pkg, filename str
 	return nil
 }
 
-// MakeFilename returns the default filename to use for saving analysis results,
-// using an optional label.
-// If the package has a version, the default filename is
-// "<label>-<version>.json" if label is nonempty, or <version>.json otherwise.
-// If the package does not have a version specified, the default filename is
-// "<label>.json" if label is nonempty, or "results.json" if not.
-func MakeFilename(p Pkg, label string) string {
-	prefix := "results"
-	version := p.Version()
-
-	if version != "" && label != "" {
-		prefix = label + "-" + version
-	} else if version != "" {
-		prefix = version
-	} else if label != "" {
-		prefix = label
-	}
-	return prefix + ".json"
-
-}
-
-// Save saves the results with the default filename
+// Save marshals the given analysis results to JSON and writes them to
+// the bucket with a default filename (key). If p is non-nil and has a
+// version specified, the default filename is <version>.json.
+// Otherwise, it is "results.json".
 func (rs *ResultStore) Save(ctx context.Context, p Pkg, analysis interface{}) error {
-	filename := MakeFilename(p, "")
+	filename := "results.json"
+	if p != nil && p.Version() != "" {
+		filename = p.Version() + ".json"
+	}
+
 	return rs.SaveWithFilename(ctx, p, filename, analysis)
 }
diff --git a/internal/worker/save_data.go b/internal/worker/save_data.go
index ee29fe2f..21bee4a0 100644
--- a/internal/worker/save_data.go
+++ b/internal/worker/save_data.go
@@ -66,7 +66,11 @@ func saveExecutionLog(ctx context.Context, pkg *pkgmanager.Pkg, dest *ResultStor
 		return nil
 	}
 
-	execLogFilename := resultstore.MakeFilename(pkg, "execution-log")
+	execLogFilename := "execution-log.json"
+	if pkg.Version() != "" {
+		execLogFilename = fmt.Sprintf("execution-log-%s.json", pkg.Version())
+	}
+
 	if err := dest.DynamicAnalysis.SaveWithFilename(ctx, pkg, execLogFilename, data.ExecutionLog); err != nil {
 		return fmt.Errorf("failed to save execution log to %s: %w", dest.DynamicAnalysis, err)
 	}


Test patch:
diff --git a/internal/resultstore/resultstore_test.go b/internal/resultstore/resultstore_test.go
new file mode 100644
index 00000000..5ef167c8
--- /dev/null
+++ b/internal/resultstore/resultstore_test.go
@@ -0,0 +1,60 @@
+package resultstore
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"path"
+	"path/filepath"
+	"testing"
+)
+
+func TestFileBucket(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	testBucketURL := "file://" + tmpDir
+	fmt.Println(testBucketURL)
+
+	testKeys := []string{
+		"test1.txt",
+		path.Join("testdir", "test2.txt"), // use path not filepath since it's a URL
+	}
+
+	ctx := context.Background()
+
+	rs := New(testBucketURL)
+	if rs == nil {
+		t.Errorf("failed to open create resultstore with URL %s (invalid url)", testBucketURL)
+	}
+
+	bucket, err := rs.openBucket(ctx)
+	if err != nil {
+		t.Errorf("failed to open bucket: %v", err)
+	}
+
+	for _, key := range testKeys {
+		t.Run(key, func(t *testing.T) {
+			writer, err := bucket.NewWriter(ctx, key, nil)
+			if err != nil {
+				t.Errorf("failed to create writer: %v", err)
+			}
+
+			if _, err := writer.Write([]byte("test bytes")); err != nil {
+				t.Errorf("failed to write to file: %v", err)
+			}
+
+			if err := writer.Close(); err != nil {
+				t.Errorf("failed to close writer: %v", err)
+			}
+
+			if _, err := os.Stat(filepath.Join(tmpDir, key)); err != nil {
+				t.Errorf("failed to stat file: %v", err)
+			}
+
+		})
+	}
+
+	if err := bucket.Close(); err != nil {
+		t.Errorf("failed to close bucket: %v", err)
+	}
+}