Skip to content

Commit

Permalink
fix: ensure that npm dependencies retain their "production" grouping (#…
Browse files Browse the repository at this point in the history 
…939)

This resolves an inconsistency in the scanner output for npm packages
that appear in the same tree multiple times but in different groups;
this happens because the table outputter deduplicates on groups on the
assumption that a package can only appear in a single group which is
incorrect for the NPM ecosystem.

To address this, I've introduced an internal map type that ensures
groups are merged when a package is added, with the twist that if either
instance of a package being merged is in no groups then that is the
result of the merge because implicitly that means an instance of the
package is in the "production" group which takes priority over the other
groups.

This is something that should most likely be improved on the future, but
right now this fix should be good enough to ship since afaik it doesn't
impact any other ecosystem and groups are something of a PoC given we
can't resolve that information richly enough across all ecosystems yet.

I think this technically could impact `pnpm` as well, but none of our
fixtures gave a good indicator and the latest lockfile version doesn't
have that information anymore so frankly I'm not worrying about it at
this point.

Resolves #924
  • Loading branch information
@G-Rath
G-Rath authored Jul 10, 2024
1 parent e35bd05 commit 874dcdd
Show file tree
Hide file tree
Showing 6 changed files with 266 additions and 7 deletions.
81 changes: 81 additions & 0 deletions pkg/lockfile/fixtures/npm/same-package-different-groups.v1.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
{
"requires": true,
"lockfileVersion": 1,
"dependencies": {
"ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"dev": true,
"requires": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
}
},
"eslint": {
"version": "1.2.3",
"dev": true,
"dependencies": {
"ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"dev": true,
"requires": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
},
"dependencies": {
"ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"optional": true,
"requires": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
}
}
}
}
}
},
"table": {
"version": "1.0.0",
"dependencies": {
"ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"requires": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
},
"dependencies": {
"ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"dev": true,
"optional": true,
"requires": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
}
}
}
}
}
}
}
}
78 changes: 78 additions & 0 deletions pkg/lockfile/fixtures/npm/same-package-different-groups.v2.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
{
"name": "my-library",
"lockfileVersion": 2,
"requires": true,
"packages": {
"": {
"dependencies": {},
"devDependencies": {}
},
"node_modules/ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"dev": true,
"dependencies": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
}
},
"node_modules/eslint": {
"version": "1.2.3",
"dev": true
},
"node_modules/eslint/node_modules/ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"dev": true,
"dependencies": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
}
},
"node_modules/eslint/node_modules/ajv/node_modules/ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"optional": true,
"dependencies": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
}
},
"node_modules/table": {
"version": "1.0.0"
},
"node_modules/table/node_modules/ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"dependencies": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
}
},
"node_modules/table/node_modules/ajv/node_modules/ajv": {
"version": "5.5.2",
"resolved": "https://registry.npmjs.org/ajv/-/ajv-5.5.2.tgz",
"integrity": "sha512-Ajr4IcMXq/2QmMkEmSvxqfLN5zGmJ92gHXAeOXq1OekoH2rfDNsgdDoL2f7QaRCy7G/E6TpxBVdRuNraMztGHw==",
"devOptional": true,
"dependencies": {
"co": "^4.6.0",
"fast-deep-equal": "^1.0.0",
"fast-json-stable-stringify": "^2.0.0",
"json-schema-traverse": "^0.3.0"
}
}
},
"dependencies": {}
}
32 changes: 32 additions & 0 deletions pkg/lockfile/parse-npm-lock-v1_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,3 +415,35 @@ func TestParseNpmLock_v1_OptionalPackage(t *testing.T) {
},
})
}

func TestParseNpmLock_v1_SamePackageDifferentGroups(t *testing.T) {
t.Parallel()

packages, err := lockfile.ParseNpmLock("fixtures/npm/same-package-different-groups.v1.json")

if err != nil {
t.Errorf("Got unexpected error: %v", err)
}

expectPackages(t, packages, []lockfile.PackageDetails{
{
Name: "eslint",
Version: "1.2.3",
Ecosystem: lockfile.NpmEcosystem,
CompareAs: lockfile.NpmEcosystem,
DepGroups: []string{"dev"},
},
{
Name: "table",
Version: "1.0.0",
Ecosystem: lockfile.NpmEcosystem,
CompareAs: lockfile.NpmEcosystem,
},
{
Name: "ajv",
Version: "5.5.2",
Ecosystem: lockfile.NpmEcosystem,
CompareAs: lockfile.NpmEcosystem,
},
})
}
32 changes: 32 additions & 0 deletions pkg/lockfile/parse-npm-lock-v2_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -409,3 +409,35 @@ func TestParseNpmLock_v2_OptionalPackage(t *testing.T) {
},
})
}

