Merge pull request #45 from senseyeio/44

Improving NPM robustness to various version strings
senseyeio · Jun 4, 2018 · 8ba3299 · 8ba3299
2 parents 9ad7fe9 + 7489916
commit 8ba3299
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 60 deletions.
diff --git a/dep.go b/dep.go
@@ -30,3 +30,9 @@ type DepsByName []Dep
 func (d DepsByName) Len() int           { return len(d) }
 func (d DepsByName) Swap(i, j int)      { d[i], d[j] = d[j], d[i] }
 func (d DepsByName) Less(i, j int) bool { return d[i].Name < d[j].Name }
+
+type Warnings []Warning
+
+func (d Warnings) Len() int           { return len(d) }
+func (d Warnings) Swap(i, j int)      { d[i], d[j] = d[j], d[i] }
+func (d Warnings) Less(i, j int) bool { return d[i].Warning() < d[j].Warning() }
diff --git a/npm/npm.go b/npm/npm.go
@@ -124,18 +124,6 @@ func getNPMLicenseFromURL(pkgName, url string) (diligent.Dep, error) {
 }
 
 func (n *npmDeper) getNPMLicense(pkgName, version string) (diligent.Dep, error) {
-	npmURL := fmt.Sprintf("%s/%s/%s", n.url, strings.Replace(url.QueryEscape(pkgName), "%40", "@", 1), url.QueryEscape(version))
-	dep, err := getNPMLicenseFromURL(pkgName, npmURL)
-	if err == nil {
-		return dep, err
-	}
-	// it seems for scoped packages (e.g. @angular/router) URLs with exact versions
-	// like https://registry.npmjs.org/@angular%2Fupgrade/4.4.5 don't work
-	// but https://registry.npmjs.org/@angular%2Fupgrade/=4.4.5 do
-	// lets try that, if it succeeds great, if not, return the original results
-	npmURL = fmt.Sprintf("%s/%s/=%s", n.url, strings.Replace(url.QueryEscape(pkgName), "%40", "@", 1), url.QueryEscape(version))
-	if dep2, err2 := getNPMLicenseFromURL(pkgName, npmURL); err2 == nil {
-		return dep2, err2
-	}
-	return dep, err
+	npmURL := fmt.Sprintf("%s/%s?version=%s", n.url, strings.Replace(url.QueryEscape(pkgName), "%40", "@", 1), url.QueryEscape(version))
+	return getNPMLicenseFromURL(pkgName, npmURL)
 }
