esm: fix cache collision on JSON files using file: URL

PR-URL: nodejs/node#49887 Backport-PR-URL: nodejs/node#50669 Fixes: nodejs/node#49724 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
sercher · Apr 25, 2024 · a6caac0 · a6caac0
1 parent fd28b3e
commit a6caac0
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 4 deletions.
diff --git a/graal-nodejs/lib/internal/modules/esm/translators.js b/graal-nodejs/lib/internal/modules/esm/translators.js
@@ -11,6 +11,7 @@ const {
   SafeArrayIterator,
   SafeMap,
   SafeSet,
+  StringPrototypeIncludes,
   StringPrototypeReplaceAll,
   StringPrototypeSlice,
   StringPrototypeStartsWith,
@@ -265,9 +266,12 @@ translators.set('json', async 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];
@@ -279,7 +283,7 @@ translators.set('json', async 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.
@@ -305,7 +309,7 @@ translators.set('json', async function jsonStrategy(url, source) {
     err.message = errPath(url) + ': ' + err.message;
     throw err;
   }
-  if (pathname) {
+  if (shouldCheckAndPopulateCJSModuleCache) {
     CJSModule._cache[modulePath] = module;
   }
   return new ModuleWrap(url, undefined, ['default'], function() {

diff --git a/graal-nodejs/test/es-module/test-esm-json.mjs b/graal-nodejs/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/graal-nodejs/test/es-module/test-esm-virtual-json.mjs b/graal-nodejs/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' } }),
+);