fix: honour the lockfile when building nodejs dependencies in source

aws_lambda_builders/workflows/nodejs_npm/actions.py

@@ -102,6 +102,22 @@ class NodejsNpmInstallAction(NodejsNpmInstallOrUpdateBaseAction):
     NAME = "NpmInstall"
     DESCRIPTION = "Installing dependencies from NPM"
 
+    def __init__(self, install_dir: str, subprocess_npm: SubprocessNpm, install_links: Optional[bool] = False):
+        """
+        Parameters
+        ----------
+        install_dir : str
+            Dependencies will be installed in this directory.
+        subprocess_npm : SubprocessNpm
+            An instance of the NPM process wrapper
+        install_links : Optional[bool]
+            Uses the --install-links npm option if True, by default False. Required when installing into the
+            source directory, so that local file dependencies are installed as regular dependencies.
+        """
+
+        super().__init__(install_dir=install_dir, subprocess_npm=subprocess_npm)
+        self.install_links = install_links
+
     def execute(self):
         """
         Runs the action.
@@ -112,6 +128,8 @@ def execute(self):
             LOG.debug("NODEJS installing production dependencies in: %s", self.install_dir)
 
             command = ["install", "-q", "--no-audit", "--no-save", "--omit=dev"]
+            if self.install_links:
+                command.append("--install-links")
             self.subprocess_npm.run(command, cwd=self.install_dir)
 
         except NpmExecutionError as ex:
@@ -120,7 +138,11 @@ def execute(self):
 
 class NodejsNpmUpdateAction(NodejsNpmInstallOrUpdateBaseAction):
     """
-    A Lambda Builder Action that installs NPM project dependencies
+    A Lambda Builder Action that installs NPM project dependencies, ignoring any lockfile.
+
+    Only used when building in source for a project that has no lockfile: `--no-package-lock` means
+    dependency versions are resolved afresh on every build, so a project that does have a lockfile
+    is installed with NodejsNpmInstallAction instead, to keep builds reproducible.
     """
 
     NAME = "NpmUpdate"

aws_lambda_builders/workflows/nodejs_npm/workflow.py

@@ -25,7 +25,7 @@
     NodejsNpmTestAction,
     NodejsNpmUpdateAction,
 )
-from aws_lambda_builders.workflows.nodejs_npm.npm import SubprocessNpm
+from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError, SubprocessNpm
 from aws_lambda_builders.workflows.nodejs_npm.utils import OSUtils
 
 LOG = logging.getLogger(__name__)
@@ -174,6 +174,7 @@ def _actions_for_cleanup(self):
 
     @property
     def _actions_for_linking_source_dependencies_to_artifacts(self):
+        # Known gap in a workspaces monorepo - no node_modules beside the function: aws/aws-lambda-builders#933
         source_dependencies_path = os.path.join(self.source_dir, "node_modules")
         artifact_dependencies_path = os.path.join(self.artifacts_dir, "node_modules")
         return [LinkSinglePathAction(source=source_dependencies_path, dest=artifact_dependencies_path)]
@@ -256,16 +257,67 @@ def get_install_action(
             "Dev dependencies are omitted from the Lambda artifacts package"
         )
 
+        # `npm ci` stays gated on a lockfile in the source directory itself: run from a workspace package it
+        # reifies from the lockfile and empties the workspace root's dev dependencies, including build tools
+        # the build needs. `--omit=dev` prunes the install directory's own dev dependencies on every version.
         if (osutils.file_exists(lockfile_path) or osutils.file_exists(shrinkwrap_path)) and npm_ci_option:
             return NodejsNpmCIAction(
                 install_dir=install_dir, subprocess_npm=subprocess_npm, install_links=is_building_in_source
             )
 
         if is_building_in_source:
+            # With a lockfile npm will use, install the locked versions. Without one there is nothing to be
+            # reproducible about, so keep updating, which also prunes dependencies that were removed from the
+            # manifest since the last build.
+            if NodejsNpmWorkflow.get_lockfile_path(install_dir, subprocess_npm, osutils):
+                return NodejsNpmInstallAction(
+                    install_dir=install_dir, subprocess_npm=subprocess_npm, install_links=True
+                )
+
             return NodejsNpmUpdateAction(install_dir=install_dir, subprocess_npm=subprocess_npm)
 
         return NodejsNpmInstallAction(install_dir=install_dir, subprocess_npm=subprocess_npm)
 
+    @staticmethod
+    def get_lockfile_path(install_dir: str, subprocess_npm: SubprocessNpm, osutils: OSUtils) -> Optional[str]:
+        """
+        Find the lockfile npm would use when it runs in the given directory.
+
+        The lockfile does not have to sit in that directory: in an npm workspaces monorepo a single
+        package-lock.json lives at the repository root and covers every workspace package. Which directory that
+        is, is npm's own decision - `npm prefix` reports it, resolving to the workspace root for a workspace
+        package and to the directory itself for any other nested package, whose ancestors' lockfiles npm
+        ignores. Asking npm keeps this answer identical to the one the install will act on, and bounded to the
+        project.
+
+        Parameters
+        ----------
+        install_dir : str
+            the directory npm will run in, where dependencies will be installed
+        subprocess_npm : SubprocessNpm
+            An instance of the NPM process wrapper
+        osutils : OSUtils
+            An instance of OS Utilities for file manipulation
+
+        Returns
+        -------
+        Optional[str]
+            Path of the lockfile in npm's project root, or None if that project does not have one
+        """
+        try:
+            project_root = subprocess_npm.run(["prefix"], cwd=install_dir).strip()
+        except NpmExecutionError as ex:
+            # without npm's answer there is no evidence a lockfile applies, so install as if there were none
+            LOG.debug("NODEJS could not resolve the npm project root of %s: %s", install_dir, ex)
+            return None
+
+        for lockfile_name in ("package-lock.json", "npm-shrinkwrap.json"):
+            lockfile_path = osutils.joinpath(project_root, lockfile_name)
+            if osutils.file_exists(lockfile_path):
+                return lockfile_path
+
+        return None
+
     @staticmethod
     def can_use_install_links(npm_process: SubprocessNpm) -> bool:
         """