diff --git a/npm/npm_test.go b/npm/npm_test.go
@@ -10,6 +10,8 @@ import (
 
 	"sort"
 
+	"net/url"
+
 	"github.com/senseyeio/diligent"
 	"github.com/senseyeio/diligent/npm"
 	"github.com/senseyeio/diligent/warning"
@@ -48,6 +50,9 @@ func TestIsCompatible(t *testing.T) {
 }
 
 func TestDependencies(t *testing.T) {
+	pathAndQuery := func(url *url.URL) string {
+		return url.Path + "?" + url.RawQuery
+	}
 	cases := []struct {
 		description string
 		config      npm.Config
@@ -73,7 +78,7 @@ func TestDependencies(t *testing.T) {
 			if r.Method != "GET" {
 				t.Errorf("expected GET got %s", r.Method)
 			}
-			if r.URL.Path != "/d3/5.0.0" {
+			if pathAndQuery(r.URL) != "/d3?version=5.0.0" {
 				t.Errorf("unexpected path %s", r.URL.Path)
 			}
 			w.WriteHeader(http.StatusOK)
@@ -90,21 +95,21 @@ func TestDependencies(t *testing.T) {
 		[]byte(`
 			{
 				"dependencies": {
-					"d3": "5.0.0",
+					"d3": "^5.0.0",
 					"cypress": "2.1.0"
 				}
 			}
 		`),
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			switch r.URL.Path {
-			case "/d3/5.0.0":
+			switch pathAndQuery(r.URL) {
+			case "/d3?version=%5E5.0.0":
 				w.WriteHeader(http.StatusOK)
 				w.Write([]byte("{\"license\":\"GPL-3.0\"}"))
-			case "/cypress/2.1.0":
+			case "/cypress?version=2.1.0":
 				w.WriteHeader(http.StatusOK)
 				w.Write([]byte("{\"license\":\"MIT\"}"))
 			default:
-				t.Errorf("unexpected path %s", r.URL.Path)
+				t.Errorf("unexpected path %s", pathAndQuery(r.URL))
 			}
 		}),
 		map[string]string{
@@ -119,21 +124,21 @@ func TestDependencies(t *testing.T) {
 		[]byte(`
 			{
 				"dependencies": {
-					"d3": "5.0.0",
+					"d3": "~5.0.0",
 					"cypress": "2.1.0"
 				}
 			}
 		`),
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			switch r.URL.Path {
-			case "/d3/5.0.0":
+			switch pathAndQuery(r.URL) {
+			case "/d3?version=~5.0.0":
 				w.WriteHeader(http.StatusOK)
 				w.Write([]byte("{\"license\":\"GPL-3.0\"}"))
-			case "/cypress/2.1.0", "/cypress/=2.1.0":
+			case "/cypress?version=2.1.0":
 				w.WriteHeader(http.StatusInternalServerError)
 				w.Write([]byte("{\"error\":\"failed\"}"))
 			default:
-				t.Errorf("unexpected path %s", r.URL.Path)
+				t.Errorf("unexpected path %s", pathAndQuery(r.URL))
 			}
 		}),
 		map[string]string{
@@ -155,49 +160,22 @@ func TestDependencies(t *testing.T) {
 			}
 		`),
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			switch r.URL.Path {
-			case "/d3/5.0.0", "/d3/=5.0.0":
+			switch pathAndQuery(r.URL) {
+			case "/d3?version=5.0.0":
 				w.WriteHeader(http.StatusInternalServerError)
 				w.Write([]byte("{\"error\":\"failed\"}"))
-			case "/cypress/2.1.0", "/cypress/=2.1.0":
+			case "/cypress?version=2.1.0":
 				w.WriteHeader(http.StatusInternalServerError)
 				w.Write([]byte("{\"error\":\"failed\"}"))
 			default:
-				t.Errorf("unexpected path %s", r.URL.Path)
+				t.Errorf("unexpected path %s", pathAndQuery(r.URL))
 			}
 		}),
 		map[string]string{},
 		[]diligent.Warning{
 			warning.New("d3", "requested failed with status 500"),
 			warning.New("cypress", "requested failed with status 500")},
 		false,
-	}, {
-		"should fall back to using '=version' if 'version' fails",
-		npm.Config{},
-		[]byte(`
-			{
-				"dependencies": {
-					"@angular/router": "1.2.3"
-				}
-			}
-		`),
-		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			switch r.URL.Path {
-			case "/@angular/router/1.2.3":
-				w.WriteHeader(http.StatusInternalServerError)
-				w.Write([]byte("{\"error\":\"failed\"}"))
-			case "/@angular/router/=1.2.3":
-				w.WriteHeader(http.StatusOK)
-				w.Write([]byte("{\"license\":\"GPL-3.0\"}"))
-			default:
-				t.Errorf("unexpected path %s", r.URL.Path)
-			}
-		}),
-		map[string]string{
-			"@angular/router": "GPL-3.0",
-		},
-		[]diligent.Warning{},
-		false,
 	}, {
 		"should be capable of including devDependencies",
 		npm.Config{DevDependencies: true},
@@ -215,8 +193,8 @@ func TestDependencies(t *testing.T) {
 			if r.Method != "GET" {
 				t.Errorf("expected GET got %s", r.Method)
 			}
-			if r.URL.Path != "/d3/5.0.0" && r.URL.Path != "/cypress/2.1.0" {
-				t.Errorf("unexpected path %s", r.URL.Path)
+			if pathAndQuery(r.URL) != "/d3?version=5.0.0" && pathAndQuery(r.URL) != "/cypress?version=2.1.0" {
+				t.Errorf("unexpected path %s", pathAndQuery(r.URL))
 			}
 			w.WriteHeader(http.StatusOK)
 			w.Write([]byte("{\"license\":\"MIT\"}"))
@@ -312,8 +290,12 @@ func TestDependencies(t *testing.T) {
 					t.Errorf("deps: got %+v, want %+v", d, expectedDeps)
 				}
 			}
-			if (len(w) > 0 || len(tt.warnsOut) > 0) && reflect.DeepEqual(w, tt.warnsOut) == false {
-				t.Errorf("warnings: got %+v, want %+v", w, tt.warnsOut)
+			if len(w) > 0 || len(tt.warnsOut) > 0 {
+				sort.Sort(diligent.Warnings(w))
+				sort.Sort(diligent.Warnings(tt.warnsOut))
+				if reflect.DeepEqual(w, tt.warnsOut) == false {
+					t.Errorf("warnings: got %+v, want %+v", w, tt.warnsOut)
+				}
 			}
 			isErr := e != nil
 			if tt.errOut != isErr {