From 4aab4bc930baebe1a7091cfccd3109d6fb3099b1 Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Mon, 28 Sep 2026 18:04:32 +0000 Subject: [PATCH 1/2] fix: link nodejs artifacts to the hoisted dependencies in a workspaces monorepo npm hoists a workspace package's dependencies to the monorepo root, so node_modules never appears beside the function and the artifacts link found no source at all - silently producing a deployment package with no dependencies. Ask npm where its project root is, the same question the lockfile lookup asks, and link the artifacts there. Only a project root outside the install directory redirects the link, so anything that is not a workspace member keeps the path it had. Refs aws/aws-lambda-builders#933 --- .../workflows/nodejs_npm/npm.py | 31 +++++++ .../workflows/nodejs_npm/workflow.py | 59 ++++++++---- .../workflows/nodejs_npm/test_nodejs_npm.py | 43 +++++++++ .../endpoints/fn/excluded.js | 1 + .../endpoints/fn/included.js | 3 + .../endpoints/fn/package.json | 11 +++ .../workspaces-monorepo/package-lock.json | 45 ++++++++++ .../testdata/workspaces-monorepo/package.json | 10 +++ .../packages/shared/index.js | 1 + .../packages/shared/package.json | 7 ++ tests/unit/workflows/nodejs_npm/test_npm.py | 44 +++++++++ .../workflows/nodejs_npm/test_workflow.py | 90 +++++++++++++++++-- 12 files changed, 320 insertions(+), 25 deletions(-) create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/excluded.js create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/included.js create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/package.json create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package.json create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/index.js create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py index 472d3df4e..a85d34ca4 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/npm.py +++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py @@ -3,6 +3,7 @@ """ import logging +from typing import Dict, Optional from aws_lambda_builders.workflows.nodejs_npm.exceptions import NpmExecutionError @@ -33,6 +34,36 @@ def __init__(self, osutils, npm_exe=None): npm_exe = "npm" self.npm_exe = npm_exe + self._project_root_cache: Dict[str, Optional[str]] = {} + + def resolve_project_root(self, cwd: str) -> Optional[str]: + """ + Ask npm which directory it treats as the project root when it runs in ``cwd``, caching the answer. + + `npm prefix` walks up to the nearest directory holding a package.json, which for a workspace + package is the monorepo root - where npm keeps the single lockfile and hoists node_modules to - + and is the directory itself for any other package. Both the lockfile lookup that selects the + install command and the link step that points the artifacts at the installed dependencies need + that answer for the same directory, so it is resolved once per directory rather than per caller. + + Parameters + ---------- + cwd : str + the directory npm will run in + + Returns + ------- + Optional[str] + npm's project root, or None when npm could not be asked + """ + if cwd not in self._project_root_cache: + try: + self._project_root_cache[cwd] = self.run(["prefix"], cwd=cwd).strip() + except NpmExecutionError as ex: + LOG.debug("NODEJS could not resolve the npm project root of %s: %s", cwd, ex) + self._project_root_cache[cwd] = None + + return self._project_root_cache[cwd] def run(self, args, cwd=None): """ diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py index 2933af7ce..757bcf236 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py +++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py @@ -68,6 +68,9 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim osutils = OSUtils() self.osutils = osutils + # where the install leaves node_modules; resolved from npm once the install directory is known + self._installed_dependencies_dir = os.path.join(source_dir, "node_modules") + if not osutils.file_exists(manifest_path): LOG.warning("package.json file not found. Continuing the build without dependencies.") self.actions = [CopySourceAction(source_dir, artifacts_dir, excludes=self.EXCLUDED_FILES)] @@ -106,17 +109,19 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim is_building_in_source = False self.build_dir = self._select_build_dir(build_in_source=False) + # run npm install in the directory where the manifest (package.json) exists if customer is building + # in source, and manifest directory is different from source. + # This will let NPM find the local dependencies that are defined in the manifest file (they are + # usually defined as relative to the manifest location, and that is why we run `npm install` in the + # manifest directory instead of source directory). + # If customer is not building in source, so it is ok to run `npm install` in the build + # directory (the artifacts directory in this case), as the local dependencies are not supported. + install_dir = self.manifest_dir if is_building_in_source and is_external_manifest else self.build_dir + self.actions.append( NodejsNpmWorkflow.get_install_action( source_dir=source_dir, - # run npm install in the directory where the manifest (package.json) exists if customer is building - # in source, and manifest directory is different from source. - # This will let NPM find the local dependencies that are defined in the manifest file (they are - # usually defined as relative to the manifest location, and that is why we run `npm install` in the - # manifest directory instead of source directory). - # If customer is not building in source, so it is ok to run `npm install` in the build - # directory (the artifacts directory in this case), as the local dependencies are not supported. - install_dir=self.manifest_dir if is_building_in_source and is_external_manifest else self.build_dir, + install_dir=install_dir, subprocess_npm=subprocess_npm, osutils=osutils, build_options=self.options, @@ -127,7 +132,7 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim self.actions.append( NodejsNpmTestAction( - install_dir=self.manifest_dir if is_building_in_source and is_external_manifest else self.build_dir, + install_dir=install_dir, subprocess_npm=subprocess_npm, ) ) @@ -142,6 +147,31 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim ) if self.download_dependencies and is_building_in_source: + # The artifacts link has to point at the node_modules npm actually creates, which is not always + # inside the install directory: in an npm workspaces monorepo npm hoists to the monorepo root, so + # nothing appears beside the function and this link found no source at all, silently producing + # artifacts with no dependencies (aws/aws-lambda-builders#933). Ask npm where its project root is + # - the same question the lockfile lookup asks, answered from one cached `npm prefix`. + # + # Only a project root OUTSIDE the install directory redirects the link. npm reports the install + # directory itself for anything that is not a workspace member, and then this leaves the source + # exactly as it was, including the external-manifest path that links through the source tree. + # + # normcase because the two sides come from different places - npm's stdout and this build's + # own path - and on Windows two spellings that differ only in case, a drive letter included, + # name the same directory. Treating those as different roots would redirect the link for a + # project that is not a workspace member at all. It is a no-op off Windows. + project_root = subprocess_npm.resolve_project_root(install_dir) + if project_root and os.path.normcase(os.path.realpath(project_root)) != os.path.normcase( + os.path.realpath(install_dir) + ): + LOG.debug( + "NODEJS npm installs into %s rather than %s; linking the artifacts to the hoisted dependencies", + project_root, + install_dir, + ) + self._installed_dependencies_dir = os.path.join(project_root, "node_modules") + self.actions += self._actions_for_linking_source_dependencies_to_artifacts # if no dependencies dir, just cleanup artifacts and we're done @@ -175,11 +205,8 @@ 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, and the monorepo flag - # does not cover it: 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)] + return [LinkSinglePathAction(source=self._installed_dependencies_dir, dest=artifact_dependencies_path)] @property def _actions_for_updating_dependencies_dir(self): @@ -318,11 +345,9 @@ def get_lockfile_path(install_dir: str, subprocess_npm: SubprocessNpm, osutils: 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: + project_root = subprocess_npm.resolve_project_root(install_dir) + if project_root is None: # 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 # npm's own precedence: where a project root holds both, npm reads npm-shrinkwrap.json and ignores diff --git a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py index 440aa7cfd..90b44e4fb 100644 --- a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py +++ b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py @@ -3,6 +3,7 @@ import logging import os import shutil +import subprocess import tempfile from unittest import TestCase, mock @@ -478,6 +479,48 @@ def test_build_in_source_with_a_local_dependency_and_a_lockfile(self, runtime): with open(lockfile_path, "rb") as lockfile: self.assertEqual(lockfile.read(), original_lockfile) + @parameterized.expand(SUPPORTED_RUNTIMES) + def test_build_in_source_in_workspaces_monorepo_links_the_hoisted_dependencies(self, runtime): + # npm hoists a workspace package's dependencies to the monorepo root, so node_modules never + # appears beside the function. This workflow ships node_modules rather than bundling it, so the + # artifacts have to reach the directory npm actually used. + monorepo_dir = os.path.join(self.temp_testdata_dir, "workspaces-monorepo") + source_dir = os.path.join(monorepo_dir, "endpoints", "fn") + + self.builder.build( + source_dir, + self.artifacts_dir, + self.scratch_dir, + os.path.join(source_dir, "package.json"), + runtime=runtime, + build_in_source=True, + ) + + # npm hoisted to the monorepo root and left nothing beside the function + self.assertFalse(os.path.exists(os.path.join(source_dir, "node_modules"))) + + artifacts_node_modules = os.path.join(self.artifacts_dir, "node_modules") + self.assertTrue(os.path.exists(artifacts_node_modules), "the artifacts have no node_modules at all") + + installed = set(os.listdir(artifacts_node_modules)) + self.assertIn("minimal-request-promise", installed) + self.assertIn("@nodejs-workspaces-monorepo", installed) + + # the locked version won, not the newest one the range allows + installed_manifest = os.path.join(artifacts_node_modules, "minimal-request-promise", "package.json") + with open(installed_manifest) as manifest: + self.assertEqual(json.load(manifest)["version"], "1.3.0") + + # the handler's own requires resolve from the artifacts directory - the property that makes this + # a deployable package rather than a directory that merely holds the right names + require_handler = subprocess.run( + ["node", "-e", "require('./included.js')"], + cwd=self.artifacts_dir, + capture_output=True, + text=True, + ) + self.assertEqual(require_handler.returncode, 0, require_handler.stderr) + @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 diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/excluded.js b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/excluded.js new file mode 100644 index 000000000..ccbb650af --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/excluded.js @@ -0,0 +1 @@ +//excluded diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/included.js b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/included.js new file mode 100644 index 000000000..c480a20df --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/included.js @@ -0,0 +1,3 @@ +const shared = require('@nodejs-workspaces-monorepo/shared'); +require('minimal-request-promise'); +exports.handler = async () => shared; diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/package.json b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/package.json new file mode 100644 index 000000000..b6dd52673 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/fn/package.json @@ -0,0 +1,11 @@ +{ + "name": "@nodejs-workspaces-monorepo/fn", + "version": "1.0.0", + "license": "APACHE2.0", + "main": "included.js", + "files": ["included.js"], + "dependencies": { + "@nodejs-workspaces-monorepo/shared": "*", + "minimal-request-promise": "^1.3.0" + } +} diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json new file mode 100644 index 000000000..168f06fc3 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json @@ -0,0 +1,45 @@ +{ + "name": "nodejs-workspaces-monorepo", + "version": "1.0.0", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "nodejs-workspaces-monorepo", + "version": "1.0.0", + "license": "APACHE2.0", + "workspaces": [ + "endpoints/*", + "packages/*" + ] + }, + "endpoints/fn": { + "name": "@nodejs-workspaces-monorepo/fn", + "version": "1.0.0", + "license": "APACHE2.0", + "dependencies": { + "@nodejs-workspaces-monorepo/shared": "*", + "minimal-request-promise": "^1.3.0" + } + }, + "node_modules/@nodejs-workspaces-monorepo/fn": { + "resolved": "endpoints/fn", + "link": true + }, + "node_modules/@nodejs-workspaces-monorepo/shared": { + "resolved": "packages/shared", + "link": true + }, + "node_modules/minimal-request-promise": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/minimal-request-promise/-/minimal-request-promise-1.3.0.tgz", + "integrity": "sha512-eaD7GFjLCG9glxI1UOXqsKjAUAau9JIMUAG+39sT/MSqJgGP3AJuYjAyEvhgYjSBiK+ROU5cPoaiwx/C6OV2uw==", + "license": "MIT" + }, + "packages/shared": { + "name": "@nodejs-workspaces-monorepo/shared", + "version": "1.0.0", + "license": "APACHE2.0" + } + } +} diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package.json b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package.json new file mode 100644 index 000000000..6b3f3fa85 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package.json @@ -0,0 +1,10 @@ +{ + "name": "nodejs-workspaces-monorepo", + "version": "1.0.0", + "private": true, + "license": "APACHE2.0", + "workspaces": [ + "endpoints/*", + "packages/*" + ] +} diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/index.js b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/index.js new file mode 100644 index 000000000..961e0775f --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/index.js @@ -0,0 +1 @@ +module.exports = 'from the workspace package'; diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json new file mode 100644 index 000000000..6d6cdff53 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json @@ -0,0 +1,7 @@ +{ + "name": "@nodejs-workspaces-monorepo/shared", + "version": "1.0.0", + "license": "APACHE2.0", + "main": "index.js", + "files": ["index.js"] +} diff --git a/tests/unit/workflows/nodejs_npm/test_npm.py b/tests/unit/workflows/nodejs_npm/test_npm.py index 3a1ab0508..c1afdeb07 100644 --- a/tests/unit/workflows/nodejs_npm/test_npm.py +++ b/tests/unit/workflows/nodejs_npm/test_npm.py @@ -80,3 +80,47 @@ def test_raises_ValueError_if_args_empty(self): self.under_test.run([]) self.assertEqual(raised.exception.args[0], "requires at least one arg") + + +class TestSubprocessNpmResolveProjectRoot(TestCase): + @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") + def setUp(self, OSUtilMock): + self.osutils = OSUtilMock.return_value + self.osutils.pipe = "PIPE" + self.under_test = SubprocessNpm(self.osutils, npm_exe="npm") + + def test_asks_npm_for_the_prefix_and_strips_the_trailing_newline(self): + self.osutils.popen.side_effect = [FakePopen(out=b"/repo\n")] + + self.assertEqual(self.under_test.resolve_project_root("/repo/endpoints/a"), "/repo") + self.osutils.popen.assert_called_with(["npm", "prefix"], cwd="/repo/endpoints/a", stderr="PIPE", stdout="PIPE") + + def test_asks_npm_once_per_directory(self): + # both the lockfile lookup and the artifacts link need this answer for the same directory, and + # every extra call is another npm process in a build that already runs one per function + self.osutils.popen.side_effect = [FakePopen(out=b"/repo\n")] + + self.assertEqual(self.under_test.resolve_project_root("/repo/endpoints/a"), "/repo") + self.assertEqual(self.under_test.resolve_project_root("/repo/endpoints/a"), "/repo") + + self.assertEqual(self.osutils.popen.call_count, 1) + + def test_asks_again_for_a_different_directory(self): + self.osutils.popen.side_effect = [FakePopen(out=b"/repo\n"), FakePopen(out=b"/other\n")] + + self.assertEqual(self.under_test.resolve_project_root("/repo/endpoints/a"), "/repo") + self.assertEqual(self.under_test.resolve_project_root("/other"), "/other") + + def test_returns_none_when_npm_fails(self): + self.osutils.popen.side_effect = [FakePopen(err=b"boom!", retcode=1)] + + self.assertIsNone(self.under_test.resolve_project_root("/repo/endpoints/a")) + + def test_caches_the_failure_too(self): + # a second npm process would fail the same way; the caller falls back either way + self.osutils.popen.side_effect = [FakePopen(err=b"boom!", retcode=1)] + + self.assertIsNone(self.under_test.resolve_project_root("/repo/endpoints/a")) + self.assertIsNone(self.under_test.resolve_project_root("/repo/endpoints/a")) + + self.assertEqual(self.osutils.popen.call_count, 1) diff --git a/tests/unit/workflows/nodejs_npm/test_workflow.py b/tests/unit/workflows/nodejs_npm/test_workflow.py index 51268c3fc..e043ce9c6 100644 --- a/tests/unit/workflows/nodejs_npm/test_workflow.py +++ b/tests/unit/workflows/nodejs_npm/test_workflow.py @@ -95,8 +95,9 @@ def test_workflow_sets_up_npm_actions_with_download_dependencies_without_depende @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.get_lockfile_path") @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links") + @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm.resolve_project_root") def test_workflow_sets_up_npm_actions_with_download_dependencies_without_dependencies_dir_external_manifest_and_build_in_source( - self, can_use_links_mock, get_lockfile_path_mock + self, resolve_project_root_mock, can_use_links_mock, get_lockfile_path_mock ): can_use_links_mock.return_value = True get_lockfile_path_mock.return_value = os.path.join("not_source", "package-lock.json") @@ -105,6 +106,9 @@ def test_workflow_sets_up_npm_actions_with_download_dependencies_without_depende self.osutils.file_exists.return_value = True self.osutils.file_exists.side_effect = [True, False, False] + # npm reports the install directory itself as its project root for anything that is not a + # workspace member, which is what leaves the artifacts link pointing at the source tree + resolve_project_root_mock.return_value = "not_source" workflow = NodejsNpmWorkflow( "source", @@ -338,12 +342,19 @@ def test_build_in_source_without_download_dependencies_and_without_dependencies_ @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.get_lockfile_path") @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links") - def test_build_in_source_with_download_dependencies(self, can_use_links_mock, get_lockfile_path_mock): + @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm.resolve_project_root") + def test_build_in_source_with_download_dependencies( + self, resolve_project_root_mock, can_use_links_mock, get_lockfile_path_mock + ): can_use_links_mock.return_value = True get_lockfile_path_mock.return_value = os.path.join("source", "package-lock.json") source_dir = "source" artifacts_dir = "artifacts" + # npm reports the install directory itself as its project root for anything that is not a + # workspace member, which is what leaves the artifacts link pointing at the source tree + resolve_project_root_mock.return_value = source_dir + workflow = NodejsNpmWorkflow( source_dir=source_dir, artifacts_dir=artifacts_dir, @@ -368,6 +379,65 @@ def test_build_in_source_with_download_dependencies(self, can_use_links_mock, ge self.assertEqual(workflow.actions[5]._dest, os.path.join(artifacts_dir, "node_modules")) self.assertIsInstance(workflow.actions[6], NodejsNpmrcCleanUpAction) + @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.get_lockfile_path") + @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links") + @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm.resolve_project_root") + def test_build_in_source_links_artifacts_to_the_hoisted_dependencies_in_a_workspaces_monorepo( + self, resolve_project_root_mock, can_use_links_mock, get_lockfile_path_mock + ): + # npm hoists a workspace package's dependencies to the monorepo root it reports as its project + # root, so that - not the function directory - is where the artifacts have to be linked + can_use_links_mock.return_value = True + get_lockfile_path_mock.return_value = os.path.join("monorepo", "package-lock.json") + resolve_project_root_mock.return_value = "monorepo" + self.osutils.dirname.return_value = os.path.join("monorepo", "endpoints", "a") + + workflow = NodejsNpmWorkflow( + source_dir=os.path.join("monorepo", "endpoints", "a"), + artifacts_dir="artifacts", + scratch_dir="scratch_dir", + manifest_path=os.path.join("monorepo", "endpoints", "a", "manifest"), + osutils=self.osutils, + build_in_source=True, + ) + + links = [ + action + for action in workflow.actions + if isinstance(action, LinkSinglePathAction) and action._dest == os.path.join("artifacts", "node_modules") + ] + self.assertEqual(len(links), 1) + self.assertEqual(links[0]._source, os.path.join("monorepo", "node_modules")) + + @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.get_lockfile_path") + @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links") + @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm.resolve_project_root") + def test_build_in_source_links_artifacts_to_the_source_when_npm_cannot_be_asked( + self, resolve_project_root_mock, can_use_links_mock, get_lockfile_path_mock + ): + # no answer from npm is no evidence the dependencies went anywhere else, so keep the old source + can_use_links_mock.return_value = True + get_lockfile_path_mock.return_value = None + resolve_project_root_mock.return_value = None + + source_dir = "source" + workflow = NodejsNpmWorkflow( + source_dir=source_dir, + artifacts_dir="artifacts", + scratch_dir="scratch_dir", + manifest_path="source/manifest", + osutils=self.osutils, + build_in_source=True, + ) + + links = [ + action + for action in workflow.actions + if isinstance(action, LinkSinglePathAction) and action._dest == os.path.join("artifacts", "node_modules") + ] + self.assertEqual(len(links), 1) + self.assertEqual(links[0]._source, os.path.join(source_dir, "node_modules")) + @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.get_lockfile_path") @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links") def test_build_in_source_without_lockfile_keeps_updating_dependencies( @@ -395,14 +465,19 @@ def test_build_in_source_without_lockfile_keeps_updating_dependencies( @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.get_lockfile_path") @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links") + @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm.resolve_project_root") def test_build_in_source_with_download_dependencies_and_dependencies_dir( - self, can_use_links_mock, get_lockfile_path_mock + self, resolve_project_root_mock, can_use_links_mock, get_lockfile_path_mock ): can_use_links_mock.return_value = True get_lockfile_path_mock.return_value = os.path.join("source", "package-lock.json") source_dir = "source" artifacts_dir = "artifacts" + # npm reports the install directory itself as its project root for anything that is not a + # workspace member, which is what leaves the artifacts link pointing at the source tree + resolve_project_root_mock.return_value = source_dir + workflow = NodejsNpmWorkflow( source_dir=source_dir, artifacts_dir=artifacts_dir, @@ -531,8 +606,7 @@ def _touch(self, *path_parts): return file_path def _npm_prefix_is(self, *path_parts): - # npm prints the project root with a trailing newline - self.subprocess_npm.run.return_value = os.path.join(self.tmp_dir, *path_parts) + os.linesep + self.subprocess_npm.resolve_project_root.return_value = os.path.join(self.tmp_dir, *path_parts) def _lockfile_path(self, install_dir_parts): return NodejsNpmWorkflow.get_lockfile_path( @@ -552,13 +626,13 @@ def _install_action(self, source_dir, install_dir, experimental_flags=("experime experimental_flags=list(experimental_flags), ) - def test_asks_npm_for_the_project_root(self): + def test_asks_npm_for_the_project_root_of_the_install_dir(self): self._touch("fn", "package.json") self._npm_prefix_is("fn") self._lockfile_path(["fn"]) - self.subprocess_npm.run.assert_called_with(["prefix"], cwd=os.path.join(self.tmp_dir, "fn")) + self.subprocess_npm.resolve_project_root.assert_called_with(os.path.join(self.tmp_dir, "fn")) def test_without_the_flag_a_lockfile_is_ignored_and_the_update_runs(self): # the rollout guarantee: a project with a lockfile npm would read still takes the update every @@ -625,7 +699,7 @@ def test_returns_none_when_the_project_root_has_no_lockfile(self): def test_returns_none_when_npm_cannot_report_the_project_root(self): self._touch("fn", "package.json") self._touch("fn", "package-lock.json") - self.subprocess_npm.run.side_effect = NpmExecutionError(message="boom!") + self.subprocess_npm.resolve_project_root.return_value = None self.assertIsNone(self._lockfile_path(["fn"])) From 1c026ca3e88c44a1eb9ba48404eef1b0dc96da96 Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Mon, 28 Sep 2026 18:44:38 +0000 Subject: [PATCH 2/2] fix: ship only the function's own dependencies from a hoisted monorepo tree Linking the monorepo root's whole node_modules into the artifacts also ships every sibling function's dependencies, because npm hoists them all into one directory. Ask npm which packages this function resolves (npm ls --all --parseable --omit=dev) and link those under their own names. A workspace dependency is reported as its source directory, whose basename is not the package name, so the name comes from its manifest. A nested copy that a dependency pins to another version stays inside that dependency rather than being hoisted, where it would shadow the top-level version. When npm cannot answer, link the whole installed tree: an over-complete node_modules still runs, an empty one does not. Refs aws/aws-lambda-builders#933 --- .../workflows/nodejs_npm/actions.py | 255 ++++++++++++++++++ .../workflows/nodejs_npm/npm.py | 32 ++- .../workflows/nodejs_npm/workflow.py | 34 ++- .../workflows/nodejs_npm/test_nodejs_npm.py | 17 ++ .../endpoints/other/included.js | 1 + .../endpoints/other/package.json | 10 + .../workspaces-monorepo/package-lock.json | 18 ++ .../unit/workflows/nodejs_npm/test_actions.py | 204 +++++++++++++- .../workflows/nodejs_npm/test_workflow.py | 58 +++- 9 files changed, 611 insertions(+), 18 deletions(-) create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/other/included.js create mode 100644 tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/other/package.json diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py index 109366283..8fdfa2369 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/actions.py +++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py @@ -4,14 +4,20 @@ import logging import os +import re from typing import Optional +from aws_lambda_builders import utils from aws_lambda_builders.actions import ActionFailedError, BaseAction, Purpose from aws_lambda_builders.utils import extract_tarfile from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError, SubprocessNpm LOG = logging.getLogger(__name__) +# A package name as node_modules can spell it: `pkg` or `@scope/pkg`, and nothing else. No separator +# beyond the single scope slash, so no traversal, no absolute path and no drive letter can get through. +NODE_MODULES_PACKAGE_NAME = re.compile(r"^(?:@[^/\\:]+/)?[^.@/\\:][^/\\:]*$") + class NodejsNpmPackAction(BaseAction): """ @@ -374,3 +380,252 @@ def execute(self): except NpmExecutionError as ex: raise ActionFailedError(str(ex)) + + +class NodejsNpmLinkDependencyClosureAction(BaseAction): + """ + A Lambda Builder Action that links only this function's own dependencies into the artifacts directory. + + Used when npm installed somewhere other than the function's directory, which is what npm does for a + workspaces monorepo: it hoists every workspace package's dependencies into one node_modules at the + monorepo root. Linking that whole directory would ship every sibling function's dependencies too, so + this asks npm which packages this function actually resolves and links those under their own names. + + Every name comes from the directory npm installed the package in, not from the package's own manifest - + see `_link_name` for the alias that makes those two differ, and for why a manifest value does not belong + in a path. When npm cannot report a closure at all, `_link_every_installed_package` links the installed + packages one by one rather than the whole tree in one link, which is the only form of "ship everything" + that is genuinely a superset of what the function resolves. + """ + + NAME = "NpmLinkDependencyClosure" + DESCRIPTION = "Linking this function's dependencies into the artifacts directory" + PURPOSE = Purpose.LINK_SOURCE + + def __init__(self, install_dir, project_root, artifacts_dir, subprocess_npm, osutils): + """ + Parameters + ---------- + install_dir : str + the directory npm ran in, whose project's closure is wanted + project_root : str + the directory npm installed into, linked whole if the closure cannot be resolved + artifacts_dir : str + an existing (writable) directory where node_modules is assembled + subprocess_npm : aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm + An instance of the NPM process wrapper + osutils : aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils + An instance of OS Utilities for file manipulation + """ + super(NodejsNpmLinkDependencyClosureAction, self).__init__() + self._install_dir = install_dir + self._project_root = project_root + self._artifacts_dir = artifacts_dir + self._subprocess_npm = subprocess_npm + self._osutils = osutils + + def execute(self): + closure = self._subprocess_npm.resolve_dependency_closure(self._install_dir) + destination = os.path.join(self._artifacts_dir, "node_modules") + + if closure is None: + self._link_every_installed_package(destination) + return + + for name, package_dir in self._packages_by_name(self._outermost_packages(closure)).items(): + self._link(package_dir, destination, name) + + def _link_every_installed_package(self, destination): + """ + Link every package npm installed, when npm could not say which ones this function resolves. + + AN OVERLAY, NOT ONE LINK FOR THE WHOLE DIRECTORY. Linking `project_root/node_modules` as the + artifacts' `node_modules` looks simpler and is not a superset of what the function resolves: in a + workspaces install a member's dependency that conflicts with the hoisted version is installed + under the member's OWN `node_modules`, and a single link to the root's directory cannot carry it. + The function would then resolve the root's different version, or nothing at all for a package that + was never hoisted. Per-entry links can carry both, with the function's own copy winning, which is + what node resolution does for the function anyway. + + It also removes the dangling link that one symlink produced: `create_symlink_or_copy` goes + straight to `os.symlink`, which succeeds against a missing target on POSIX, so a function with no + installed dependencies got a `node_modules` pointing nowhere - worse than the no-op it replaced, + because the copy that ships the artifacts then follows it. + """ + # The function's own directory LAST, so its entries replace the hoisted ones of the same name. + # Resolved into one mapping BEFORE anything is linked, because `create_symlink_or_copy` returns + # early when the destination already exists as a symlink - linking in order would keep the first + # of each name, which is the opposite of the precedence this needs. + chosen = {} + for directory in (self._project_root, self._install_dir): + tree = os.path.join(directory, "node_modules") + if not os.path.isdir(tree): + LOG.debug("NODEJS no dependencies installed in %s, nothing to link from there", tree) + continue + LOG.debug("NODEJS linking every package installed in %s into the artifacts", tree) + chosen.update(dict(self._installed_packages(tree))) + + if not chosen: + LOG.warning( + "No installed dependencies were found for %s; the artifacts will have no node_modules", + self._install_dir, + ) + for name, package_dir in chosen.items(): + self._link(package_dir, destination, name) + + def _installed_packages(self, tree): + """ + `(name, directory)` for every package directly inside one `node_modules`, scopes walked one level. + + Entries beginning with a dot are skipped: `.package-lock.json` and `.bin` are npm's bookkeeping + rather than packages, and the name rule rejects them anyway. + """ + for entry in sorted(os.listdir(tree)): + if entry.startswith("."): + continue + path = os.path.join(tree, entry) + if entry.startswith("@"): + if os.path.isdir(path): + for scoped in sorted(os.listdir(path)): + if not scoped.startswith("."): + yield f"{entry}/{scoped}", os.path.join(path, scoped) + continue + yield entry, path + + def _packages_by_name(self, package_dirs): + """ + Map each package to the one name it will be linked under, resolving same-name collisions. + + TWO PATHS CAN WANT ONE NAME, and until this existed whichever npm printed first won: + `create_symlink_or_copy` returns early when the destination already exists, so the other version + was silently dropped. The case is a function that pins its own version of a package the root also + hoists - `install_dir/node_modules/lodash` and `project_root/node_modules/lodash` - and neither is + nested inside the other, so both reach here. + + The function's own copy wins, because that is what node resolves for the function's code. The + hoisted version is not lost for anyone else: every other package is linked as a symlink into the + real tree, so a dependency resolving `lodash` from inside its real directory still walks up to the + root's copy. + + NOT FIXED BY TREATING `install_dir` AS A NESTING CONTAINER, which is the obvious reading of "the + nesting test never matches install_dir". `project_root` contains every hoisted package, so adding + it as a container would exclude all of them, and excluding the function's own pinned copy would + put back the bug this action exists to fix. + """ + chosen = {} + for package_dir in package_dirs: + name = self._link_name(package_dir) + if name in chosen: + if self._is_inside(package_dir, self._install_dir): + LOG.debug("NODEJS %s pins its own %s; it wins over %s", self._install_dir, name, chosen[name]) + chosen[name] = package_dir + else: + # Logged rather than silently resolved: two copies of one name that are not the + # function's own is npm reporting something this rule does not model. + LOG.warning( + "Two installed copies of %s claim the same name; keeping %s and ignoring %s", + name, + chosen[name], + package_dir, + ) + continue + chosen[name] = package_dir + return chosen + + def _link_name(self, package_dir): + """ + The name node must find this package under. + + FROM THE PATH, NOT THE MANIFEST, wherever npm installed it. npm supports aliases - + `"lodash4": "npm:lodash@^4.0.0"` installs at `node_modules/lodash4` while the manifest inside + still says `"name": "lodash"` - so reading the manifest renames the package and `require("lodash4")` + fails with `Cannot find module`, which is the very failure this action exists to prevent. The + directory npm chose is the name npm resolves, so the segments after the last `node_modules` + component are the answer, scope included. + + It also keeps an untrusted value out of a filesystem path. The manifest belongs to a third-party + dependency, and `"name": "../../../evil"` or `"/tmp/x"` joined onto the destination writes outside + the artifacts directory. + + The manifest is still the only source for a path that is NOT under node_modules - a workspace + dependency, which npm reports as its own source directory, whose basename need not be its name. + That value is validated like any other. + """ + segments = os.path.normpath(package_dir).split(os.sep) + for index in range(len(segments) - 1, -1, -1): + if os.path.normcase(segments[index]) == "node_modules": + return self._validated_name("/".join(segments[index + 1 :]), package_dir) + + try: + name = self._osutils.parse_json(os.path.join(package_dir, "package.json"))["name"] + except (OSError, ValueError, KeyError) as ex: + # a directory npm named but that carries no readable manifest cannot be placed under a + # name, and guessing one from the path is how a package lands where node will not find it + raise ActionFailedError(f"Cannot read the package name of {package_dir}: {ex}") + return self._validated_name(name, package_dir) + + @staticmethod + def _validated_name(name, package_dir): + if not isinstance(name, str) or not NODE_MODULES_PACKAGE_NAME.match(name): + raise ActionFailedError(f"{package_dir} claims the unusable package name {name!r}") + return name + + def _link(self, package_dir, destination, name): + link_path = os.path.join(destination, *name.split("/")) + # Belt and braces behind the name rule: that rule already makes an escape unconstructible, and + # this says so in a way that survives someone relaxing it. + # + # THE PARENT IS RESOLVED, NOT THE LINK ITSELF. realpath on link_path follows a link that is + # already there and reports its TARGET, which sits outside the artifacts by design - so checking + # that would refuse every legitimate re-link. Resolving the directory it will be created in, and + # appending the final component unresolved, asks the actual question: where will this land. + # NORMALISED, because commonpath compares components without interpreting them: a final `..` + # stays a literal component, so `/..` reads as contained and the link lands on the + # artifacts directory itself. Measured while mutation-testing this guard with the name rule + # relaxed - the only one of four hostile names it then still caught was `../../../evil`. + real_destination = os.path.realpath(destination) + landing = os.path.normpath( + os.path.join(os.path.realpath(os.path.dirname(link_path)), os.path.basename(link_path)) + ) + try: + contained = os.path.commonpath([real_destination, landing]) == real_destination + except ValueError: + # different drives on Windows, which is an escape by definition rather than an error to pass on + contained = False + if not contained: + raise ActionFailedError(f"{package_dir} would be linked outside the artifacts as {name!r}") + os.makedirs(os.path.dirname(link_path), exist_ok=True) + utils.create_symlink_or_copy(package_dir, link_path) + + @staticmethod + def _is_inside(path, directory): + parent = os.path.normcase(os.path.realpath(directory)) + return os.path.normcase(os.path.realpath(path)).startswith(parent + os.sep) + + def _outermost_packages(self, closure): + """ + Keep the packages that need their own entry in node_modules. + + npm reports the project itself, which is not one of its own dependencies, and it reports a nested + copy of a package that a dependency pins to a different version. A nested copy must stay where it + is - hoisting it would shadow the top-level version for every other caller - and it is already + reachable through the dependency that contains it, so only the outermost paths are linked. + """ + # Every comparison below goes through normcase, never the raw path: these paths are npm's + # spelling while project_root and install_dir are the build's, and on Windows two spellings + # differing only in case name the same directory. An unmatched project root would be linked as + # if it were one of its own dependencies, and an unrecognised nested copy would be hoisted to + # the top level, shadowing the version every other caller resolves. The original paths are what + # gets linked, so only the comparisons are normalised. No-op off Windows. + paths = [os.path.realpath(path) for path in closure] + excluded = {os.path.normcase(os.path.realpath(d)) for d in (self._project_root, self._install_dir)} + candidates = [path for path in paths if os.path.normcase(path) not in excluded] + + def is_nested_in_another(path): + key = os.path.normcase(path) + return any( + key != os.path.normcase(other) and key.startswith(os.path.normcase(other) + os.sep) + for other in candidates + ) + + return [path for path in candidates if not is_nested_in_another(path)] diff --git a/aws_lambda_builders/workflows/nodejs_npm/npm.py b/aws_lambda_builders/workflows/nodejs_npm/npm.py index a85d34ca4..4ee85038b 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/npm.py +++ b/aws_lambda_builders/workflows/nodejs_npm/npm.py @@ -3,7 +3,7 @@ """ import logging -from typing import Dict, Optional +from typing import Dict, List, Optional from aws_lambda_builders.workflows.nodejs_npm.exceptions import NpmExecutionError @@ -65,6 +65,36 @@ def resolve_project_root(self, cwd: str) -> Optional[str]: return self._project_root_cache[cwd] + def resolve_dependency_closure(self, cwd: str) -> Optional[List[str]]: + """ + Ask npm for every package the project in ``cwd`` actually resolves, production only. + + `npm ls --all --parseable --omit=dev` walks the installed tree and prints one absolute path per + resolved package. In a workspaces monorepo that answers a question the directory layout cannot: + which of the packages hoisted to the monorepo root belong to THIS function, and which belong to + a sibling. The paths are real paths, so a workspace dependency is reported as its own source + directory rather than as the link under node_modules. + + Parameters + ---------- + cwd : str + the directory whose project npm should report on + + Returns + ------- + Optional[List[str]] + the resolved package directories, or None when npm could not answer. npm exits non-zero for + any tree it considers incomplete (missing peer, invalid version) and its partial output is + not worth trusting, so callers fall back to shipping the whole installed tree instead. + """ + try: + output = self.run(["ls", "--all", "--parseable", "--omit=dev"], cwd=cwd) + except NpmExecutionError as ex: + LOG.debug("NODEJS could not resolve the dependency closure of %s: %s", cwd, ex) + return None + + return [line.strip() for line in output.splitlines() if line.strip()] + def run(self, args, cwd=None): """ Runs the action. diff --git a/aws_lambda_builders/workflows/nodejs_npm/workflow.py b/aws_lambda_builders/workflows/nodejs_npm/workflow.py index 757bcf236..573f525a3 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/workflow.py +++ b/aws_lambda_builders/workflows/nodejs_npm/workflow.py @@ -18,6 +18,7 @@ from aws_lambda_builders.workflows.nodejs_npm.actions import ( NodejsNpmCIAction, NodejsNpmInstallAction, + NodejsNpmLinkDependencyClosureAction, NodejsNpmLockFileCleanUpAction, NodejsNpmPackAction, NodejsNpmrcAndLockfileCopyAction, @@ -25,7 +26,7 @@ NodejsNpmTestAction, NodejsNpmUpdateAction, ) -from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError, SubprocessNpm +from aws_lambda_builders.workflows.nodejs_npm.npm import SubprocessNpm from aws_lambda_builders.workflows.nodejs_npm.utils import OSUtils, is_nodejs_monorepo_support_enabled LOG = logging.getLogger(__name__) @@ -68,9 +69,6 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim osutils = OSUtils() self.osutils = osutils - # where the install leaves node_modules; resolved from npm once the install directory is known - self._installed_dependencies_dir = os.path.join(source_dir, "node_modules") - if not osutils.file_exists(manifest_path): LOG.warning("package.json file not found. Continuing the build without dependencies.") self.actions = [CopySourceAction(source_dir, artifacts_dir, excludes=self.EXCLUDED_FILES)] @@ -161,18 +159,33 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim # own path - and on Windows two spellings that differ only in case, a drive letter included, # name the same directory. Treating those as different roots would redirect the link for a # project that is not a workspace member at all. It is a no-op off Windows. - project_root = subprocess_npm.resolve_project_root(install_dir) + # + # Opt-in while this rolls out: without experimentalNodejsMonorepo npm is not asked at all and + # the link stays where it was, which for a monorepo is the silent gap #933 describes. + project_root = ( + subprocess_npm.resolve_project_root(install_dir) + if is_nodejs_monorepo_support_enabled(self.experimental_flags) + else None + ) if project_root and os.path.normcase(os.path.realpath(project_root)) != os.path.normcase( os.path.realpath(install_dir) ): LOG.debug( - "NODEJS npm installs into %s rather than %s; linking the artifacts to the hoisted dependencies", + "NODEJS npm installs into %s rather than %s; linking this function's own dependencies", project_root, install_dir, ) - self._installed_dependencies_dir = os.path.join(project_root, "node_modules") - - self.actions += self._actions_for_linking_source_dependencies_to_artifacts + self.actions.append( + NodejsNpmLinkDependencyClosureAction( + install_dir=install_dir, + project_root=project_root, + artifacts_dir=artifacts_dir, + subprocess_npm=subprocess_npm, + osutils=osutils, + ) + ) + else: + self.actions += self._actions_for_linking_source_dependencies_to_artifacts # if no dependencies dir, just cleanup artifacts and we're done if not self.dependencies_dir: @@ -205,8 +218,9 @@ def _actions_for_cleanup(self): @property def _actions_for_linking_source_dependencies_to_artifacts(self): + 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=self._installed_dependencies_dir, dest=artifact_dependencies_path)] + return [LinkSinglePathAction(source=source_dependencies_path, dest=artifact_dependencies_path)] @property def _actions_for_updating_dependencies_dir(self): diff --git a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py index 90b44e4fb..c3929f2e2 100644 --- a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py +++ b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py @@ -487,6 +487,10 @@ def test_build_in_source_in_workspaces_monorepo_links_the_hoisted_dependencies(s monorepo_dir = os.path.join(self.temp_testdata_dir, "workspaces-monorepo") source_dir = os.path.join(monorepo_dir, "endpoints", "fn") + # the developer's own install, at the monorepo root, which is how a workspaces project is set up + # before `sam build` ever runs. It installs every endpoint's dependencies into one node_modules. + SubprocessNpm(OSUtils()).run(["install", "--silent", "--no-audit", "--no-fund"], cwd=monorepo_dir) + self.builder.build( source_dir, self.artifacts_dir, @@ -494,6 +498,7 @@ def test_build_in_source_in_workspaces_monorepo_links_the_hoisted_dependencies(s os.path.join(source_dir, "package.json"), runtime=runtime, build_in_source=True, + experimental_flags=["experimentalNodejsMonorepo"], ) # npm hoisted to the monorepo root and left nothing beside the function @@ -506,6 +511,18 @@ def test_build_in_source_in_workspaces_monorepo_links_the_hoisted_dependencies(s self.assertIn("minimal-request-promise", installed) self.assertIn("@nodejs-workspaces-monorepo", installed) + # npm hoists every workspace package's dependencies into the one node_modules it creates, so the + # sibling endpoint's `ms` is sitting right beside this function's own dependencies. It must not + # reach this function's artifacts. + self.assertIn("ms", set(os.listdir(os.path.join(monorepo_dir, "node_modules")))) + self.assertNotIn("ms", installed) + + # the workspace package this function depends on is reachable under its own name, which is not the + # name of the directory it lives in + self.assertTrue( + os.path.exists(os.path.join(artifacts_node_modules, "@nodejs-workspaces-monorepo", "shared", "index.js")) + ) + # the locked version won, not the newest one the range allows installed_manifest = os.path.join(artifacts_node_modules, "minimal-request-promise", "package.json") with open(installed_manifest) as manifest: diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/other/included.js b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/other/included.js new file mode 100644 index 000000000..eda64e76d --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/other/included.js @@ -0,0 +1 @@ +exports.handler = async () => require('ms'); diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/other/package.json b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/other/package.json new file mode 100644 index 000000000..cc0624fc4 --- /dev/null +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/endpoints/other/package.json @@ -0,0 +1,10 @@ +{ + "name": "@nodejs-workspaces-monorepo/other", + "version": "1.0.0", + "license": "APACHE2.0", + "main": "included.js", + "files": ["included.js"], + "dependencies": { + "ms": "^2.1.3" + } +} diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json index 168f06fc3..3fa3f05a6 100644 --- a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json @@ -22,10 +22,22 @@ "minimal-request-promise": "^1.3.0" } }, + "endpoints/other": { + "name": "@nodejs-workspaces-monorepo/other", + "version": "1.0.0", + "license": "APACHE2.0", + "dependencies": { + "ms": "^2.1.3" + } + }, "node_modules/@nodejs-workspaces-monorepo/fn": { "resolved": "endpoints/fn", "link": true }, + "node_modules/@nodejs-workspaces-monorepo/other": { + "resolved": "endpoints/other", + "link": true + }, "node_modules/@nodejs-workspaces-monorepo/shared": { "resolved": "packages/shared", "link": true @@ -36,6 +48,12 @@ "integrity": "sha512-eaD7GFjLCG9glxI1UOXqsKjAUAau9JIMUAG+39sT/MSqJgGP3AJuYjAyEvhgYjSBiK+ROU5cPoaiwx/C6OV2uw==", "license": "MIT" }, + "node_modules/ms": { + "version": "2.1.3", + "resolved": "https://registry.npmjs.org/ms/-/ms-2.1.3.tgz", + "integrity": "sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==", + "license": "MIT" + }, "packages/shared": { "name": "@nodejs-workspaces-monorepo/shared", "version": "1.0.0", diff --git a/tests/unit/workflows/nodejs_npm/test_actions.py b/tests/unit/workflows/nodejs_npm/test_actions.py index e648f020b..3ded102e8 100644 --- a/tests/unit/workflows/nodejs_npm/test_actions.py +++ b/tests/unit/workflows/nodejs_npm/test_actions.py @@ -1,10 +1,15 @@ import itertools +import json +import os +import shutil +import tempfile from unittest import TestCase -from unittest.mock import patch, call +from unittest.mock import MagicMock, patch, call from parameterized import parameterized from aws_lambda_builders.actions import ActionFailedError from aws_lambda_builders.workflows.nodejs_npm.actions import ( + NodejsNpmLinkDependencyClosureAction, NodejsNpmPackAction, NodejsNpmInstallAction, NodejsNpmrcAndLockfileCopyAction, @@ -14,6 +19,7 @@ NodejsNpmTestAction, ) from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError +from aws_lambda_builders.workflows.nodejs_npm.utils import OSUtils class TestNodejsNpmPackAction(TestCase): @@ -273,3 +279,199 @@ def test_raises_action_failed_when_npm_test_fails(self, SubprocessNpmMock): action.execute() self.assertEqual(raised.exception.args[0], "NPM Failed: boom!") + + +class TestNodejsNpmLinkDependencyClosureAction(TestCase): + """ + the action places real directories under names npm reports, so these tests use a real temporary tree + """ + + def setUp(self): + self.osutils = OSUtils() + self.tmp_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.tmp_dir, True) + self.subprocess_npm = MagicMock() + self.artifacts_dir = os.path.join(self.tmp_dir, "artifacts") + os.makedirs(self.artifacts_dir) + self.root = os.path.join(self.tmp_dir, "monorepo") + self.install_dir = os.path.join(self.root, "endpoints", "fn") + + def _package(self, relative_path, name): + path = os.path.join(self.root, *relative_path.split("/")) + os.makedirs(path, exist_ok=True) + with open(os.path.join(path, "package.json"), "w") as manifest: + json.dump({"name": name, "version": "1.0.0"}, manifest) + return path + + def _action(self): + return NodejsNpmLinkDependencyClosureAction( + install_dir=self.install_dir, + project_root=self.root, + artifacts_dir=self.artifacts_dir, + subprocess_npm=self.subprocess_npm, + osutils=self.osutils, + ) + + def _linked(self): + node_modules = os.path.join(self.artifacts_dir, "node_modules") + found = set() + for entry in os.listdir(node_modules): + if entry.startswith("@"): + found.update(f"{entry}/{scoped}" for scoped in os.listdir(os.path.join(node_modules, entry))) + else: + found.add(entry) + return found + + def test_links_only_this_function_s_dependencies(self): + self._package("endpoints/fn", "@mono/fn") + mine = self._package("node_modules/lodash", "lodash") + siblings = self._package("node_modules/ms", "ms") + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, mine] + + self._action().execute() + + self.assertEqual(self._linked(), {"lodash"}) + self.assertTrue(os.path.exists(siblings), "the sibling's package must stay where npm put it") + + def test_links_a_workspace_dependency_under_its_own_name(self): + # npm reports a workspace dependency as its source directory, whose basename is not the package name + self._package("endpoints/fn", "@mono/fn") + shared = self._package("packages/shared-impl", "@mono/shared") + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, shared] + + self._action().execute() + + self.assertEqual(self._linked(), {"@mono/shared"}) + + def test_leaves_a_nested_copy_inside_the_dependency_that_pins_it(self): + # hoisting a nested copy to the top level would shadow the top-level version for every caller + self._package("endpoints/fn", "@mono/fn") + dep = self._package("packages/dep", "@mono/dep") + nested = self._package("packages/dep/node_modules/lodash", "lodash") + hoisted = self._package("node_modules/lodash", "lodash") + self.subprocess_npm.resolve_dependency_closure.return_value = [ + self.root, + self.install_dir, + dep, + nested, + hoisted, + ] + + self._action().execute() + + self.assertEqual(self._linked(), {"@mono/dep", "lodash"}) + self.assertEqual( + os.path.realpath(os.path.join(self.artifacts_dir, "node_modules", "lodash")), + os.path.realpath(hoisted), + ) + + def test_links_the_whole_installed_tree_when_npm_cannot_be_asked(self): + # an over-complete node_modules still runs; an empty one does not + self._package("node_modules/lodash", "lodash") + self._package("node_modules/ms", "ms") + self.subprocess_npm.resolve_dependency_closure.return_value = None + + self._action().execute() + + self.assertEqual(self._linked(), {"lodash", "ms"}) + + def test_the_fallback_carries_the_function_s_own_non_hoisted_dependencies(self): + # the fallback is only sound if it is a SUPERSET: one link for project_root/node_modules is not, + # because a member's conflicting version is installed under the member's own node_modules + self._package("node_modules/lodash", "lodash") + pinned = self._package("endpoints/fn/node_modules/lodash", "lodash") + self._package("endpoints/fn/node_modules/only-mine", "only-mine") + self.subprocess_npm.resolve_dependency_closure.return_value = None + + self._action().execute() + + self.assertEqual(self._linked(), {"lodash", "only-mine"}) + self.assertEqual( + os.path.realpath(os.path.join(self.artifacts_dir, "node_modules", "lodash")), + os.path.realpath(pinned), + "the function's own copy is what its code resolves, so it wins over the hoisted one", + ) + + def test_the_fallback_links_nothing_rather_than_a_dangling_node_modules(self): + # os.symlink succeeds against a missing target, so linking an absent tree used to leave a + # node_modules pointing nowhere - worse than the no-op, because the artifacts copy then follows it + self.subprocess_npm.resolve_dependency_closure.return_value = None + + self._action().execute() + + self.assertFalse( + os.path.lexists(os.path.join(self.artifacts_dir, "node_modules")), + "nothing was installed, so there is nothing to point at", + ) + + def test_an_aliased_dependency_keeps_the_name_npm_installed_it_under(self): + # `"lodash4": "npm:lodash@^4.0.0"` installs at node_modules/lodash4 with a manifest still saying + # "lodash"; taking the manifest name breaks require("lodash4") - this action's own failure mode + self._package("endpoints/fn", "@mono/fn") + alias = self._package("node_modules/lodash4", "lodash") + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, alias] + + self._action().execute() + + self.assertEqual(self._linked(), {"lodash4"}) + + def test_the_function_s_own_pinned_copy_wins_a_name_it_shares_with_the_hoisted_one(self): + # neither path is nested inside the other, so both reach the link step and used to race: whichever + # npm printed first won, because create_symlink_or_copy returns early on an existing destination + self._package("endpoints/fn", "@mono/fn") + pinned = self._package("endpoints/fn/node_modules/lodash", "lodash") + hoisted = self._package("node_modules/lodash", "lodash") + for order in ([pinned, hoisted], [hoisted, pinned]): + shutil.rmtree(os.path.join(self.artifacts_dir, "node_modules"), ignore_errors=True) + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, *order] + + self._action().execute() + + self.assertEqual(self._linked(), {"lodash"}) + self.assertEqual( + os.path.realpath(os.path.join(self.artifacts_dir, "node_modules", "lodash")), + os.path.realpath(pinned), + f"the function's own copy must win whatever order npm reports ({order})", + ) + + def test_a_hostile_manifest_name_cannot_write_outside_the_artifacts(self): + # the name of a workspace dependency is the one value still read from a manifest, so it is the one + # that has to be refused rather than joined onto the destination + self._package("endpoints/fn", "@mono/fn") + for hostile in ("../../../evil", "/tmp/evil", "..", "a/b/c"): + shutil.rmtree(os.path.join(self.artifacts_dir, "node_modules"), ignore_errors=True) + shared = self._package("packages/shared-impl", hostile) + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, shared] + + with self.assertRaises(ActionFailedError) as raised: + self._action().execute() + + self.assertIn(repr(hostile), str(raised.exception)) + self.assertFalse( + os.path.exists(os.path.join(self.tmp_dir, "evil")), + f"{hostile} must not have created anything outside the artifacts", + ) + + def test_fails_loudly_when_a_workspace_dependency_has_no_manifest(self): + # outside node_modules the path carries no name, so the manifest is the only source and its + # absence has to stop the build rather than guess one from the directory + self._package("endpoints/fn", "@mono/fn") + no_manifest = os.path.join(self.root, "packages", "mystery-impl") + os.makedirs(no_manifest) + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, no_manifest] + + with self.assertRaises(ActionFailedError) as raised: + self._action().execute() + + self.assertIn("mystery-impl", str(raised.exception)) + + def test_a_package_under_node_modules_needs_no_manifest_at_all(self): + # the path already carries the name, so an unreadable manifest is no longer a reason to fail + self._package("endpoints/fn", "@mono/fn") + no_manifest = os.path.join(self.root, "node_modules", "mystery") + os.makedirs(no_manifest) + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, no_manifest] + + self._action().execute() + + self.assertEqual(self._linked(), {"mystery"}) diff --git a/tests/unit/workflows/nodejs_npm/test_workflow.py b/tests/unit/workflows/nodejs_npm/test_workflow.py index e043ce9c6..e06b4a178 100644 --- a/tests/unit/workflows/nodejs_npm/test_workflow.py +++ b/tests/unit/workflows/nodejs_npm/test_workflow.py @@ -18,6 +18,7 @@ from aws_lambda_builders.workflows.nodejs_npm.utils import OSUtils from aws_lambda_builders.workflows.nodejs_npm.workflow import NodejsNpmWorkflow from aws_lambda_builders.workflows.nodejs_npm.actions import ( + NodejsNpmLinkDependencyClosureAction, NodejsNpmPackAction, NodejsNpmInstallAction, NodejsNpmrcAndLockfileCopyAction, @@ -399,15 +400,60 @@ def test_build_in_source_links_artifacts_to_the_hoisted_dependencies_in_a_worksp manifest_path=os.path.join("monorepo", "endpoints", "a", "manifest"), osutils=self.osutils, build_in_source=True, + experimental_flags=["experimentalNodejsMonorepo"], ) - links = [ - action - for action in workflow.actions - if isinstance(action, LinkSinglePathAction) and action._dest == os.path.join("artifacts", "node_modules") + # a workspaces monorepo gets the closure action instead of a link to the whole installed tree, + # which would carry every sibling function's dependencies into this function's artifacts + closure_actions = [ + action for action in workflow.actions if isinstance(action, NodejsNpmLinkDependencyClosureAction) ] - self.assertEqual(len(links), 1) - self.assertEqual(links[0]._source, os.path.join("monorepo", "node_modules")) + self.assertEqual(len(closure_actions), 1) + self.assertEqual(closure_actions[0]._install_dir, os.path.join("monorepo", "endpoints", "a")) + self.assertEqual(closure_actions[0]._project_root, "monorepo") + self.assertEqual(closure_actions[0]._artifacts_dir, "artifacts") + self.assertFalse( + [ + action + for action in workflow.actions + if isinstance(action, LinkSinglePathAction) + and action._dest == os.path.join("artifacts", "node_modules") + ] + ) + + @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.get_lockfile_path") + @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links") + @patch("aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm.resolve_project_root") + def test_without_the_flag_a_monorepo_keeps_the_plain_link_and_npm_is_not_asked( + self, resolve_project_root_mock, can_use_links_mock, get_lockfile_path_mock + ): + # the rollout guarantee: the same monorepo shape without the flag takes the link every release so + # far has taken - the #933 gap - and npm is never asked where its project root is + can_use_links_mock.return_value = True + get_lockfile_path_mock.return_value = None + self.osutils.dirname.return_value = os.path.join("monorepo", "endpoints", "a") + + workflow = NodejsNpmWorkflow( + source_dir=os.path.join("monorepo", "endpoints", "a"), + artifacts_dir="artifacts", + scratch_dir="scratch_dir", + manifest_path=os.path.join("monorepo", "endpoints", "a", "manifest"), + osutils=self.osutils, + build_in_source=True, + ) + + resolve_project_root_mock.assert_not_called() + self.assertFalse( + [action for action in workflow.actions if isinstance(action, NodejsNpmLinkDependencyClosureAction)] + ) + self.assertTrue( + [ + action + for action in workflow.actions + if isinstance(action, LinkSinglePathAction) + and action._dest == os.path.join("artifacts", "node_modules") + ] + ) @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.get_lockfile_path") @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links")