internal/scan: add generalized param parsing
Add a function that parses query params according to struct fields.
Also add a function that parses just the URL path into module, version
and suffix.
Change-Id: I219b3953462b0c3614d8da9147749b27ce5f6833
Reviewed-on: https://go-review.googlesource.com/c/pkgsite-metrics/+/467215
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Julie Qiu <julieqiu@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/internal/queue/queue_test.go b/internal/queue/queue_test.go
index ded8d00..be5ec81 100644
--- a/internal/queue/queue_test.go
+++ b/internal/queue/queue_test.go
@@ -64,11 +64,15 @@
TaskNameSuffix: "suf",
}
sreq := &scan.Request{
- Module: "mod",
- Version: "v1.2.3",
- ImportedBy: 0,
- Mode: "test",
- Insecure: true,
+ ModuleURLPath: scan.ModuleURLPath{
+ Module: "mod",
+ Version: "v1.2.3",
+ },
+ RequestParams: scan.RequestParams{
+ ImportedBy: 0,
+ Mode: "test",
+ Insecure: true,
+ },
}
got, err := gcp.newTaskRequest(sreq, opts)
if err != nil {
diff --git a/internal/scan/parse.go b/internal/scan/parse.go
index be59220..76aeaaa 100644
--- a/internal/scan/parse.go
+++ b/internal/scan/parse.go
@@ -7,9 +7,11 @@
import (
"bufio"
+ "errors"
"fmt"
"net/http"
"os"
+ "reflect"
"strconv"
"strings"
@@ -20,14 +22,15 @@
// Request contains information passed
// to a scan endpoint.
type Request struct {
- Module string
- Version string
- Suffix string
+ ModuleURLPath
+ RequestParams
+}
+
+// RequestParams has query parameters for a scan request.
+type RequestParams struct {
ImportedBy int
Mode string
Insecure bool
-
- // TODO: support optional parameters?
}
func (r *Request) URLPathAndParams() string {
@@ -55,55 +58,27 @@
// - <module>/@latest
//
// (These are the same forms that the module proxy accepts.)
-func ParseRequest(r *http.Request, scanPrefix string) (*Request, error) {
- mod, vers, suff, err := ParseModuleVersionSuffix(strings.TrimPrefix(r.URL.Path, scanPrefix))
+func ParseRequest(r *http.Request, scanPrefix string) (_ *Request, err error) {
+ defer derrors.Wrap(&err, "ParseRequest(%s)", scanPrefix)
+
+ mp, err := ParseModuleURLPath(strings.TrimPrefix(r.URL.Path, scanPrefix))
if err != nil {
return nil, err
}
- importedBy, err := ParseRequiredIntParam(r, "importedby")
- if err != nil {
+
+ rp := RequestParams{ImportedBy: -1} // use -1 to detect missing param (explicit 0 is OK)
+ if err := ParseParams(r, &rp); err != nil {
return nil, err
}
- insecure, err := ParseOptionalBoolParam(r, "insecure", false)
- if err != nil {
- return nil, err
+ if rp.ImportedBy < 0 {
+ return nil, errors.New(`missing or negative "importedby" query param`)
}
return &Request{
- Module: mod,
- Version: vers,
- Suffix: suff,
- ImportedBy: importedBy,
- Mode: ParseMode(r),
- Insecure: insecure,
+ ModuleURLPath: mp,
+ RequestParams: rp,
}, nil
}
-// ParseModuleVersionSuffix returns the module path, version and suffix described by
-// the argument. The suffix is the part of the path after the version.
-func ParseModuleVersionSuffix(requestPath string) (path, vers, suffix string, err error) {
- p := strings.TrimPrefix(requestPath, "/")
- modulePath, versionAndSuffix, found := strings.Cut(p, "@")
- if !found {
- return "", "", "", fmt.Errorf("invalid path %q: missing '@'", requestPath)
- }
- modulePath = strings.TrimSuffix(modulePath, "/")
- if modulePath == "" {
- return "", "", "", fmt.Errorf("invalid path %q: missing module", requestPath)
- }
- if strings.HasPrefix(versionAndSuffix, "v/") {
- versionAndSuffix = versionAndSuffix[2:]
- }
- // Now versionAndSuffix begins with a version.
- version, suffix, _ := strings.Cut(versionAndSuffix, "/")
- if version == "" {
- return "", "", "", fmt.Errorf("invalid path %q: missing version", requestPath)
- }
- if version[0] != 'v' {
- version = "v" + version
- }
- return modulePath, version, suffix, nil
-}
-
func ParseRequiredIntParam(r *http.Request, name string) (int, error) {
value := r.FormValue(name)
if value == "" {
@@ -204,3 +179,107 @@
}
return lines, nil
}
+
+// A ModuleURLPath holds the components of a URL path parsed
+// as module, version and suffix.
+type ModuleURLPath struct {
+ Module string
+ Version string
+ Suffix string
+}
+
+// ParseModuleURLPath parse the module path, version and suffix described by
+// the argument, which is expected to be a URL path.
+// The module and version should have one of the following three forms:
+// - <module>/@v/<version>
+// - <module>@<version>
+// - <module>/@latest
+//
+// The suffix is the part of the path after the version.
+func ParseModuleURLPath(requestPath string) (_ ModuleURLPath, err error) {
+ defer derrors.Wrap(&err, "ParseModuleURLPath(%q)", requestPath)
+
+ p := strings.TrimPrefix(requestPath, "/")
+ modulePath, versionAndSuffix, found := strings.Cut(p, "@")
+ if !found {
+ return ModuleURLPath{}, fmt.Errorf("invalid path %q: missing '@'", requestPath)
+ }
+ modulePath = strings.TrimSuffix(modulePath, "/")
+ if modulePath == "" {
+ return ModuleURLPath{}, fmt.Errorf("invalid path %q: missing module", requestPath)
+ }
+ if strings.HasPrefix(versionAndSuffix, "v/") {
+ versionAndSuffix = versionAndSuffix[2:]
+ }
+ // Now versionAndSuffix begins with a version.
+ version, suffix, _ := strings.Cut(versionAndSuffix, "/")
+ if version == "" {
+ return ModuleURLPath{}, fmt.Errorf("invalid path %q: missing version", requestPath)
+ }
+ if version[0] != 'v' {
+ version = "v" + version
+ }
+ return ModuleURLPath{modulePath, version, suffix}, nil
+}
+
+// Path reconstructs a URL path from m.
+func (m ModuleURLPath) Path() string {
+ p := m.Module + "@" + m.Version
+ if m.Suffix != "" {
+ p += "/" + m.Suffix
+ }
+ return p
+}
+
+// ParseParams populates the fields of pstruct, which must a pointer to a struct,
+// with the form and query parameters of r.
+//
+// The fields of pstruct must be exported, and each field must be a string, an
+// int or a bool. If there is a request parameter corresponding to the
+// lower-cased field name, it is parsed according to the field's type and
+// assigned to the field. If there is no matching parameter (or it is the empty
+// string), the field is not assigned.
+//
+// For default values or to detect missing parameters, set the struct field
+// before calling ParseParams; if there is no matching parameter, the field will
+// retain its value.
+func ParseParams(r *http.Request, pstruct any) (err error) {
+ defer derrors.Wrap(&err, "ParseParams(%q)", r.URL)
+
+ v := reflect.ValueOf(pstruct)
+ t := v.Type()
+ if t.Kind() != reflect.Pointer || t.Elem().Kind() != reflect.Struct {
+ return fmt.Errorf("need struct pointer, got %T", pstruct)
+ }
+ t = t.Elem()
+ v = v.Elem()
+
+ for i := 0; i < t.NumField(); i++ {
+ f := t.Field(i)
+ paramName := strings.ToLower(f.Name)
+ param := r.FormValue(paramName)
+ if param == "" {
+ // If param is missing, do not set field.
+ continue
+ }
+ pval, err := parseParam(param, f.Type.Kind())
+ if err != nil {
+ return fmt.Errorf("param %s: %v", paramName, err)
+ }
+ v.Field(i).Set(reflect.ValueOf(pval))
+ }
+ return nil
+}
+
+func parseParam(param string, kind reflect.Kind) (any, error) {
+ switch kind {
+ case reflect.String:
+ return param, nil
+ case reflect.Int:
+ return strconv.Atoi(param)
+ case reflect.Bool:
+ return strconv.ParseBool(param)
+ default:
+ return nil, fmt.Errorf("cannot parse kind %s", kind)
+ }
+}
diff --git a/internal/scan/parse_test.go b/internal/scan/parse_test.go
index 5397e8f..19f86d2 100644
--- a/internal/scan/parse_test.go
+++ b/internal/scan/parse_test.go
@@ -7,6 +7,8 @@
import (
"net/http"
"net/url"
+ "reflect"
+ "strings"
"testing"
"github.com/google/go-cmp/cmp"
@@ -23,41 +25,32 @@
name: "ValidScanURL",
url: "https://worker.com/scan/module/@v/v1.0.0?importedby=50",
want: Request{
- Module: "module",
- Version: "v1.0.0",
- ImportedBy: 50,
- Mode: "",
+ ModuleURLPath{Module: "module", Version: "v1.0.0"},
+ RequestParams{ImportedBy: 50, Mode: ""},
},
},
{
name: "ValidImportsOnlyScanURL",
url: "https://worker.com/scan/module/@v/v1.0.0-abcdefgh?importedby=100&mode=mode1",
want: Request{
- Module: "module",
- Version: "v1.0.0-abcdefgh",
- ImportedBy: 100,
- Mode: "mode1",
+ ModuleURLPath{Module: "module", Version: "v1.0.0-abcdefgh"},
+ RequestParams{ImportedBy: 100, Mode: "mode1"},
},
},
{
name: "Module@Version",
url: "https://worker.com/scan/module@v1.2.3?importedby=1",
want: Request{
- Module: "module",
- Version: "v1.2.3",
- ImportedBy: 1,
- Mode: "",
+ ModuleURLPath{Module: "module", Version: "v1.2.3"},
+ RequestParams{ImportedBy: 1, Mode: ""},
},
},
{
name: "Module@Version suffix",
url: "https://worker.com/scan/module@v1.2.3/path/to/dir?importedby=1",
want: Request{
- Module: "module",
- Version: "v1.2.3",
- Suffix: "path/to/dir",
- ImportedBy: 1,
- Mode: "",
+ ModuleURLPath{Module: "module", Version: "v1.2.3", Suffix: "path/to/dir"},
+ RequestParams{ImportedBy: 1, Mode: ""},
},
},
} {
@@ -112,12 +105,12 @@
{
name: "MissingImportedBy",
url: "/module/@v/v1.0.0",
- want: `missing query param "importedby"`,
+ want: `missing or negative "importedby" query param`,
},
{
name: "BadImportedBy",
url: "/module@v1?importedby=1a",
- want: `want integer for "importedby" query param, got "1a"`,
+ want: `param importedby: strconv.Atoi: parsing "1a": invalid syntax`,
},
} {
t.Run(test.name, func(t *testing.T) {
@@ -127,7 +120,7 @@
}
r := &http.Request{URL: u}
if _, err := ParseRequest(r, "/scan"); err != nil {
- if got := err.Error(); got != test.want {
+ if got := err.Error(); !strings.Contains(got, test.want) {
t.Fatalf("\ngot %s\nwant %s", got, test.want)
}
} else {
@@ -162,3 +155,68 @@
t.Errorf("\n got %v\nwant %v", got, want)
}
}
+
+func TestParseParams(t *testing.T) {
+ type S struct {
+ Str string
+ Int int
+ Bool bool
+ }
+
+ t.Run("success", func(t *testing.T) {
+ for _, test := range []struct {
+ params string
+ want S
+ }{
+ {
+ "str=foo&int=1&bool=true",
+ S{Str: "foo", Int: 1, Bool: true},
+ },
+ {
+ "", // all defaults
+ S{Str: "d", Int: 17, Bool: false},
+ },
+ {
+ "int=3&bool=t&str=", // empty string is same as default
+ S{Str: "d", Int: 3, Bool: true},
+ },
+ } {
+ r, err := http.NewRequest("GET", "https://path?"+test.params, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ got := S{Str: "d", Int: 17} // set defaults
+ if err := ParseParams(r, &got); err != nil {
+ t.Fatal(err)
+ }
+ if !reflect.DeepEqual(got, test.want) {
+ t.Errorf("%q: \ngot %+v\nwant %+v", test.params, got, test.want)
+ }
+ }
+ })
+ t.Run("errors", func(t *testing.T) {
+ for _, test := range []struct {
+ arg any
+ params string
+ errContains string
+ }{
+ {3, "", "struct pointer"},
+ {&S{}, "int=foo", "invalid syntax"},
+ {&S{}, "bool=foo", "invalid syntax"},
+ {&struct{ F float64 }{}, "f=1.1", "cannot parse kind"},
+ } {
+ r, err := http.NewRequest("GET", "https://path?"+test.params, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ err = ParseParams(r, test.arg)
+ got := "<nil>"
+ if err != nil {
+ got = err.Error()
+ }
+ if !strings.Contains(got, test.errContains) {
+ t.Errorf("%v, %q: got %q, want string containing %q", test.arg, test.params, got, test.errContains)
+ }
+ }
+ })
+}
diff --git a/internal/worker/enqueue.go b/internal/worker/enqueue.go
index 20f6867..d80f09f 100644
--- a/internal/worker/enqueue.go
+++ b/internal/worker/enqueue.go
@@ -79,10 +79,8 @@
var sreqs []*scan.Request
for _, ms := range modspecs {
sreqs = append(sreqs, &scan.Request{
- Module: ms.Path,
- Version: ms.Version,
- ImportedBy: ms.ImportedBy,
- Mode: mode,
+ ModuleURLPath: scan.ModuleURLPath{Module: ms.Path, Version: ms.Version},
+ RequestParams: scan.RequestParams{ImportedBy: ms.ImportedBy, Mode: mode},
})
}
return sreqs
diff --git a/internal/worker/vulncheck_enqueue.go b/internal/worker/vulncheck_enqueue.go
index bf93a8f..778b252 100644
--- a/internal/worker/vulncheck_enqueue.go
+++ b/internal/worker/vulncheck_enqueue.go
@@ -121,15 +121,13 @@
if err != nil {
return nil, err
}
- mod, vers, suff, err := scan.ParseModuleVersionSuffix(strings.TrimPrefix(attrs.Name, binaryDir+"/"))
+ mp, err := scan.ParseModuleURLPath(strings.TrimPrefix(attrs.Name, binaryDir+"/"))
if err != nil {
return nil, err
}
sreqs = append(sreqs, &scan.Request{
- Module: mod,
- Version: vers,
- Suffix: suff,
- Mode: ModeBinary,
+ ModuleURLPath: mp,
+ RequestParams: scan.RequestParams{Mode: ModeBinary},
})
}
return sreqs, nil
diff --git a/internal/worker/vulncheck_enqueue_test.go b/internal/worker/vulncheck_enqueue_test.go
index 83d2121..f178601 100644
--- a/internal/worker/vulncheck_enqueue_test.go
+++ b/internal/worker/vulncheck_enqueue_test.go
@@ -23,10 +23,12 @@
t.Fatal(err)
}
want := &scan.Request{
- Module: "golang.org/x/pkgsite",
- Version: "v0.0.0-20221004150836-873fb37c2479",
- Suffix: "cmd/worker",
- Mode: ModeBinary,
+ ModuleURLPath: scan.ModuleURLPath{
+ Module: "golang.org/x/pkgsite",
+ Version: "v0.0.0-20221004150836-873fb37c2479",
+ Suffix: "cmd/worker",
+ },
+ RequestParams: scan.RequestParams{Mode: ModeBinary},
}
found := false
for _, sr := range sreqs {
diff --git a/internal/worker/vulncheck_scan_test.go b/internal/worker/vulncheck_scan_test.go
index 686b9f1..ccd488d 100644
--- a/internal/worker/vulncheck_scan_test.go
+++ b/internal/worker/vulncheck_scan_test.go
@@ -34,7 +34,7 @@
}
func TestRunScanModule(t *testing.T) {
- t.Skip()
+ t.Skip("breaks on trybots")
ctx := context.Background()
cfg, err := config.Init(ctx)
