From 3c68c3dcc1d702fe44c1981f4ebe889f79e85ec6 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Thu, 18 Apr 2024 13:02:57 +1200 Subject: [PATCH 01/10] fix: consider npm packages in different groups as unique --- .../npm/same-package-different-groups.v1.json | 81 +++++++++++++++++++ .../npm/same-package-different-groups.v2.json | 78 ++++++++++++++++++ pkg/lockfile/parse-npm-lock-v1_test.go | 55 +++++++++++++ pkg/lockfile/parse-npm-lock-v2_test.go | 55 +++++++++++++ pkg/lockfile/parse-npm-lock.go | 18 ++++- 5 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 pkg/lockfile/fixtures/npm/same-package-different-groups.v1.json create mode 100644 pkg/lockfile/fixtures/npm/same-package-different-groups.v2.json diff --git a/pkg/lockfile/fixtures/npm/same-package-different-groups.v1.json b/pkg/lockfile/fixtures/npm/same-package-different-groups.v1.json new file mode 100644 index 0000000000..a31088bd7c --- /dev/null +++ b/pkg/lockfile/fixtures/npm/same-package-different-groups.v1.json @@ -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" + } + } + } + } + } + } + } +} diff --git a/pkg/lockfile/fixtures/npm/same-package-different-groups.v2.json b/pkg/lockfile/fixtures/npm/same-package-different-groups.v2.json new file mode 100644 index 0000000000..44f85f9d13 --- /dev/null +++ b/pkg/lockfile/fixtures/npm/same-package-different-groups.v2.json @@ -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": {} +} diff --git a/pkg/lockfile/parse-npm-lock-v1_test.go b/pkg/lockfile/parse-npm-lock-v1_test.go index 81a55e0832..f217987a1c 100644 --- a/pkg/lockfile/parse-npm-lock-v1_test.go +++ b/pkg/lockfile/parse-npm-lock-v1_test.go @@ -415,3 +415,58 @@ 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, + DepGroups: nil, + }, + { + Name: "ajv", + Version: "5.5.2", + Ecosystem: lockfile.NpmEcosystem, + CompareAs: lockfile.NpmEcosystem, + DepGroups: nil, + }, + { + Name: "ajv", + Version: "5.5.2", + Ecosystem: lockfile.NpmEcosystem, + CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{"dev"}, + }, + { + Name: "ajv", + Version: "5.5.2", + Ecosystem: lockfile.NpmEcosystem, + CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{"optional"}, + }, + { + Name: "ajv", + Version: "5.5.2", + Ecosystem: lockfile.NpmEcosystem, + CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{"dev", "optional"}, + }, + }) +} diff --git a/pkg/lockfile/parse-npm-lock-v2_test.go b/pkg/lockfile/parse-npm-lock-v2_test.go index f9ee0da3e0..352d5491d2 100644 --- a/pkg/lockfile/parse-npm-lock-v2_test.go +++ b/pkg/lockfile/parse-npm-lock-v2_test.go @@ -409,3 +409,58 @@ 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, + DepGroups: nil, + }, + { + Name: "ajv", + Version: "5.5.2", + Ecosystem: lockfile.NpmEcosystem, + CompareAs: lockfile.NpmEcosystem, + DepGroups: nil, + }, + { + Name: "ajv", + Version: "5.5.2", + Ecosystem: lockfile.NpmEcosystem, + CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{"dev"}, + }, + { + Name: "ajv", + Version: "5.5.2", + Ecosystem: lockfile.NpmEcosystem, + CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{"optional"}, + }, + { + Name: "ajv", + Version: "5.5.2", + Ecosystem: lockfile.NpmEcosystem, + CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{"dev", "optional"}, + }, + }) +} diff --git a/pkg/lockfile/parse-npm-lock.go b/pkg/lockfile/parse-npm-lock.go index 0471d0cbfa..08f67aae8b 100644 --- a/pkg/lockfile/parse-npm-lock.go +++ b/pkg/lockfile/parse-npm-lock.go @@ -63,6 +63,10 @@ func (dep NpmLockDependency) depGroups() []string { return nil } +func npmPkgKey(name string, version string, dev bool, optional bool) string { + return fmt.Sprintf("%s@%s@%t+%t", name, version, dev, optional) +} + func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[string]PackageDetails { details := map[string]PackageDetails{} @@ -98,7 +102,12 @@ func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[str } } - details[name+"@"+version] = PackageDetails{ + details[npmPkgKey( + name, + version, + detail.Dev, + detail.Optional, + )] = PackageDetails{ Name: name, Version: finalVersion, Ecosystem: NpmEcosystem, @@ -159,7 +168,12 @@ func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]Package finalVersion = commit } - details[finalName+"@"+finalVersion] = PackageDetails{ + details[npmPkgKey( + finalName, + finalVersion, + detail.DevOptional || detail.Dev, + detail.DevOptional || detail.Optional, + )] = PackageDetails{ Name: finalName, Version: detail.Version, Ecosystem: NpmEcosystem, From d2be869007db30fc650fbb61f20198844e76e195 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Thu, 18 Apr 2024 13:46:40 +1200 Subject: [PATCH 02/10] refactor: use a type with a function --- pkg/lockfile/parse-npm-lock.go | 68 ++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/pkg/lockfile/parse-npm-lock.go b/pkg/lockfile/parse-npm-lock.go index 08f67aae8b..80e21068ff 100644 --- a/pkg/lockfile/parse-npm-lock.go +++ b/pkg/lockfile/parse-npm-lock.go @@ -68,7 +68,7 @@ func npmPkgKey(name string, version string, dev bool, optional bool) 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 { @@ -102,19 +102,22 @@ func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[str } } - details[npmPkgKey( - name, - version, - detail.Dev, - detail.Optional, - )] = PackageDetails{ - Name: name, - Version: finalVersion, - Ecosystem: NpmEcosystem, - CompareAs: NpmEcosystem, - Commit: commit, - DepGroups: detail.depGroups(), - } + details.add( + npmPkgKey( + name, + version, + detail.Dev, + detail.Optional, + ), + PackageDetails{ + Name: name, + Version: finalVersion, + Ecosystem: NpmEcosystem, + CompareAs: NpmEcosystem, + Commit: commit, + DepGroups: detail.depGroups(), + }, + ) } return details @@ -145,8 +148,14 @@ func (pkg NpmLockPackage) depGroups() []string { return nil } +type npmPackageDetailsMap map[string]PackageDetails + +func (pdm npmPackageDetailsMap) add(key string, details PackageDetails) { + pdm[key] = details +} + func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]PackageDetails { - details := map[string]PackageDetails{} + details := npmPackageDetailsMap{} for namePath, detail := range packages { if namePath == "" { @@ -168,19 +177,22 @@ func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]Package finalVersion = commit } - details[npmPkgKey( - finalName, - finalVersion, - detail.DevOptional || detail.Dev, - detail.DevOptional || detail.Optional, - )] = PackageDetails{ - Name: finalName, - Version: detail.Version, - Ecosystem: NpmEcosystem, - CompareAs: NpmEcosystem, - Commit: commit, - DepGroups: detail.depGroups(), - } + details.add( + npmPkgKey( + finalName, + finalVersion, + detail.DevOptional || detail.Dev, + detail.DevOptional || detail.Optional, + ), + PackageDetails{ + Name: finalName, + Version: detail.Version, + Ecosystem: NpmEcosystem, + CompareAs: NpmEcosystem, + Commit: commit, + DepGroups: detail.depGroups(), + }, + ) } return details From 53f080c29326368e8d53386e0c4475f71fcdcc32 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Thu, 18 Apr 2024 15:51:08 +1200 Subject: [PATCH 03/10] fix: merge groups for npm packages --- pkg/lockfile/parse-npm-lock-v1_test.go | 23 +--------------- pkg/lockfile/parse-npm-lock-v2_test.go | 24 +---------------- pkg/lockfile/parse-npm-lock.go | 36 +++++++++++++++++++++++--- 3 files changed, 34 insertions(+), 49 deletions(-) diff --git a/pkg/lockfile/parse-npm-lock-v1_test.go b/pkg/lockfile/parse-npm-lock-v1_test.go index f217987a1c..02b6307506 100644 --- a/pkg/lockfile/parse-npm-lock-v1_test.go +++ b/pkg/lockfile/parse-npm-lock-v1_test.go @@ -445,28 +445,7 @@ func TestParseNpmLock_v1_SamePackageDifferentGroups(t *testing.T) { Version: "5.5.2", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: nil, - }, - { - Name: "ajv", - Version: "5.5.2", - Ecosystem: lockfile.NpmEcosystem, - CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{"dev"}, - }, - { - Name: "ajv", - Version: "5.5.2", - Ecosystem: lockfile.NpmEcosystem, - CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{"optional"}, - }, - { - Name: "ajv", - Version: "5.5.2", - Ecosystem: lockfile.NpmEcosystem, - CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{"dev", "optional"}, + DepGroups: []string{}, }, }) } diff --git a/pkg/lockfile/parse-npm-lock-v2_test.go b/pkg/lockfile/parse-npm-lock-v2_test.go index 352d5491d2..bd763b4d1d 100644 --- a/pkg/lockfile/parse-npm-lock-v2_test.go +++ b/pkg/lockfile/parse-npm-lock-v2_test.go @@ -432,35 +432,13 @@ func TestParseNpmLock_v2_SamePackageDifferentGroups(t *testing.T) { Version: "1.0.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: nil, }, { Name: "ajv", Version: "5.5.2", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: nil, - }, - { - Name: "ajv", - Version: "5.5.2", - Ecosystem: lockfile.NpmEcosystem, - CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{"dev"}, - }, - { - Name: "ajv", - Version: "5.5.2", - Ecosystem: lockfile.NpmEcosystem, - CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{"optional"}, - }, - { - Name: "ajv", - Version: "5.5.2", - Ecosystem: lockfile.NpmEcosystem, - CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{"dev", "optional"}, + DepGroups: []string{}, }, }) } diff --git a/pkg/lockfile/parse-npm-lock.go b/pkg/lockfile/parse-npm-lock.go index 80e21068ff..2be7ca0cc5 100644 --- a/pkg/lockfile/parse-npm-lock.go +++ b/pkg/lockfile/parse-npm-lock.go @@ -8,6 +8,7 @@ import ( "strings" "golang.org/x/exp/maps" + "golang.org/x/exp/slices" ) type NpmLockDependency struct { @@ -106,8 +107,8 @@ func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[str npmPkgKey( name, version, - detail.Dev, - detail.Optional, + true || detail.Dev, + true || detail.Optional, ), PackageDetails{ Name: name, @@ -150,7 +151,34 @@ func (pkg NpmLockPackage) depGroups() []string { 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 []string{} + } + + 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 } @@ -181,8 +209,8 @@ func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]Package npmPkgKey( finalName, finalVersion, - detail.DevOptional || detail.Dev, - detail.DevOptional || detail.Optional, + true || detail.DevOptional || detail.Dev, + true || detail.DevOptional || detail.Optional, ), PackageDetails{ Name: finalName, From e4b7862f08ebdf0ad971377272d85bd58d222198 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 24 Apr 2024 07:10:44 +1200 Subject: [PATCH 04/10] refactor: clean up code --- pkg/lockfile/parse-npm-lock.go | 132 ++++++++++++++++----------------- 1 file changed, 63 insertions(+), 69 deletions(-) diff --git a/pkg/lockfile/parse-npm-lock.go b/pkg/lockfile/parse-npm-lock.go index 2be7ca0cc5..06dff0fce3 100644 --- a/pkg/lockfile/parse-npm-lock.go +++ b/pkg/lockfile/parse-npm-lock.go @@ -50,6 +50,53 @@ 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 []string{} + } + + 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 mergePkgDetailsMap(m1 map[string]PackageDetails, m2 map[string]PackageDetails) map[string]PackageDetails { + details := map[string]PackageDetails{} + + for name, detail := range m1 { + details[name] = detail + } + + for name, detail := range m2 { + details[name] = detail + } + + return details +} + func (dep NpmLockDependency) depGroups() []string { if dep.Dev && dep.Optional { return []string{"dev", "optional"} @@ -64,10 +111,6 @@ func (dep NpmLockDependency) depGroups() []string { return nil } -func npmPkgKey(name string, version string, dev bool, optional bool) string { - return fmt.Sprintf("%s@%s@%t+%t", name, version, dev, optional) -} - func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[string]PackageDetails { details := npmPackageDetailsMap{} @@ -103,22 +146,14 @@ func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[str } } - details.add( - npmPkgKey( - name, - version, - true || detail.Dev, - true || detail.Optional, - ), - PackageDetails{ - Name: name, - Version: finalVersion, - Ecosystem: NpmEcosystem, - CompareAs: NpmEcosystem, - Commit: commit, - DepGroups: detail.depGroups(), - }, - ) + details.add(name+"@"+version, PackageDetails{ + Name: name, + Version: finalVersion, + Ecosystem: NpmEcosystem, + CompareAs: NpmEcosystem, + Commit: commit, + DepGroups: detail.depGroups(), + }) } return details @@ -149,39 +184,6 @@ func (pkg NpmLockPackage) depGroups() []string { return nil } -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 []string{} - } - - 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 parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]PackageDetails { details := npmPackageDetailsMap{} @@ -205,22 +207,14 @@ func parseNpmLockPackages(packages map[string]NpmLockPackage) map[string]Package finalVersion = commit } - details.add( - npmPkgKey( - finalName, - finalVersion, - true || detail.DevOptional || detail.Dev, - true || detail.DevOptional || detail.Optional, - ), - PackageDetails{ - Name: finalName, - Version: detail.Version, - Ecosystem: NpmEcosystem, - CompareAs: NpmEcosystem, - Commit: commit, - DepGroups: detail.depGroups(), - }, - ) + details.add(finalName+"@"+finalVersion, PackageDetails{ + Name: finalName, + Version: detail.Version, + Ecosystem: NpmEcosystem, + CompareAs: NpmEcosystem, + Commit: commit, + DepGroups: detail.depGroups(), + }) } return details From b8712e5e6d7b9b001836dd0f8f087b72d94ad044 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 24 Apr 2024 07:19:49 +1200 Subject: [PATCH 05/10] fix: merge responsibly --- pkg/lockfile/parse-npm-lock.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/lockfile/parse-npm-lock.go b/pkg/lockfile/parse-npm-lock.go index 06dff0fce3..9339166eda 100644 --- a/pkg/lockfile/parse-npm-lock.go +++ b/pkg/lockfile/parse-npm-lock.go @@ -83,15 +83,15 @@ func (pdm npmPackageDetailsMap) add(key string, details PackageDetails) { pdm[key] = details } -func mergePkgDetailsMap(m1 map[string]PackageDetails, m2 map[string]PackageDetails) map[string]PackageDetails { - details := map[string]PackageDetails{} +func mergePkgDetailsMap(m1 npmPackageDetailsMap, m2 npmPackageDetailsMap) map[string]PackageDetails { + details := npmPackageDetailsMap{} for name, detail := range m1 { - details[name] = detail + details.add(name, detail) } for name, detail := range m2 { - details[name] = detail + details.add(name, detail) } return details From 1ad64e2b9008d200824cd58a9b1199b3107ad06f Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 24 Apr 2024 07:25:21 +1200 Subject: [PATCH 06/10] test: update cases --- pkg/lockfile/parse-npm-lock-v1_test.go | 3 +++ pkg/lockfile/parse-npm-lock-v2_test.go | 2 ++ 2 files changed, 5 insertions(+) diff --git a/pkg/lockfile/parse-npm-lock-v1_test.go b/pkg/lockfile/parse-npm-lock-v1_test.go index 02b6307506..a7b98119a4 100644 --- a/pkg/lockfile/parse-npm-lock-v1_test.go +++ b/pkg/lockfile/parse-npm-lock-v1_test.go @@ -188,6 +188,7 @@ func TestParseNpmLock_v1_NestedDependenciesDup(t *testing.T) { Version: "6.1.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{}, }) expectPackage(t, packages, lockfile.PackageDetails{ @@ -195,6 +196,7 @@ func TestParseNpmLock_v1_NestedDependenciesDup(t *testing.T) { Version: "5.5.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{}, }) expectPackage(t, packages, lockfile.PackageDetails{ @@ -202,6 +204,7 @@ func TestParseNpmLock_v1_NestedDependenciesDup(t *testing.T) { Version: "2.0.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{}, }) } diff --git a/pkg/lockfile/parse-npm-lock-v2_test.go b/pkg/lockfile/parse-npm-lock-v2_test.go index bd763b4d1d..4cead55f29 100644 --- a/pkg/lockfile/parse-npm-lock-v2_test.go +++ b/pkg/lockfile/parse-npm-lock-v2_test.go @@ -184,12 +184,14 @@ func TestParseNpmLock_v2_NestedDependenciesDup(t *testing.T) { Version: "6.1.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{}, }, { Name: "supports-color", Version: "2.0.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, + DepGroups: []string{}, }, }) } From 1ec33497272cd2ad10f3ac1f28ff1f738bfcf33d Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 24 Apr 2024 07:33:11 +1200 Subject: [PATCH 07/10] refactor: return `nil` instead of an empty slice --- pkg/lockfile/parse-npm-lock-v1_test.go | 4 ---- pkg/lockfile/parse-npm-lock-v2_test.go | 3 --- pkg/lockfile/parse-npm-lock.go | 2 +- 3 files changed, 1 insertion(+), 8 deletions(-) diff --git a/pkg/lockfile/parse-npm-lock-v1_test.go b/pkg/lockfile/parse-npm-lock-v1_test.go index a7b98119a4..218b11251d 100644 --- a/pkg/lockfile/parse-npm-lock-v1_test.go +++ b/pkg/lockfile/parse-npm-lock-v1_test.go @@ -188,7 +188,6 @@ func TestParseNpmLock_v1_NestedDependenciesDup(t *testing.T) { Version: "6.1.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{}, }) expectPackage(t, packages, lockfile.PackageDetails{ @@ -196,7 +195,6 @@ func TestParseNpmLock_v1_NestedDependenciesDup(t *testing.T) { Version: "5.5.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{}, }) expectPackage(t, packages, lockfile.PackageDetails{ @@ -204,7 +202,6 @@ func TestParseNpmLock_v1_NestedDependenciesDup(t *testing.T) { Version: "2.0.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{}, }) } @@ -448,7 +445,6 @@ func TestParseNpmLock_v1_SamePackageDifferentGroups(t *testing.T) { Version: "5.5.2", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{}, }, }) } diff --git a/pkg/lockfile/parse-npm-lock-v2_test.go b/pkg/lockfile/parse-npm-lock-v2_test.go index 4cead55f29..13a62ee5bd 100644 --- a/pkg/lockfile/parse-npm-lock-v2_test.go +++ b/pkg/lockfile/parse-npm-lock-v2_test.go @@ -184,14 +184,12 @@ func TestParseNpmLock_v2_NestedDependenciesDup(t *testing.T) { Version: "6.1.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{}, }, { Name: "supports-color", Version: "2.0.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{}, }, }) } @@ -440,7 +438,6 @@ func TestParseNpmLock_v2_SamePackageDifferentGroups(t *testing.T) { Version: "5.5.2", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: []string{}, }, }) } diff --git a/pkg/lockfile/parse-npm-lock.go b/pkg/lockfile/parse-npm-lock.go index 9339166eda..d32fa11f8e 100644 --- a/pkg/lockfile/parse-npm-lock.go +++ b/pkg/lockfile/parse-npm-lock.go @@ -61,7 +61,7 @@ type npmPackageDetailsMap map[string]PackageDetails 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 []string{} + return nil } combined := make([]string, 0, len(a.DepGroups)+len(b.DepGroups)) From 7eb21024e72b55eb54f2be0c03037f85cd783a9c Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 24 Apr 2024 07:36:23 +1200 Subject: [PATCH 08/10] refactor: clean up merge so it's not used by NuGet --- pkg/lockfile/parse-npm-lock.go | 16 ++++------------ pkg/lockfile/parse-nuget-lock.go | 4 +++- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/pkg/lockfile/parse-npm-lock.go b/pkg/lockfile/parse-npm-lock.go index d32fa11f8e..642bba8d7f 100644 --- a/pkg/lockfile/parse-npm-lock.go +++ b/pkg/lockfile/parse-npm-lock.go @@ -83,18 +83,10 @@ func (pdm npmPackageDetailsMap) add(key string, details PackageDetails) { pdm[key] = details } -func mergePkgDetailsMap(m1 npmPackageDetailsMap, m2 npmPackageDetailsMap) map[string]PackageDetails { - details := npmPackageDetailsMap{} - - for name, detail := range m1 { - details.add(name, detail) - } - - for name, detail := range m2 { - details.add(name, detail) +func (pdm npmPackageDetailsMap) merge(m npmPackageDetailsMap) { + for key, details := range m { + pdm.add(key, details) } - - return details } func (dep NpmLockDependency) depGroups() []string { @@ -116,7 +108,7 @@ func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[str for name, detail := range dependencies { if detail.Dependencies != nil { - maps.Copy(details, parseNpmLockDependencies(detail.Dependencies)) + details.merge(parseNpmLockDependencies(detail.Dependencies)) } version := detail.Version diff --git a/pkg/lockfile/parse-nuget-lock.go b/pkg/lockfile/parse-nuget-lock.go index c097aee4aa..7c88cca17d 100644 --- a/pkg/lockfile/parse-nuget-lock.go +++ b/pkg/lockfile/parse-nuget-lock.go @@ -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 From b2f5ce0cb4f4ef461a1831b502a471162e80925c Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Fri, 28 Jun 2024 12:38:20 +1200 Subject: [PATCH 09/10] refactor: use native `maps.Copy` --- pkg/lockfile/parse-npm-lock.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/pkg/lockfile/parse-npm-lock.go b/pkg/lockfile/parse-npm-lock.go index 642bba8d7f..a63a0aa7b3 100644 --- a/pkg/lockfile/parse-npm-lock.go +++ b/pkg/lockfile/parse-npm-lock.go @@ -83,12 +83,6 @@ func (pdm npmPackageDetailsMap) add(key string, details PackageDetails) { pdm[key] = details } -func (pdm npmPackageDetailsMap) merge(m npmPackageDetailsMap) { - for key, details := range m { - pdm.add(key, details) - } -} - func (dep NpmLockDependency) depGroups() []string { if dep.Dev && dep.Optional { return []string{"dev", "optional"} @@ -108,7 +102,7 @@ func parseNpmLockDependencies(dependencies map[string]NpmLockDependency) map[str for name, detail := range dependencies { if detail.Dependencies != nil { - details.merge(parseNpmLockDependencies(detail.Dependencies)) + maps.Copy(details, parseNpmLockDependencies(detail.Dependencies)) } version := detail.Version From c132a552e0795263438df99eef3b2c4632c93d32 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Sat, 29 Jun 2024 07:41:20 +1200 Subject: [PATCH 10/10] test: remove `nil` --- pkg/lockfile/parse-npm-lock-v1_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/lockfile/parse-npm-lock-v1_test.go b/pkg/lockfile/parse-npm-lock-v1_test.go index 218b11251d..8c07224f53 100644 --- a/pkg/lockfile/parse-npm-lock-v1_test.go +++ b/pkg/lockfile/parse-npm-lock-v1_test.go @@ -438,7 +438,6 @@ func TestParseNpmLock_v1_SamePackageDifferentGroups(t *testing.T) { Version: "1.0.0", Ecosystem: lockfile.NpmEcosystem, CompareAs: lockfile.NpmEcosystem, - DepGroups: nil, }, { Name: "ajv",