From 9577a00f98833e755d706e859cfb8a782807b61c Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 29 Sep 2023 09:46:32 +0100 Subject: [PATCH] esm: fix cache collision on JSON files using file: URL PR-URL: https://github.com/nodejs/node/pull/49887 Fixes: https://github.com/nodejs/node/issues/49724 Reviewed-By: Geoffrey Booth Reviewed-By: LiviaMedeiros Reviewed-By: Jacob Smith Reviewed-By: Chemi Atlow --- lib/internal/modules/esm/translators.js | 10 ++- test/es-module/test-esm-json.mjs | 62 ++++++++++++++++++- test/es-module/test-esm-virtual-json.mjs | 30 +++++++++ test/fixtures/errors/force_colors.snapshot | 10 +-- ...urce_map_sourcemapping_url_string.snapshot | 2 +- 5 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 test/es-module/test-esm-virtual-json.mjs diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index cf13aadcb4645f..c9f7845a664cb7 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -11,6 +11,7 @@ const { SafeArrayIterator, SafeMap, SafeSet, + StringPrototypeIncludes, StringPrototypeReplaceAll, StringPrototypeSlice, StringPrototypeStartsWith, @@ -440,9 +441,12 @@ translators.set('json', function jsonStrategy(url, source) { debug(`Loading JSONModule ${url}`); const pathname = StringPrototypeStartsWith(url, 'file:') ? fileURLToPath(url) : null; + const shouldCheckAndPopulateCJSModuleCache = + // We want to involve the CJS loader cache only for `file:` URL with no search query and no hash. + pathname && !StringPrototypeIncludes(url, '?') && !StringPrototypeIncludes(url, '#'); let modulePath; let module; - if (pathname) { + if (shouldCheckAndPopulateCJSModuleCache) { modulePath = isWindows ? StringPrototypeReplaceAll(pathname, '/', '\\') : pathname; module = CJSModule._cache[modulePath]; @@ -454,7 +458,7 @@ translators.set('json', function jsonStrategy(url, source) { } } source = stringify(source); - if (pathname) { + if (shouldCheckAndPopulateCJSModuleCache) { // A require call could have been called on the same file during loading and // that resolves synchronously. To make sure we always return the identical // export, we have to check again if the module already exists or not. @@ -481,7 +485,7 @@ translators.set('json', function jsonStrategy(url, source) { err.message = errPath(url) + ': ' + err.message; throw err; } - if (pathname) { + if (shouldCheckAndPopulateCJSModuleCache) { CJSModule._cache[modulePath] = module; } cjsCache.set(url, module); diff --git a/test/es-module/test-esm-json.mjs b/test/es-module/test-esm-json.mjs index 2740c0097f77da..82232838b79150 100644 --- a/test/es-module/test-esm-json.mjs +++ b/test/es-module/test-esm-json.mjs @@ -2,7 +2,10 @@ import { spawnPromisified } from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; import assert from 'node:assert'; import { execPath } from 'node:process'; -import { describe, it } from 'node:test'; +import { describe, it, test } from 'node:test'; + +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import * as tmpdir from '../common/tmpdir.js'; import secret from '../fixtures/experimental.json' assert { type: 'json' }; @@ -21,4 +24,61 @@ describe('ESM: importing JSON', () => { assert.strictEqual(code, 0); assert.strictEqual(signal, null); }); + + test('should load different modules when the URL is different', async (t) => { + const root = tmpdir.fileURL(`./test-esm-json-${Math.random()}/`); + try { + await mkdir(root, { recursive: true }); + + await t.test('json', async () => { + let i = 0; + const url = new URL('./foo.json', root); + await writeFile(url, JSON.stringify({ id: i++ })); + const absoluteURL = await import(`${url}`, { + assert: { type: 'json' }, + }); + await writeFile(url, JSON.stringify({ id: i++ })); + const queryString = await import(`${url}?a=2`, { + assert: { type: 'json' }, + }); + await writeFile(url, JSON.stringify({ id: i++ })); + const hash = await import(`${url}#a=2`, { + assert: { type: 'json' }, + }); + await writeFile(url, JSON.stringify({ id: i++ })); + const queryStringAndHash = await import(`${url}?a=2#a=2`, { + assert: { type: 'json' }, + }); + + assert.notDeepStrictEqual(absoluteURL, queryString); + assert.notDeepStrictEqual(absoluteURL, hash); + assert.notDeepStrictEqual(queryString, hash); + assert.notDeepStrictEqual(absoluteURL, queryStringAndHash); + assert.notDeepStrictEqual(queryString, queryStringAndHash); + assert.notDeepStrictEqual(hash, queryStringAndHash); + }); + + await t.test('js', async () => { + let i = 0; + const url = new URL('./foo.mjs', root); + await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`); + const absoluteURL = await import(`${url}`); + await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`); + const queryString = await import(`${url}?a=1`); + await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`); + const hash = await import(`${url}#a=1`); + await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`); + const queryStringAndHash = await import(`${url}?a=1#a=1`); + + assert.notDeepStrictEqual(absoluteURL, queryString); + assert.notDeepStrictEqual(absoluteURL, hash); + assert.notDeepStrictEqual(queryString, hash); + assert.notDeepStrictEqual(absoluteURL, queryStringAndHash); + assert.notDeepStrictEqual(queryString, queryStringAndHash); + assert.notDeepStrictEqual(hash, queryStringAndHash); + }); + } finally { + await rm(root, { force: true, recursive: true }); + } + }); }); diff --git a/test/es-module/test-esm-virtual-json.mjs b/test/es-module/test-esm-virtual-json.mjs new file mode 100644 index 00000000000000..8ff185a428ef01 --- /dev/null +++ b/test/es-module/test-esm-virtual-json.mjs @@ -0,0 +1,30 @@ +import '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import { register } from 'node:module'; +import assert from 'node:assert'; + +async function resolve(referrer, context, next) { + const result = await next(referrer, context); + const url = new URL(result.url); + url.searchParams.set('randomSeed', Math.random()); + result.url = url.href; + return result; +} + +function load(url, context, next) { + if (context.importAssertions.type === 'json') { + return { + shortCircuit: true, + format: 'json', + source: JSON.stringify({ data: Math.random() }), + }; + } + return next(url, context); +} + +register(`data:text/javascript,export ${encodeURIComponent(resolve)};export ${encodeURIComponent(load)}`); + +assert.notDeepStrictEqual( + await import(fixtures.fileURL('empty.json'), { assert: { type: 'json' } }), + await import(fixtures.fileURL('empty.json'), { assert: { type: 'json' } }), +); diff --git a/test/fixtures/errors/force_colors.snapshot b/test/fixtures/errors/force_colors.snapshot index be1d45d0d8e8ba..8416731841187a 100644 --- a/test/fixtures/errors/force_colors.snapshot +++ b/test/fixtures/errors/force_colors.snapshot @@ -4,11 +4,11 @@ throw new Error('Should include grayed stack trace') Error: Should include grayed stack trace at Object. (/test*force_colors.js:1:7) - at Module._compile (node:internal*modules*cjs*loader:1241:14) - at Module._extensions..js (node:internal*modules*cjs*loader:1295:10) - at Module.load (node:internal*modules*cjs*loader:1091:32) - at Module._load (node:internal*modules*cjs*loader:938:12) - at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:83:12) + at Module._compile (node:internal*modules*cjs*loader:1406:14) + at Module._extensions..js (node:internal*modules*cjs*loader:1464:10) + at Module.load (node:internal*modules*cjs*loader:1239:32) + at Module._load (node:internal*modules*cjs*loader:1061:12) + at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:102:12)  at node:internal*main*run_main_module:23:47 Node.js * diff --git a/test/fixtures/source-map/output/source_map_sourcemapping_url_string.snapshot b/test/fixtures/source-map/output/source_map_sourcemapping_url_string.snapshot index 2c1e11eeb9eab1..97a0fc702652f3 100644 --- a/test/fixtures/source-map/output/source_map_sourcemapping_url_string.snapshot +++ b/test/fixtures/source-map/output/source_map_sourcemapping_url_string.snapshot @@ -1,3 +1,3 @@ Error: an exception. at Object. (*typescript-sourcemapping_url_string.ts:3:7) - at Module._compile (node:internal*modules*cjs*loader:1241:14) + at Module._compile (node:internal*modules*cjs*loader:1406:14)