tests/integration/workflows/nodejs_npm/test_nodejs_npm.py

@@ -1,4 +1,5 @@
 import itertools
+import json
 import logging
 import os
 import shutil
@@ -11,6 +12,8 @@
 from aws_lambda_builders.builder import LambdaBuilder
 from aws_lambda_builders.exceptions import WorkflowFailedError
 from aws_lambda_builders.supported_runtimes import NODEJS_RUNTIMES
+from aws_lambda_builders.workflows.nodejs_npm.npm import SubprocessNpm
+from aws_lambda_builders.workflows.nodejs_npm.utils import OSUtils
 from tests.testing_utils import read_link_without_junction_prefix
 
 logger = logging.getLogger("aws_lambda_builders.workflows.nodejs_npm.workflow")
@@ -306,6 +309,149 @@ def test_build_in_source_with_download_dependencies(self, runtime):
         output_files = set(os.listdir(self.artifacts_dir))
         self.assertEqual(expected_files, output_files)
 
+    @parameterized.expand(SUPPORTED_RUNTIMES)
+    def test_build_in_source_with_removed_dependencies_and_a_lockfile(self, runtime):
+        # a project with a lockfile installs the locked versions, and still drops a dependency that was
+        # removed from the manifest even though the lockfile it reads still lists it
+        source_dir = os.path.join(self.temp_testdata_dir, "npm-deps-with-lockfile")
+        lockfile_path = os.path.join(source_dir, "package-lock.json")
+        with open(lockfile_path, "rb") as lockfile:
+            original_lockfile = lockfile.read()
+
+        self.builder.build(
+            source_dir,
+            self.artifacts_dir,
+            self.scratch_dir,
+            os.path.join(source_dir, "package.json"),
+            runtime=runtime,
+            build_in_source=True,
+        )
+
+        source_node_modules = os.path.join(source_dir, "node_modules")
+        self.assertIn("minimal-request-promise", set(os.listdir(source_node_modules)))
+        installed_manifest = os.path.join(source_node_modules, "minimal-request-promise", "package.json")
+        with open(installed_manifest) as manifest:
+            # the lockfile pins 1.3.0 while the manifest allows ^1.3.0
+            self.assertEqual(json.load(manifest)["version"], "1.3.0")
+
+        # the install runs in the developer's own directory, so it must leave their lockfile untouched -
+        # including the `ms` devDependency entry that `--omit=dev` keeps out of node_modules
+        with open(lockfile_path, "rb") as lockfile:
+            self.assertEqual(lockfile.read(), original_lockfile)
+
+        shutil.copy2(
+            os.path.join(self.temp_testdata_dir, "no-deps", "package.json"),
+            os.path.join(source_dir, "package.json"),
+        )
+
+        self.builder.build(
+            source_dir,
+            self.artifacts_dir,
+            self.scratch_dir,
+            os.path.join(source_dir, "package.json"),
+            runtime=runtime,
+            build_in_source=True,
+        )
+
+        self.assertNotIn("minimal-request-promise", set(os.listdir(source_node_modules)))
+        # still untouched with the lockfile now out of date: the manifest no longer lists the dependency
+        with open(lockfile_path, "rb") as lockfile:
+            self.assertEqual(lockfile.read(), original_lockfile)
+
+    @parameterized.expand(SUPPORTED_RUNTIMES)
+    def test_build_in_source_with_a_version_1_lockfile(self, runtime):
+        # npm 6 wrote lockfileVersion 1 and npm 7+ has to migrate it in memory before it can reify, which
+        # is a different write path from reifying a version 2 or 3 lockfile directly. The locked versions
+        # still have to win, and the developer's file still has to come back untouched - in its original
+        # format, not migrated in place.
+        source_dir = os.path.join(self.temp_testdata_dir, "npm-deps-with-v1-lockfile")
+        lockfile_path = os.path.join(source_dir, "package-lock.json")
+        with open(lockfile_path, "rb") as lockfile:
+            original_lockfile = lockfile.read()
+
+        self.builder.build(
+            source_dir,
+            self.artifacts_dir,
+            self.scratch_dir,
+            os.path.join(source_dir, "package.json"),
+            runtime=runtime,
+            build_in_source=True,
+        )
+
+        source_node_modules = os.path.join(source_dir, "node_modules")
+        installed_manifest = os.path.join(source_node_modules, "minimal-request-promise", "package.json")
+        with open(installed_manifest) as manifest:
+            # the lockfile pins 1.3.0 while the manifest allows ^1.3.0, so this version can only come
+            # from npm having read the version 1 lockfile
+            self.assertEqual(json.load(manifest)["version"], "1.3.0")
+
+        with open(lockfile_path, "rb") as lockfile:
+            self.assertEqual(lockfile.read(), original_lockfile)
+
+    @parameterized.expand(SUPPORTED_RUNTIMES)
+    def test_build_in_source_drops_already_installed_dev_dependencies(self, runtime):
+        # building in source installs into the developer's own directory, which normally already holds the
+        # dev dependencies their own `npm install` put there. Those must not reach the artifacts, which for
+        # this workflow are a symlink to the same node_modules.
+        source_dir = os.path.join(self.temp_testdata_dir, "npm-deps-with-lockfile")
+        source_node_modules = os.path.join(source_dir, "node_modules")
+
+        # the developer's own install: the `ms` devDependency is present before the build. Go through
+        # SubprocessNpm rather than a bare `npm`, since the executable is `npm.cmd` on Windows.
+        SubprocessNpm(OSUtils()).run(["install", "--silent", "--no-audit", "--no-fund"], cwd=source_dir)
+        self.assertIn("ms", set(os.listdir(source_node_modules)))
+
+        self.builder.build(
+            source_dir,
+            self.artifacts_dir,
+            self.scratch_dir,
+            os.path.join(source_dir, "package.json"),
+            runtime=runtime,
+            build_in_source=True,
+        )
+
+        installed = set(os.listdir(source_node_modules))
+        self.assertNotIn("ms", installed)
+        self.assertIn("minimal-request-promise", installed)
+        self.assertEqual(set(os.listdir(os.path.join(self.artifacts_dir, "node_modules"))), installed)
+
+    @parameterized.expand(SUPPORTED_RUNTIMES)
+    def test_build_in_source_with_a_local_dependency_and_a_lockfile(self, runtime):
+        # a lockfile written by a plain `npm install` records a file: dependency as a link entry
+        # ("resolved": "../npm-deps", "link": true), which is the tree --install-links exists to override.
+        # Reading a lockfile and passing --install-links used to be mutually exclusive here, because every
+        # build-in-source install ran with --no-package-lock, so this combination needs pinning: the local
+        # dependency has to land as a real directory, or the artifacts ship a symlink pointing outside them.
+        source_dir = os.path.join(self.temp_testdata_dir, "with-local-dependency-and-lockfile")
+        lockfile_path = os.path.join(source_dir, "package-lock.json")
+        with open(lockfile_path, "rb") as lockfile:
+            original_lockfile = lockfile.read()
+
+        self.builder.build(
+            source_dir,
+            self.artifacts_dir,
+            self.scratch_dir,
+            os.path.join(source_dir, "package.json"),
+            runtime=runtime,
+            build_in_source=True,
+        )
+
+        source_node_modules = os.path.join(source_dir, "node_modules")
+        local_dependency = os.path.join(source_node_modules, "local-dependency")
+        # --install-links wins over the lockfile's link entry: a real directory holding the package's files
+        self.assertFalse(os.path.islink(local_dependency))
+        self.assertTrue(os.path.isdir(local_dependency))
+        self.assertTrue(os.path.isfile(os.path.join(local_dependency, "included.js")))
+
+        installed_manifest = os.path.join(source_node_modules, "minimal-request-promise", "package.json")
+        with open(installed_manifest) as manifest:
+            # the lockfile pins 1.3.0 while the manifest allows ^1.3.0, so the lockfile was read even
+            # though --install-links was also in effect
+            self.assertEqual(json.load(manifest)["version"], "1.3.0")
+
+        with open(lockfile_path, "rb") as lockfile:
+            self.assertEqual(lockfile.read(), original_lockfile)
+
     @parameterized.expand(SUPPORTED_RUNTIMES)
     def test_build_in_source_with_removed_dependencies(self, runtime):
         # run a build with default requirements and confirm dependencies are downloaded

