Work around NPM reporting for transitive dependencies with deduplicat…

…ion (#258)

Fixes #257.

analyzers/nodejs/nodejs.go

@@ -5,7 +5,7 @@
 //
 // A `BuildTarget` for Node.js is defined as the relative path to the directory
 // containing the `package.json`, and the `Dir` is defined as the CWD for
-// running tools.
+// running build tools (like `npm` or `yarn`).
 //
 // `npm` and `yarn` are explicitly supported as first-class tools. Where
 // possible, these tools are queried before falling back to other strategies.
@@ -17,15 +17,14 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/apex/log"
 	"github.com/mitchellh/mapstructure"
 	"github.com/pkg/errors"
 
 	"github.com/fossas/fossa-cli/buildtools/npm"
-	"github.com/fossas/fossa-cli/graph"
-
-	"github.com/apex/log"
 	"github.com/fossas/fossa-cli/exec"
 	"github.com/fossas/fossa-cli/files"
+	"github.com/fossas/fossa-cli/graph"
 	"github.com/fossas/fossa-cli/module"
 	"github.com/fossas/fossa-cli/pkg"
 )
@@ -151,6 +150,7 @@ func Discover(dir string, options map[string]interface{}) ([]module.Module, erro
 	return modules, nil
 }
 
+// Clean removes `node_modules`.
 func (a *Analyzer) Clean() error {
 	return files.Rm(a.Module.Dir, "node_modules")
 }
@@ -161,7 +161,7 @@ func (a *Analyzer) Clean() error {
 func (a *Analyzer) Build() error {
 	log.Debugf("Running Node.js build: %#v", a.Module)
 
-	// Prefer Yarn where possible
+	// Prefer Yarn where possible.
 	if ok, err := files.Exists(a.Module.Dir, "yarn.lock"); err == nil && ok && a.YarnCmd != "" {
 		_, _, err := exec.Run(exec.Cmd{
 			Name: a.YarnCmd,
@@ -223,6 +223,8 @@ func (a *Analyzer) Analyze() (graph.Deps, error) {
 		log.Warnf("NPM had non-zero exit code: %s", err.Error())
 	}
 
+	// TODO: we should move this functionality in to the buildtool, and have it
+	// return `pkg.Package`s.
 	// Set direct dependencies.
 	var imports []pkg.Import
 	for name, dep := range pkgs.Dependencies {
@@ -260,12 +262,10 @@ func recurseDeps(pkgMap map[pkg.ID]pkg.Package, p npm.Output) {
 			Revision: dep.Version,
 			Location: dep.Resolved,
 		}
-		// Don't process duplicates.
-		_, ok := pkgMap[id]
-		if ok {
-			continue
-		}
-		// Get direct imports.
+		// Handle previously seen (usually deduplicated) entries: see #257.
+		previous := pkgMap[id]
+
+		// Set direct imports.
 		var imports []pkg.Import
 		for name, i := range p.Dependencies {
 			imports = append(imports, pkg.Import{
@@ -279,9 +279,12 @@ func recurseDeps(pkgMap map[pkg.ID]pkg.Package, p npm.Output) {
 			})
 		}
 		// Update map.
+		// NOTE: We're assuming that each deduplicated dependency's imports will
+		// only be listed once. This assumption might not be true. If it's not, then
+		// we need to do a set union instead of a list concatenation.
 		pkgMap[id] = pkg.Package{
 			ID:      id,
-			Imports: imports,
+			Imports: append(imports, previous.Imports...),
 		}
 		// Recurse in imports.
 		recurseDeps(pkgMap, dep)

analyzers/nodejs/nodejs_test.go

@@ -1,18 +1,87 @@
 package nodejs_test
 
 import (
+	"log"
 	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 
+	"github.com/fossas/fossa-cli/analyzers"
 	"github.com/fossas/fossa-cli/analyzers/nodejs"
 	"github.com/fossas/fossa-cli/module"
 	"github.com/fossas/fossa-cli/pkg"
 )
 
+// TestNoDependencies checks that there is no error even when `package.json` is
+// missing a `dependencies` key or has an empty object as the value for
+// `dependencies`.
 func TestNoDependencies(t *testing.T) {
-	t.Skip("unimplemented")
+	m := module.Module{
+		Dir:  filepath.Join("testdata", "empty"),
+		Type: pkg.NodeJS,
+	}
+
+	a, err := analyzers.New(m)
+	assert.NoError(t, err)
+
+	a.(*nodejs.Analyzer).Tool = MockNPM{
+		JSONFilename: filepath.Join("testdata", "empty", "npm-ls-json.json"),
+	}
+
+	deps, err := a.Analyze()
+	assert.NoError(t, err)
+	assert.Empty(t, deps.Direct)
+	assert.Empty(t, deps.Transitive)
+}
+
+// TestDuplicateDependencies checks that analysis correctly handles duplicate
+// dependencies, even in the case where duplicates may not have the same set of
+// imports listed.
+//
+// For example, try running `npm ls --json` in the `testdata/duplicates` folder.
+// Notice that `babel-runtime` is included as a dependency twice: once by
+// `babel-polyfill` and once by `jira-client`. However, the _dependencies_ of
+// `babel-runtime` are only listed _once,_ when it's imported by
+// `babel-polyfill`. This means that we must ensure that we get transitive
+// dependencies from the original dependency entry, not the deduplicated entry.
+//
+// See #257 for details.
+func TestDuplicateDependencies(t *testing.T) {
+	m := module.Module{
+		BuildTarget: filepath.Join("testdata", "duplicates", "package.json"),
+		Dir:         filepath.Join("testdata", "duplicates"),
+		Type:        pkg.NodeJS,
+	}
+
+	a, err := analyzers.New(m)
+	assert.NoError(t, err)
+
+	a.(*nodejs.Analyzer).Tool = MockNPM{
+		JSONFilename: filepath.Join("testdata", "duplicates", "npm-ls-json.json"),
+	}
+
+	// We run this multiple times because this bug may flake; map traversal order
+	// is random in Go.
+	var failed pkg.Deps
+	for i := 0; i < 10; i++ {
+		deps, err := a.Analyze()
+		assert.NoError(t, err)
+		id := pkg.ID{
+			Type:     pkg.NodeJS,
+			Name:     "regenerator-runtime",
+			Revision: "0.11.1",
+			Location: "https://registry.npmjs.org/regenerator-runtime/-/regenerator-runtime-0.11.1.tgz",
+		}
+		ok := assert.Contains(t, deps.Transitive, id)
+		if !ok {
+			failed = deps.Transitive
+		}
+	}
+
+	if t.Failed() {
+		log.Printf("%#v", failed)
+	}
 }
 
 func TestAnalyzeWithNpmLs(t *testing.T) {