RTECO-1362 - Add strict mode for npm build-info dependency resolution

build/npm.go

@@ -13,12 +13,13 @@ import (
 const minSupportedNpmVersion = "5.4.0"
 
 type NpmModule struct {
-	containingBuild  *Build
-	name             string
-	srcPath          string
-	executablePath   string
-	npmArgs          []string
-	collectBuildInfo bool
+	containingBuild   *Build
+	name              string
+	srcPath           string
+	executablePath    string
+	npmArgs           []string
+	collectBuildInfo  bool
+	failOnMissingDeps bool
 }
 
 // Pass an empty string for srcPath to find the npm project in the working directory.
@@ -75,7 +76,7 @@ func (nm *NpmModule) CalcDependencies() error {
 		return errors.New("a build name must be provided in order to collect the project's dependencies")
 	}
 	buildInfoDependencies, err := buildutils.CalculateNpmDependenciesList(nm.executablePath, nm.srcPath, nm.name,
-		buildutils.NpmTreeDepListParam{Args: nm.npmArgs}, true, nm.containingBuild.logger)
+		buildutils.NpmTreeDepListParam{Args: nm.npmArgs, FailOnMissingDeps: nm.failOnMissingDeps}, true, nm.containingBuild.logger)
 	if err != nil {
 		return err
 	}
@@ -96,6 +97,10 @@ func (nm *NpmModule) SetCollectBuildInfo(collectBuildInfo bool) {
 	nm.collectBuildInfo = collectBuildInfo
 }
 
+func (nm *NpmModule) SetFailOnMissingDeps(failOnMissingDeps bool) {
+	nm.failOnMissingDeps = failOnMissingDeps
+}
+
 func (nm *NpmModule) AddArtifacts(artifacts ...entities.Artifact) error {
 	return nm.containingBuild.AddArtifacts(nm.name, entities.Npm, artifacts...)
 }

build/utils/npm.go

@@ -84,12 +84,26 @@ func CalculateNpmDependenciesList(executablePath, srcPath, moduleId string, npmP
 	if len(missingOptionalDeps) > 0 {
 		printMissingDependenciesWarning("optionalDependencies", missingOptionalDeps, log)
 	}
-	if len(otherMissingDeps) > 0 {
-		log.Warn("The following dependencies will not be included in the build-info, because they are missing in the npm cache: '" + strings.Join(otherMissingDeps, ",") + "'.\nHint: Try deleting 'node_modules' and/or 'package-lock.json'.")
+	if err = handleOtherMissingDeps(otherMissingDeps, npmParams.FailOnMissingDeps, log); err != nil {
+		return nil, err
 	}
 	return dependenciesList, nil
 }
 
+// handleOtherMissingDeps reports dependencies whose tarball could not be resolved from the npm cache.
+// In strict mode (FailOnMissingDeps), this returns an error instead of only warning, causing the
+// caller to fail the build rather than publish an incomplete build-info.
+func handleOtherMissingDeps(otherMissingDeps []string, failOnMissingDeps bool, log utils.Log) error {
+	if len(otherMissingDeps) == 0 {
+		return nil
+	}
+	if failOnMissingDeps {
+		return errors.New("the following dependencies could not be resolved from the npm cache and could not be mapped into the build-info: '" + strings.Join(otherMissingDeps, ",") + "' (strict mode: --fail-on-missing-deps).\nHint: Try deleting 'node_modules' and/or 'package-lock.json'.")
+	}
+	log.Warn("The following dependencies will not be included in the build-info, because they are missing in the npm cache: '" + strings.Join(otherMissingDeps, ",") + "'.\nHint: Try deleting 'node_modules' and/or 'package-lock.json'.")
+	return nil
+}
+
 type dependencyInfo struct {
 	entities.Dependency
 	*npmLsDependency
@@ -266,6 +280,8 @@ type NpmTreeDepListParam struct {
 	IgnoreNodeModules bool
 	// Rewrite package-lock.json, if exists.
 	OverwritePackageLock bool
+	// If true, fail the build instead of only warning when a dependency's tarball can't be found in the npm cache.
+	FailOnMissingDeps bool
 }
 
 // npm >=7 ls results for a single dependency

build/utils/npm_test.go

@@ -138,8 +138,7 @@
 		{"shopify-liquid:1.d7.9", [][]string{{"xpm:0.1.1", "@jfrog/npm_scoped:1.0.0", "root"}}},
 	}
 	dependencies := make(map[string]*dependencyInfo)
-	var unresolvedDeps []string
-	err = parseDependencies(dependenciesJsonList, []string{"root"}, dependencies, npmLsDependencyParser, &unresolvedDeps, utils.NewDefaultLogger(utils.INFO))
+	err = parseDependencies(dependenciesJsonList, []string{"root"}, dependencies, npmLsDependencyParser, utils.NewDefaultLogger(utils.INFO))
 	assert.NoError(t, err)
 	assert.Equal(t, len(expectedDependenciesList), len(dependencies))
 	for _, eDependency := range expectedDependenciesList {
@@ -154,23 +153,6 @@
 	}
 }
 
