From 92908631abb4e708aef6bfd731bafac8c4b9d494 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 ++++++++++++ 3 files changed, 98 insertions(+), 4 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 bd67593f993e07..cf9afb741aab85 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, @@ -443,9 +444,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]; @@ -457,7 +461,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. @@ -484,7 +488,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' } }), +);