tests/integration/workflows/nodejs_npm/testdata/npm-deps-with-lockfile/excluded.js

@@ -0,0 +1,2 @@
+//excluded
+const x = 1;

tests/integration/workflows/nodejs_npm/testdata/npm-deps-with-lockfile/included.js

@@ -0,0 +1,2 @@
+//included
+const x = 1;

tests/integration/workflows/nodejs_npm/testdata/npm-deps-with-lockfile/package.json

@@ -0,0 +1,18 @@
+{
+  "name": "npmdepswithlockfile",
+  "version": "1.0.0",
+  "description": "",
+  "files": [
+    "included.js"
+  ],
+  "keywords": [],
+  "author": "",
+  "license": "APACHE2.0",
+  "main": "included.js",
+  "dependencies": {
+    "minimal-request-promise": "^1.3.0"
+  },
+  "devDependencies": {
+    "ms": "^2.1.3"
+  }
+}

tests/integration/workflows/nodejs_npm/testdata/npm-deps-with-v1-lockfile/excluded.js

@@ -0,0 +1,2 @@
+//excluded
+const x = 1;

tests/integration/workflows/nodejs_npm/testdata/npm-deps-with-v1-lockfile/included.js

@@ -0,0 +1,2 @@
+//included
+const x = 1;

tests/integration/workflows/nodejs_npm/testdata/npm-deps-with-v1-lockfile/package.json

@@ -0,0 +1,18 @@
+{
+  "name": "npmdepswithv1lockfile",
+  "version": "1.0.0",
+  "description": "",
+  "files": [
+    "included.js"
+  ],
+  "keywords": [],
+  "author": "",
+  "license": "APACHE2.0",
+  "main": "included.js",
+  "dependencies": {
+    "minimal-request-promise": "^1.3.0"
+  },
+  "devDependencies": {
+    "ms": "^2.1.3"
+  }
+}