-func TestParseDependencies_UnresolvedDepsDeduplicated(t *testing.T) {
-	inputJson := `{
-		"pkg-a": {"version": "1.0.0", "dependencies": {
-			"react": {"problems": ["missing: react@^18.0.0, required by pkg-a@1.0.0"]}
-		}},
-		"pkg-b": {"version": "1.0.0", "dependencies": {
-			"react": {"problems": ["missing: react@^18.0.0, required by pkg-b@1.0.0"]}
-		}}
-	}`
-	depsMap := make(map[string]*dependencyInfo)
-	var unresolvedDeps []string
-	err := parseDependencies([]byte(inputJson), []string{"root"}, depsMap, npmLsDependencyParser, &unresolvedDeps, &utils.NullLog{})
-	assert.NoError(t, err)
-	assert.Equal(t, []string{"react"}, unresolvedDeps,
-		"expected 'react' to be reported once even though it is missing under two different parents")
-}
-
 func TestAppendScopes(t *testing.T) {
 	var scopes = []struct {
 		a        []string
@@ -567,13 +549,12 @@
 
 func TestParseDependenciesEdgeCases(t *testing.T) {
 	testcases := []struct {
-		name                   string
-		inputJson              string
-		expectedId             string
-		shouldBeSkipped        bool
-		expectParseError       bool
-		expectedRequestedBy    [][]string
-		expectedUnresolvedName string
+		name                string
+		inputJson           string
+		expectedId          string
+		shouldBeSkipped     bool
+		expectParseError    bool
+		expectedRequestedBy [][]string
 	}{
 		{
 			name:             "Git URL with hash in resolved",
@@ -613,11 +594,10 @@
 			expectParseError: false,
 		},
 		{
-			name:                   "No version and no resolved, but missing",
-			inputJson:              `{"bad-pkg":{"missing": true}}`,
-			shouldBeSkipped:        true,
-			expectParseError:       false,
-			expectedUnresolvedName: "bad-pkg",
+			name:             "No version and no resolved, but missing",
+			inputJson:        `{"bad-pkg":{"missing": true}}`,
+			shouldBeSkipped:  true,
+			expectParseError: false,
 		},
 		{
 			name:             "No version and no resolved, not missing",
@@ -626,62 +606,11 @@
 			expectParseError: true,
 		},
 		{
-			name:                   "Missing dependency with no problems array",
-			inputJson:              `{"peer-pkg":{"missing": true}}`,
-			shouldBeSkipped:        true,
-			expectParseError:       false,
-			expectedUnresolvedName: "peer-pkg",
-		},
-		{
-			// npm reports this identical shape for a missing peer, prod, or dev dependency;
-			// react/react-dom here just mirrors the ticket's actual repro (an unmet peer).
-			name:                   "Missing dependency with semver range in problems",
-			inputJson:              `{"react":{"problems": ["missing: react@^18.2.0, required by react-dom@18.2.0"]}}`,
-			shouldBeSkipped:        true,
-			expectParseError:       false,
-			expectedUnresolvedName: "react",
-		},
-		{
-			name:      "Missing dependency with git locator in problems",
-			inputJson: `{"my-private-package":{"problems": ["missing: my-private-package@git+ssh://git@github.com/my-org/my-private-package.git#v1.0.0, required by root"]}}`,
-			expectedId: func() string {
-				return "my-private-package:v1.0.0"
-			}(),
-			shouldBeSkipped:  false,
-			expectParseError: false,
-		},
-		{
-			name:                   "Missing dependency with bare GitHub shorthand, no ref, in problems",
-			inputJson:              `{"express":{"problems": ["missing: express@expressjs/express, required by react-dom@18.2.0"]}}`,
-			shouldBeSkipped:        true,
-			expectParseError:       false,
-			expectedUnresolvedName: "express",
-		},
-		{
-			name:             "Missing dependency with bare GitHub shorthand and ref in problems",
-			inputJson:        `{"express":{"problems": ["missing: express@expressjs/express#v4.18.0, required by react-dom@18.2.0"]}}`,
-			expectedId:       "express:v4.18.0",
-			shouldBeSkipped:  false,
+			name:             "Missing peer dependency",
+			inputJson:        `{"peer-pkg":{"missing": true}}`,
+			shouldBeSkipped:  true,
 			expectParseError: false,
 		},
-		{
-			// The literal "peer dependency" scenario: a peer pinned to an exact version that
-			// was never installed. X here is a bare version, not a range and not a locator.
-			name:                   "Missing peer dependency with exact pinned version in problems",
-			inputJson:              `{"left-pad":{"problems": ["missing: left-pad@2.5.3, required by pkg-a@1.0.0"]}}`,
-			shouldBeSkipped:        true,
-			expectParseError:       false,
-			expectedUnresolvedName: "left-pad",
-		},
-		{
-			// Same mechanism, but the requirer relationship is an ordinary (non-peer) dependency,
-			// not a peer — proves the skip isn't peer-specific (see Finding #1/#2 discussion).
-			name:                   "Missing non-peer dependency with semver range in problems",
-			inputJson:              `{"lodash":{"problems": ["missing: lodash@^4.17.0, required by pkg-a@1.0.0"]}}`,
-			shouldBeSkipped:        true,
-			expectParseError:       false,
-			expectedUnresolvedName: "lodash",
-		},
 		{
 			name:             "Regular dependency is not affected",
 			inputJson:        `{"react":{"version": "18.2.0", "integrity": "sha512-..."}}`,
@@ -723,9 +652,6 @@
 
 			if tc.shouldBeSkipped {
 				assert.Empty(t, depsMap, "Expected dependency to be skipped, but it was added")
-				if tc.expectedUnresolvedName != "" {
-					assert.Contains(t, unresolvedDeps, tc.expectedUnresolvedName, "Expected skipped dependency to be reported as unresolved")
-				}
 			} else {
 				assert.Len(t, depsMap, 1, "Expected exactly one dependency")
 				// Check if the key exists
@@ -766,3 +692,52 @@
 		})
 	}
 }
+
+func TestHandleOtherMissingDeps(t *testing.T) {
+	testcases := []struct {
+		name              string
+		otherMissingDeps  []string
+		failOnMissingDeps bool
+		expectError       bool
+	}{
+		{
+			name:              "no missing deps, strict mode off",
+			otherMissingDeps:  nil,
+			failOnMissingDeps: false,
+			expectError:       false,
+		},
+		{
+			name:              "no missing deps, strict mode on",
+			otherMissingDeps:  nil,
+			failOnMissingDeps: true,
+			expectError:       false,
+		},
+		{
+			name:              "missing deps, strict mode off - warns and succeeds",
+			otherMissingDeps:  []string{"lodash:4.17.21"},
+			failOnMissingDeps: false,
+			expectError:       false,
+		},
+		{
+			name:              "missing deps, strict mode on - fails the build",
+			otherMissingDeps:  []string{"lodash:4.17.21", "chalk:5.0.0"},
+			failOnMissingDeps: true,
+			expectError:       true,
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := handleOtherMissingDeps(tc.otherMissingDeps, tc.failOnMissingDeps, &utils.NullLog{})
+			if tc.expectError {
+				assert.Error(t, err)
+				for _, dep := range tc.otherMissingDeps {
+					assert.Contains(t, err.Error(), dep)
+				}
+				assert.Contains(t, err.Error(), "fail-on-missing-deps")
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}