internal/fetch,internal/proxy: encode module paths in proxy requests

The proxy requires the <module> and <version> elements to be case-encoded
in requests. The proxy client is updated to encode these elements using
internal/thirdparty/module.EncodePath.

Change-Id: Ie5d6b6ab4156efa8a4d6ac36dbe9b0ef743f3d9a
Reviewed-on: https://team-review.git.corp.google.com/c/golang/discovery/+/439140
Reviewed-by: Andrew Bonventre <andybons@google.com>

cmd/fetch/main.go

@@ -45,7 +45,6 @@ func makeFetchHandler(proxyClient *proxy.Client, db *postgres.DB) http.HandlerFu
 			return
 		}
 
-		log.Printf("Request received: %q", r.URL.Path)
 		module, version, err := fetch.ParseModulePathAndVersion(r.URL.Path)
 		if err != nil {
 			http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)

internal/cron/cron_test.go

@@ -52,15 +52,15 @@ func TestGetVersionsFromIndex(t *testing.T) {
 			name: "valid_get_versions",
 			indexInfo: []map[string]string{
 				map[string]string{
-					"name":    "my/module",
+					"name":    "my.mod/module",
 					"version": "v1.0.0",
 				},
 				map[string]string{
-					"name":    "my/module",
+					"name":    "my.mod/module",
 					"version": "v1.1.0",
 				},
 				map[string]string{
-					"name":    "my/module/v2",
+					"name":    "my.mod/module/v2",
 					"version": "v2.0.0",
 				},
 			},
@@ -113,14 +113,14 @@ func TestNewVersionFromProxyIndex(t *testing.T) {
 			name: "version-logs-no-existing-entries",
 			indexInfo: []map[string]string{
 				map[string]string{
-					"name":    "my/module",
+					"name":    "my.mod/module",
 					"version": "v1.0.0",
 				},
 			},
 			oldVersionLogs: nil,
 			wantVersionLogs: []*internal.VersionLog{
 				&internal.VersionLog{
-					ModulePath: "my/module",
+					ModulePath: "my.mod/module",
 					Version:    "v1.0.0",
 					Source:     internal.VersionSourceProxyIndex,
 				},
@@ -130,29 +130,29 @@ func TestNewVersionFromProxyIndex(t *testing.T) {
 			name: "version-logs-existing-duplicate-entry",
 			indexInfo: []map[string]string{
 				map[string]string{
-					"name":    "my/module",
+					"name":    "my.mod/module",
 					"version": "v1.0.0",
 				},
 				map[string]string{
-					"name":    "my/module",
+					"name":    "my.mod/module",
 					"version": "v2.0.0",
 				},
 			},
 			oldVersionLogs: []*internal.VersionLog{
 				&internal.VersionLog{
-					ModulePath: "my/module",
+					ModulePath: "my.mod/module",
 					Version:    "v1.0.0",
 					Source:     internal.VersionSourceProxyIndex,
 				},
 			},
 			wantVersionLogs: []*internal.VersionLog{
 				&internal.VersionLog{
-					ModulePath: "my/module",
+					ModulePath: "my.mod/module",
 					Version:    "v1.0.0",
 					Source:     internal.VersionSourceProxyIndex,
 				},
 				&internal.VersionLog{
-					ModulePath: "my/module",
+					ModulePath: "my.mod/module",
 					Version:    "v2.0.0",
 					Source:     internal.VersionSourceProxyIndex,
 				},

internal/fetch/fetch.go

@@ -6,6 +6,7 @@ package fetch
 
 import (
 	"archive/zip"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"go/ast"
@@ -121,7 +122,7 @@ func FetchAndInsertVersion(name, version string, proxyClient *proxy.Client, db *
 
 	packages, err := extractPackagesFromZip(name, version, zipReader)
 	if err != nil && err != errModuleContainsNoPackages {
-		return fmt.Errorf("extractPackagesFromZip(%q, %q, %v): %v", name, version, zipReader, err)
+		return fmt.Errorf("extractPackagesFromZip(%q, %q): %v", name, version, err)
 	}
 
 	v := internal.Version{
@@ -139,7 +140,13 @@ func FetchAndInsertVersion(name, version string, proxyClient *proxy.Client, db *
 		VersionType: versionType,
 	}
 	if err = db.InsertVersion(&v); err != nil {
-		return fmt.Errorf("db.InsertVersion(%+v): %v", v, err)
+		// Encode the struct to a JSON string so that it is more readable in
+		// the error message.
+		b, jsonerr := json.Marshal(v)
+		if jsonerr != nil {
+			return fmt.Errorf("db.InsertVersion(%+v): %v; json.Marshal: %v", v, err, jsonerr)
+		}
+		return fmt.Errorf("db.InsertVersion(%+v): %v", string(b), err)
 	}
 	return nil
 }
@@ -176,7 +183,8 @@ func extractPackagesFromZip(module, version string, r *zip.Reader) ([]*internal.
 
 			b, err := readZipFile(f)
 			if err != nil {
-				return nil, fmt.Errorf("readZipFile(%q): %v", f.Name, err)
+				log.Printf("readZipFile(%q): %v", f.Name, err)
+				continue
 			}
 
 			if _, err := file.Write(b); err != nil {
@@ -223,8 +231,8 @@ func extractPackagesFromZip(module, version string, r *zip.Reader) ([]*internal.
 
 // seriesPathForModule reports the series name for the given module. The series
 // name is the shared base path of a group of major-version variants. For
-// example, my/module, my/module/v2, my/module/v3 are a single series, with the
-// series name my/module.
+// example, my.mod/module, my.mod/module/v2, my.mod/module/v3 are a single series, with the
+// series name my.mod/module.
 func seriesPathForModule(name string) (string, error) {
 	if name == "" {
 		return "", errors.New("module name cannot be empty")
@@ -241,9 +249,9 @@ func seriesPathForModule(name string) (string, error) {
 		// Attempt to convert the portion of suffix following "v" to an
 		// integer. If that portion cannot be converted to an integer, or the
 		// version = 0 or 1, return the full module name. For example:
-		// my/module/v2 has series name my/module
-		// my/module/v1 has series name my/module/v1
-		// my/module/v2x has series name my/module/v2x
+		// my.mod/module/v2 has series name my.mod/module
+		// my.mod/module/v1 has series name my.mod/module/v1
+		// my.mod/module/v2x has series name my.mod/module/v2x
 		version, err := strconv.Atoi(suffix[1:])
 		if err != nil || version < 2 {
 			return name, nil

internal/fetch/fetch_test.go

@@ -28,11 +28,11 @@ func TestFetchAndInsertVersion(t *testing.T) {
 		versionData *internal.Version
 	}{
 		{
-			name:    "my/module",
+			name:    "my.mod/module",
 			version: "v1.0.0",
 			versionData: &internal.Version{
 				Module: &internal.Module{
-					Path: "my/module",
+					Path: "my.mod/module",
 				},
 				Version:    "v1.0.0",
 				CommitTime: time.Date(2019, 1, 30, 0, 0, 0, 0, time.UTC),
@@ -145,7 +145,7 @@ func TestHasFilename(t *testing.T) {
 		want         bool
 	}{
 		{
-			file:         "my/module@v1.0.0/README.md",
+			file:         "my.mod/module@v1.0.0/README.md",
 			expectedFile: "README.md",
 			want:         true,
 		},
@@ -179,8 +179,8 @@ func TestHasFilename(t *testing.T) {
 			want:         false,
 		},
 		{
-			file:         "my/module@v1.0.0/LICENSE",
-			expectedFile: "my/module@v1.0.0/LICENSE",
+			file:         "my.mod/module@v1.0.0/LICENSE",
+			expectedFile: "my.mod/module@v1.0.0/LICENSE",
 			want:         true,
 		},
 	} {
@@ -198,14 +198,14 @@ func TestHasFilename(t *testing.T) {
 
 func TestSeriesPathForModule(t *testing.T) {
 	for input, want := range map[string]string{
-		"module/":          "module",
-		"module/v2/":       "module",
-		"my/module":        "my/module",
-		"my/module/v":      "my/module/v",
-		"my/module/v0":     "my/module/v0",
-		"my/module/v1":     "my/module/v1",
-		"my/module/v23456": "my/module",
-		"v2/":              "v2",
+		"module/":              "module",
+		"module/v2/":           "module",
+		"my.mod/module":        "my.mod/module",
+		"my.mod/module/v":      "my.mod/module/v",
+		"my.mod/module/v0":     "my.mod/module/v0",
+		"my.mod/module/v1":     "my.mod/module/v1",
+		"my.mod/module/v23456": "my.mod/module",
+		"v2/":                  "v2",
 	} {
 		t.Run(input, func(t *testing.T) {
 			got, err := seriesPathForModule(input)
@@ -230,25 +230,25 @@ func TestContainsFile(t *testing.T) {
 		want                bool
 	}{
 		{
-			name:    "my/module",
+			name:    "my.mod/module",
 			version: "v1.0.0",
 			file:    "README",
 			want:    true,
 		},
 		{
-			name:    "my/module",
+			name:    "my.mod/module",
 			version: "v1.0.0",
-			file:    "my/module@v1.0.0/LICENSE",
+			file:    "my.mod/module@v1.0.0/LICENSE",
 			want:    true,
 		},
 		{
-			name:    "empty/module",
+			name:    "emp.ty/module",
 			version: "v1.0.0",
 			file:    "README",
 			want:    false,
 		},
 		{
-			name:    "empty/module",
+			name:    "emp.ty/module",
 			version: "v1.0.0",
 			file:    "LICENSE",
 			want:    false,
@@ -277,13 +277,13 @@ func TestExtractFile(t *testing.T) {
 		err                 error
 	}{
 		{
-			name:    "my/module",
+			name:    "my.mod/module",
 			version: "v1.0.0",
-			file:    "my/module@v1.0.0/README.md",
+			file:    "my.mod/module@v1.0.0/README.md",
 			want:    []byte("README FILE FOR TESTING."),
 		},
 		{
-			name:    "empty/module",
+			name:    "emp.ty/module",
 			version: "v1.0.0",
 			file:    "empty/nonexistent/README.md",
 			err:     errors.New(`zip does not contain "README.md"`),
@@ -324,23 +324,23 @@ func TestExtractPackagesFromZip(t *testing.T) {
 	}{
 		{
 			zip:     "module.zip",
-			name:    "my/module",
+			name:    "my.mod/module",
 			version: "v1.0.0",
 			packages: map[string]*internal.Package{
 				"foo": &internal.Package{
 					Name:     "foo",
-					Path:     "my/module/foo",
+					Path:     "my.mod/module/foo",
 					Synopsis: "package foo",
 				},
 				"bar": &internal.Package{
 					Name:     "bar",
-					Path:     "my/module/bar",
+					Path:     "my.mod/module/bar",
 					Synopsis: "package bar",
 				},
 			},
 		},
 		{
-			name:     "empty/module",
+			name:     "emp.ty/module",
 			version:  "v1.0.0",
 			packages: map[string]*internal.Package{},
 		},

internal/proxy/client.go

@@ -13,6 +13,8 @@ import (
 	"net/http"
 	"strings"
 	"time"
+
+	"golang.org/x/discovery/internal/thirdparty/module"
 )
 
 // A Client is used by the fetch service to communicate with a module
@@ -40,22 +42,30 @@ func New(rawurl string) *Client {
 
 // infoURL constructs a url for a request to
 // $GOPROXY/<module>/@v/list.
-func (c *Client) infoURL(name, version string) string {
-	return fmt.Sprintf("%s/%s/@v/%s.info", c.url, name, version)
+func (c *Client) infoURL(path, version string) (string, error) {
+	encodedPath, encodedVersion, err := encodeModulePathAndVersion(path, version)
+	if err != nil {
+		return "", fmt.Errorf("encodeModulePathAndVersion(%q, %q): %v", path, version, err)
+	}
+	return fmt.Sprintf("%s/%s/@v/%s.info", c.url, encodedPath, encodedVersion), nil
 }
 
 // GetInfo makes a request to $GOPROXY/<module>/@v/<version>.info and
 // transforms that data into a *VersionInfo.
-func (c *Client) GetInfo(name, version string) (*VersionInfo, error) {
-	r, err := http.Get(c.infoURL(name, version))
+func (c *Client) GetInfo(path, version string) (*VersionInfo, error) {
+	u, err := c.infoURL(path, version)
+	if err != nil {
+		return nil, fmt.Errorf("infoURL(%q, %q): %v", path, version, err)
+	}
+
+	r, err := http.Get(u)
 	if err != nil {
 		return nil, err
 	}
 	defer r.Body.Close()
 
 	if r.StatusCode < 200 || r.StatusCode >= 300 {
-		return nil, fmt.Errorf("http.Get(%q) returned response: %d (%q)",
-			c.infoURL(name, version), r.StatusCode, r.Status)
+		return nil, fmt.Errorf("http.Get(%q) returned response: %d (%q)", u, r.StatusCode, r.Status)
 	}
 
 	var v VersionInfo
@@ -66,36 +76,52 @@ func (c *Client) GetInfo(name, version string) (*VersionInfo, error) {
 }
 
 // zipURL constructs a url for a request to $GOPROXY/<module>/@v/<version>.zip.
-func (c *Client) zipURL(name, version string) string {
-	return fmt.Sprintf("%s/%s/@v/%s.zip", c.url, name, version)
+func (c *Client) zipURL(path, version string) (string, error) {
+	encodedPath, encodedVersion, err := encodeModulePathAndVersion(path, version)
+	if err != nil {
+		return "", fmt.Errorf("encodeModulePathAndVersion(%q, %q): %v", path, version, err)
+	}
+	return fmt.Sprintf("%s/%s/@v/%s.zip", c.url, encodedPath, encodedVersion), nil
 }
 
 // GetZip makes a request to $GOPROXY/<module>/@v/<version>.zip and transforms
 // that data into a *zip.Reader.
-func (c *Client) GetZip(name, version string) (*zip.Reader, error) {
-	u := c.zipURL(name, version)
+func (c *Client) GetZip(path, version string) (*zip.Reader, error) {
+	u, err := c.zipURL(path, version)
+	if err != nil {
+		return nil, fmt.Errorf("zipURL(%q, %q): %v", path, version, err)
+	}
+
 	r, err := http.Get(u)
 	if err != nil {
-		return nil, fmt.Errorf("http.Get(%q): %v",
-			c.zipURL(name, version), err)
+		return nil, fmt.Errorf("http.Get(%q): %v", u, err)
 	}
 	defer r.Body.Close()
 
 	if r.StatusCode < 200 || r.StatusCode >= 300 {
-		return nil, fmt.Errorf("http.Get(%q) returned response: %d (%q)",
-			c.zipURL(name, version), r.StatusCode, r.Status)
+		return nil, fmt.Errorf("http.Get(%q) returned response: %d (%q)", u, r.StatusCode, r.Status)
 	}
 
 	body, err := ioutil.ReadAll(r.Body)
 	if err != nil {
-		return nil, fmt.Errorf("http.Get(%q): %v",
-			c.zipURL(name, version), err)
+		return nil, fmt.Errorf("http.Get(%q): %v", u, err)
 	}
 
 	zipReader, err := zip.NewReader(bytes.NewReader(body), int64(len(body)))
 	if err != nil {
-		return nil, fmt.Errorf("http.Get(%q): %v",
-			c.zipURL(name, version), err)
+		return nil, fmt.Errorf("http.Get(%q): %v", u, err)
 	}
 	return zipReader, nil
 }
+
+func encodeModulePathAndVersion(path, version string) (string, string, error) {
+	encodedPath, err := module.EncodePath(path)
+	if err != nil {
+		return "", "", fmt.Errorf("module.EncodePath(%q): %v", path, err)
+	}
+	encodedVersion, err := module.EncodeVersion(version)
+	if err != nil {
+		return "", "", fmt.Errorf("module.EncodeVersion(%q): %v", version, err)
+	}
+	return encodedPath, encodedVersion, nil
+}

internal/proxy/client_test.go

@@ -26,7 +26,7 @@ func TestGetInfo(t *testing.T) {
 	teardownProxy, client := SetupTestProxy(t)
 	defer teardownProxy(t)
 
-	name := "my/module"
+	name := "my.mod/module"
 	version := "v1.0.0"
 	info, err := client.GetInfo(name, version)
 	if err != nil {
@@ -47,7 +47,7 @@ func TestGetInfoVersionDoesNotExist(t *testing.T) {
 	teardownProxy, client := SetupTestProxy(t)
 	defer teardownProxy(t)
 
-	name := "my/module"
+	name := "my.mod/module"
 	version := "v3.0.0"
 	info, _ := client.GetInfo(name, version)
 	if info != nil {
@@ -59,7 +59,7 @@ func TestGetZip(t *testing.T) {
 	teardownProxy, client := SetupTestProxy(t)
 	defer teardownProxy(t)
 
-	name := "my/module"
+	name := "my.mod/module"
 	version := "v1.0.0"
 	zipReader, err := client.GetZip(name, version)
 	if err != nil {
@@ -67,11 +67,11 @@ func TestGetZip(t *testing.T) {
 	}
 
 	expectedFiles := map[string]bool{
-		"my/module@v1.0.0/LICENSE":    true,
-		"my/module@v1.0.0/README.md":  true,
-		"my/module@v1.0.0/go.mod":     true,
-		"my/module@v1.0.0/foo/foo.go": true,
-		"my/module@v1.0.0/bar/bar.go": true,
+		"my.mod/module@v1.0.0/LICENSE":    true,
+		"my.mod/module@v1.0.0/README.md":  true,
+		"my.mod/module@v1.0.0/go.mod":     true,
+		"my.mod/module@v1.0.0/foo/foo.go": true,
+		"my.mod/module@v1.0.0/bar/bar.go": true,
 	}
 	if len(zipReader.File) != len(expectedFiles) {
 		t.Errorf("GetZip(%q, %q) returned number of files: got %d, want %d",
@@ -91,12 +91,51 @@ func TestGetZipNonExist(t *testing.T) {
 	teardownProxy, client := SetupTestProxy(t)
 	defer teardownProxy(t)
 
-	name := "my/nonexistmodule"
+	name := "my.mod/nonexistmodule"
 	version := "v1.0.0"
-	expectedErr := fmt.Sprintf("http.Get(%q) returned response: %d (%q)",
-		client.zipURL(name, version), 404, "404 Not Found")
+	zipURL, err := client.zipURL(name, version)
+	if err != nil {
+		t.Fatalf("client.zipURL(%q, %q): %v", name, version, err)
+	}
 
+	expectedErr := fmt.Sprintf("http.Get(%q) returned response: %d (%q)", zipURL, 404, "404 Not Found")
 	if _, err := client.GetZip(name, version); err.Error() != expectedErr {
 		t.Errorf("GetZip(%q, %q) returned error %v, want %v", name, version, err, expectedErr)
 	}
 }
+
+func TestEncodeModulePathAndVersion(t *testing.T) {
+	for _, tc := range []struct {
+		name, version, wantName, wantVersion string
+		wantErr                              bool
+	}{
+		{
+			name:        "github.com/Azure/go-autorest",
+			version:     "v11.0.0+incompatible",
+			wantName:    "github.com/!azure/go-autorest",
+			wantVersion: "v11.0.0+incompatible",
+			wantErr:     false,
+		},
+		{
+			name:        "github.com/!azure/go-autorest",
+			version:     "v11.0.0+incompatible",
+			wantName:    "github.com/!azure/go-autorest",
+			wantVersion: "v11.0.0+incompatible",
+			wantErr:     true,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			gotName, gotVersion, err := encodeModulePathAndVersion(tc.name, tc.version)
+			if err != nil && !tc.wantErr {
+				t.Fatalf("encodeModulePathAndVersion(%q, %q): %v", tc.name, tc.version, err)
+			}
+			if err != nil && tc.wantErr {
+				return
+			}
+
+			if gotName != tc.wantName || gotVersion != tc.wantVersion {
+				t.Errorf("encodeModulePathAndVersion(%q, %q) = %q, %q, %v; want %q, %q, %v", tc.name, tc.version, gotName, gotVersion, err, tc.wantName, tc.wantVersion, nil)
+			}
+		})
+	}
+}

internal/proxy/test_helper.go

@@ -33,8 +33,8 @@ func SetupTestProxy(t *testing.T) (func(t *testing.T), *Client) {
 	client := New(p.URL)
 
 	for _, v := range [][]string{
-		[]string{"my/module", "v1.0.0"},
-		[]string{"empty/module", "v1.0.0"},
+		[]string{"my.mod/module", "v1.0.0"},
+		[]string{"emp.ty/module", "v1.0.0"},
 		[]string{"rsc.io/quote", "v1.5.2"},
 		[]string{"rsc.io/quote/v2", "v2.0.1"},
 	} {

internal/proxy/testdata/modproxy/main.go

@@ -5,9 +5,9 @@
 // modproxy runs a local module proxy for testing. It implements the Module
 // proxy protocol described at `go help goproxy` by serving files stored at
 // ./proxy. The following modules are supported by this proxy:
-// my/module   v1.0.0
-// my/module   v1.1.0
-// my/module/2 v12.0.0
+// my.mod/module   v1.0.0
+// my.mod/module   v1.1.0
+// my.mod/module/2 v12.0.0
 package main
 
 import (

internal/proxy/testdata/modproxy/modules/emp.ty/module@v1.0.0/go.mod

@@ -0,0 +1,3 @@
+module emp.ty/module
+
+go 1.12

internal/proxy/testdata/modproxy/modules/empty/module@v1.0.0/go.mod

@@ -1,3 +0,0 @@
-module empty/module
-
-go 1.12

internal/proxy/testdata/modproxy/modules/my.mod/module@v1.0.0/foo/foo.go

@@ -7,7 +7,8 @@ package foo
 
 import (
 	"fmt"
-	"my/module/bar"
+
+	"my.mod/module/bar"
 )
 
 // FooBar returns the string "foo bar".

internal/proxy/testdata/modproxy/modules/my.mod/module@v1.0.0/go.mod

@@ -0,0 +1,3 @@
+module my.mod/module
+
+go 1.12

internal/proxy/testdata/modproxy/modules/my/module@v1.0.0/go.mod

@@ -1,3 +0,0 @@
-module my/module
-
-go 1.12

internal/proxy/testdata/modproxy/proxy/emp.ty/module/@v/v1.0.0.mod

@@ -0,0 +1,3 @@
+module emp.ty/module
+
+go 1.12

internal/proxy/testdata/modproxy/proxy/empty/module/@v/v1.0.0.mod

@@ -1,3 +0,0 @@
-module empty/module
-
-go 1.12

internal/proxy/testdata/modproxy/proxy/my.mod/module/@v/v1.0.0.mod

@@ -0,0 +1 @@
+module my.mod/module

internal/proxy/testdata/modproxy/proxy/my.mod/module/@v/v1.1.0.mod

@@ -0,0 +1 @@
+module my.mod/module

internal/proxy/testdata/modproxy/proxy/my.mod/module/@v/v1.1.1.mod

@@ -0,0 +1 @@
+module my.mod/module

internal/proxy/testdata/modproxy/proxy/my/module/@v/v1.0.0.mod

@@ -1 +0,0 @@
-module my/module

internal/proxy/testdata/modproxy/proxy/my/module/@v/v1.1.0.mod

@@ -1 +0,0 @@
-module my/module

internal/proxy/testdata/modproxy/proxy/my/module/@v/v1.1.1.mod

@@ -1 +0,0 @@
-module my/module