func TestParseNpmLock_v2_SamePackageDifferentGroups(t *testing.T) {
t.Parallel()

packages, err := lockfile.ParseNpmLock("fixtures/npm/same-package-different-groups.v2.json")

if err != nil {
t.Errorf("Got unexpected error: %v", err)
}

expectPackages(t, packages, []lockfile.PackageDetails{
{
Name: "eslint",
Version: "1.2.3",
Ecosystem: lockfile.NpmEcosystem,
CompareAs: lockfile.NpmEcosystem,
DepGroups: []string{"dev"},
},
{
Name: "table",
Version: "1.0.0",
Ecosystem: lockfile.NpmEcosystem,
CompareAs: lockfile.NpmEcosystem,
},
{
Name: "ajv",
Version: "5.5.2",
Ecosystem: lockfile.NpmEcosystem,
CompareAs: lockfile.NpmEcosystem,
},
})
}
46 changes: 40 additions & 6 deletions pkg/lockfile/parse-npm-lock.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strings"

"golang.org/x/exp/maps"
"golang.org/x/exp/slices"
)

type NpmLockDependency struct {
Expand Down Expand Up @@ -49,6 +50,39 @@ type NpmLockfile struct {

const NpmEcosystem Ecosystem = "npm"

type npmPackageDetailsMap map[string]PackageDetails

// mergeNpmDepsGroups handles merging the dependency groups of packages within the
// NPM ecosystem, since they can appear multiple times in the same dependency tree
//
// the merge happens almost as you'd expect, except that if either given packages
// belong to no groups, then that is the result since it indicates the package
// is implicitly a production dependency.
func mergeNpmDepsGroups(a, b PackageDetails) []string {
// if either group includes no groups, then the package is in the "production" group
if len(a.DepGroups) == 0 || len(b.DepGroups) == 0 {
return nil
}

combined := make([]string, 0, len(a.DepGroups)+len(b.DepGroups))
combined = append(combined, a.DepGroups...)
combined = append(combined, b.DepGroups...)

slices.Sort(combined)

return slices.Compact(combined)
}

func (pdm npmPackageDetailsMap) add(key string, details PackageDetails) {
existing, ok := pdm[key]

if ok {
details.DepGroups = mergeNpmDepsGroups(existing, details)
}

pdm[key] = details
}

func (dep NpmLockDependency) depGroups() []string {
if dep.Dev && dep.Optional {
return []string{"dev", "optional"}
Expand All @@ -64,7 +98,7 @@ func (dep NpmLockDependency) depGroups() []string {
}

func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[string]PackageDetails {
details := map[string]PackageDetails{}
details := npmPackageDetailsMap{}

for name, detail := range dependencies {
if detail.Dependencies != nil {
Expand Down Expand Up @@ -98,14 +132,14 @@ func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[str
}
}

details[name+"@"+version] = PackageDetails{
details.add(name+"@"+version, PackageDetails{
Name: name,
Version: finalVersion,
Ecosystem: NpmEcosystem,
CompareAs: NpmEcosystem,
Commit: commit,
DepGroups: detail.depGroups(),
}
})
}

return details
Expand Down Expand Up @@ -137,7 +171,7 @@ func (pkg NpmLockPackage) depGroups() []string {
}

func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]PackageDetails {
details := map[string]PackageDetails{}
details := npmPackageDetailsMap{}

for namePath, detail := range packages {
if namePath == "" {
Expand All @@ -159,14 +193,14 @@ func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]Package
finalVersion = commit
}

details[finalName+"@"+finalVersion] = PackageDetails{
details.add(finalName+"@"+finalVersion, PackageDetails{
Name: finalName,
Version: detail.Version,
Ecosystem: NpmEcosystem,
CompareAs: NpmEcosystem,
Commit: commit,
DepGroups: detail.depGroups(),
}
})
}

return details
Expand Down
4 changes: 3 additions & 1 deletion pkg/lockfile/parse-nuget-lock.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ func parseNuGetLock(lockfile NuGetLockfile) ([]PackageDetails, error) {
// its dependencies, there might be different or duplicate dependencies
// between frameworks
for _, dependencies := range lockfile.Dependencies {
maps.Copy(details, parseNuGetLockDependencies(dependencies))
for name, detail := range parseNuGetLockDependencies(dependencies) {
details[name] = detail
}
}

return maps.Values(details), nil
Expand Down

0 comments on commit 874dcdd

Please sign in to comment.