From 951da5282c7b00eb86a989336d628218fb2df057 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Thu, 29 Jun 2023 14:45:31 -0400 Subject: [PATCH] lib: merge cjs and esm package json reader caches PR-URL: https://github.com/nodejs/node/pull/48477 Reviewed-By: Jacob Smith Reviewed-By: Geoffrey Booth Reviewed-By: Mohammed Keyvanzadeh Reviewed-By: Antoine du Hamel Reviewed-By: Matteo Collina --- lib/internal/modules/cjs/loader.js | 54 +++------- lib/internal/modules/esm/package_config.js | 106 ++------------------ lib/internal/modules/esm/resolve.js | 10 +- lib/internal/modules/package_json_reader.js | 106 ++++++++++++++++++-- lib/internal/url.js | 38 +++++++ lib/url.js | 31 +----- src/node_file.cc | 17 ++-- test/fixtures/errors/force_colors.snapshot | 8 +- test/parallel/test-bootstrap-modules.js | 11 +- test/parallel/test-module-binding.js | 34 +++---- 10 files changed, 198 insertions(+), 217 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 508123c6e306ff..d32d7ebf92c438 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -82,7 +82,6 @@ const { pendingDeprecate, emitExperimentalWarning, kEmptyObject, - filterOwnProperties, setOwnProperty, getLazy, } = require('internal/util'); @@ -355,36 +354,12 @@ function initializeCJS() { // -> a. // -> a/index. -const packageJsonCache = new SafeMap(); - +/** + * @param {string} requestPath + * @return {PackageConfig} + */ function readPackage(requestPath) { - const jsonPath = path.resolve(requestPath, 'package.json'); - - const existing = packageJsonCache.get(jsonPath); - if (existing !== undefined) return existing; - - const result = packageJsonReader.read(jsonPath); - const json = result.containsKeys === false ? '{}' : result.string; - if (json === undefined) { - packageJsonCache.set(jsonPath, false); - return false; - } - - try { - const filtered = filterOwnProperties(JSONParse(json), [ - 'name', - 'main', - 'exports', - 'imports', - 'type', - ]); - packageJsonCache.set(jsonPath, filtered); - return filtered; - } catch (e) { - e.path = jsonPath; - e.message = 'Error parsing ' + jsonPath + ': ' + e.message; - throw e; - } + return packageJsonReader.read(path.resolve(requestPath, 'package.json')); } let _readPackage = readPackage; @@ -414,7 +389,7 @@ function readPackageScope(checkPath) { if (StringPrototypeEndsWith(checkPath, sep + 'node_modules')) return false; const pjson = _readPackage(checkPath + sep); - if (pjson) return { + if (pjson.exists) return { data: pjson, path: checkPath, }; @@ -423,7 +398,7 @@ function readPackageScope(checkPath) { } function tryPackage(requestPath, exts, isMain, originalPath) { - const pkg = _readPackage(requestPath)?.main; + const pkg = _readPackage(requestPath).main; if (!pkg) { return tryExtensions(path.resolve(requestPath, 'index'), exts, isMain); @@ -528,9 +503,10 @@ function trySelfParentPath(parent) { function trySelf(parentPath, request) { if (!parentPath) return false; - const { data: pkg, path: pkgPath } = readPackageScope(parentPath) || {}; - if (!pkg || pkg.exports === undefined) return false; - if (typeof pkg.name !== 'string') return false; + const { data: pkg, path: pkgPath } = readPackageScope(parentPath); + if (!pkg || pkg.exports == null || pkg.name === undefined) { + return false; + } let expansion; if (request === pkg.name) { @@ -565,7 +541,7 @@ function resolveExports(nmPath, request) { return; const pkgPath = path.resolve(nmPath, name); const pkg = _readPackage(pkgPath); - if (pkg?.exports != null) { + if (pkg.exists && pkg.exports != null) { try { const { packageExportsResolve } = require('internal/modules/esm/resolve'); return finalizeEsmResolution(packageExportsResolve( @@ -1028,7 +1004,7 @@ Module._resolveFilename = function(request, parent, isMain, options) { if (request[0] === '#' && (parent?.filename || parent?.id === '')) { const parentPath = parent?.filename ?? process.cwd() + path.sep; - const pkg = readPackageScope(parentPath) || {}; + const pkg = readPackageScope(parentPath) || { __proto__: null }; if (pkg.data?.imports != null) { try { const { packageImportsResolve } = require('internal/modules/esm/resolve'); @@ -1274,9 +1250,9 @@ Module._extensions['.js'] = function(module, filename) { content = fs.readFileSync(filename, 'utf8'); } if (StringPrototypeEndsWith(filename, '.js')) { - const pkg = readPackageScope(filename); + const pkg = readPackageScope(filename) || { __proto__: null }; // Function require shouldn't be used in ES modules. - if (pkg?.data?.type === 'module') { + if (pkg.data?.type === 'module') { const parent = moduleParentCache.get(module); const parentPath = parent?.filename; const packageJsonPath = path.resolve(pkg.path, 'package.json'); diff --git a/lib/internal/modules/esm/package_config.js b/lib/internal/modules/esm/package_config.js index dc3c37f6042333..4ca701d4810f74 100644 --- a/lib/internal/modules/esm/package_config.js +++ b/lib/internal/modules/esm/package_config.js @@ -1,102 +1,10 @@ 'use strict'; const { - JSONParse, - ObjectPrototypeHasOwnProperty, - SafeMap, StringPrototypeEndsWith, } = primordials; const { URL, fileURLToPath } = require('internal/url'); -const { - ERR_INVALID_PACKAGE_CONFIG, -} = require('internal/errors').codes; - -const { filterOwnProperties } = require('internal/util'); - - -/** - * @typedef {string | string[] | Record} Exports - * @typedef {'module' | 'commonjs'} PackageType - * @typedef {{ - * pjsonPath: string, - * exports?: ExportConfig, - * name?: string, - * main?: string, - * type?: PackageType, - * }} PackageConfig - */ - -/** @type {Map} */ -const packageJSONCache = new SafeMap(); - - -/** - * @param {string} path - * @param {string} specifier - * @param {string | URL | undefined} base - * @returns {PackageConfig} - */ -function getPackageConfig(path, specifier, base) { - const existing = packageJSONCache.get(path); - if (existing !== undefined) { - return existing; - } - const packageJsonReader = require('internal/modules/package_json_reader'); - const source = packageJsonReader.read(path).string; - if (source === undefined) { - const packageConfig = { - pjsonPath: path, - exists: false, - main: undefined, - name: undefined, - type: 'none', - exports: undefined, - imports: undefined, - }; - packageJSONCache.set(path, packageConfig); - return packageConfig; - } - - let packageJSON; - try { - packageJSON = JSONParse(source); - } catch (error) { - throw new ERR_INVALID_PACKAGE_CONFIG( - path, - (base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier), - error.message, - ); - } - - let { imports, main, name, type } = filterOwnProperties(packageJSON, ['imports', 'main', 'name', 'type']); - const exports = ObjectPrototypeHasOwnProperty(packageJSON, 'exports') ? packageJSON.exports : undefined; - if (typeof imports !== 'object' || imports === null) { - imports = undefined; - } - if (typeof main !== 'string') { - main = undefined; - } - if (typeof name !== 'string') { - name = undefined; - } - // Ignore unknown types for forwards compatibility - if (type !== 'module' && type !== 'commonjs') { - type = 'none'; - } - - const packageConfig = { - pjsonPath: path, - exists: true, - main, - name, - type, - exports, - imports, - }; - packageJSONCache.set(path, packageConfig); - return packageConfig; -} - +const packageJsonReader = require('internal/modules/package_json_reader'); /** * @param {URL | string} resolved @@ -109,7 +17,11 @@ function getPackageScopeConfig(resolved) { if (StringPrototypeEndsWith(packageJSONPath, 'node_modules/package.json')) { break; } - const packageConfig = getPackageConfig(fileURLToPath(packageJSONUrl), resolved); + const packageConfig = packageJsonReader.read(fileURLToPath(packageJSONUrl), { + __proto__: null, + specifier: resolved, + isESM: true, + }); if (packageConfig.exists) { return packageConfig; } @@ -124,7 +36,8 @@ function getPackageScopeConfig(resolved) { } } const packageJSONPath = fileURLToPath(packageJSONUrl); - const packageConfig = { + return { + __proto__: null, pjsonPath: packageJSONPath, exists: false, main: undefined, @@ -133,12 +46,9 @@ function getPackageScopeConfig(resolved) { exports: undefined, imports: undefined, }; - packageJSONCache.set(packageJSONPath, packageConfig); - return packageConfig; } module.exports = { - getPackageConfig, getPackageScopeConfig, }; diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 927b118f8ede2b..d38f75523704f1 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -53,8 +53,9 @@ const { } = require('internal/errors').codes; const { Module: CJSModule } = require('internal/modules/cjs/loader'); -const { getPackageConfig, getPackageScopeConfig } = require('internal/modules/esm/package_config'); +const { getPackageScopeConfig } = require('internal/modules/esm/package_config'); const { getConditionsSet } = require('internal/modules/esm/utils'); +const packageJsonReader = require('internal/modules/package_json_reader'); const { internalModuleStat } = internalBinding('fs'); /** @@ -734,8 +735,7 @@ function packageResolve(specifier, base, conditions) { const packageConfig = getPackageScopeConfig(base); if (packageConfig.exists) { const packageJSONUrl = pathToFileURL(packageConfig.pjsonPath); - if (packageConfig.name === packageName && - packageConfig.exports !== undefined && packageConfig.exports !== null) { + if (packageConfig.exports != null && packageConfig.name === packageName) { return packageExportsResolve( packageJSONUrl, packageSubpath, packageConfig, base, conditions); } @@ -759,8 +759,8 @@ function packageResolve(specifier, base, conditions) { } // Package match. - const packageConfig = getPackageConfig(packageJSONPath, specifier, base); - if (packageConfig.exports !== undefined && packageConfig.exports !== null) { + const packageConfig = packageJsonReader.read(packageJSONPath, { __proto__: null, specifier, base, isESM: true }); + if (packageConfig.exports != null) { return packageExportsResolve( packageJSONUrl, packageSubpath, packageConfig, base, conditions); } diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js index bb175d0df54c04..c6377faae6f5a8 100644 --- a/lib/internal/modules/package_json_reader.js +++ b/lib/internal/modules/package_json_reader.js @@ -1,30 +1,120 @@ 'use strict'; -const { SafeMap } = primordials; +const { + JSONParse, + ObjectPrototypeHasOwnProperty, + SafeMap, +} = primordials; +const { + ERR_INVALID_PACKAGE_CONFIG, +} = require('internal/errors').codes; const { internalModuleReadJSON } = internalBinding('fs'); -const { pathToFileURL } = require('url'); const { toNamespacedPath } = require('path'); +const { kEmptyObject } = require('internal/util'); + +const { fileURLToPath, pathToFileURL } = require('internal/url'); const cache = new SafeMap(); +const isAIX = process.platform === 'aix'; let manifest; /** - * + * @typedef {{ + * exists: boolean, + * pjsonPath: string, + * exports?: string | string[] | Record, + * imports?: string | string[] | Record, + * name?: string, + * main?: string, + * type: 'commonjs' | 'module' | 'none', + * }} PackageConfig + */ + +/** * @param {string} jsonPath + * @param {{ + * base?: string, + * specifier: string, + * isESM: boolean, + * }} options + * @returns {PackageConfig} */ -function read(jsonPath) { +function read(jsonPath, { base, specifier, isESM } = kEmptyObject) { if (cache.has(jsonPath)) { return cache.get(jsonPath); } - const { 0: string, 1: containsKeys } = internalModuleReadJSON( + const { + 0: string, + 1: containsKeys, + } = internalModuleReadJSON( toNamespacedPath(jsonPath), ); - const result = { string, containsKeys }; - const { getOptionValue } = require('internal/options'); - if (string !== undefined) { + const result = { + __proto__: null, + exists: false, + pjsonPath: jsonPath, + main: undefined, + name: undefined, + type: 'none', // Ignore unknown types for forwards compatibility + exports: undefined, + imports: undefined, + }; + + // Folder read operation succeeds in AIX. + // For libuv change, see https://github.com/libuv/libuv/pull/2025. + // https://github.com/nodejs/node/pull/48477#issuecomment-1604586650 + // TODO(anonrig): Follow-up on this change and remove it since it is a + // semver-major change. + const isResultValid = isAIX && !isESM ? containsKeys : string !== undefined; + + if (isResultValid) { + let parsed; + try { + parsed = JSONParse(string); + } catch (error) { + if (isESM) { + throw new ERR_INVALID_PACKAGE_CONFIG( + jsonPath, + (base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier), + error.message, + ); + } else { + // For backward compat, we modify the error returned by JSON.parse rather than creating a new one. + // TODO(aduh95): make it throw ERR_INVALID_PACKAGE_CONFIG in a semver-major with original error as cause + error.message = 'Error parsing ' + jsonPath + ': ' + error.message; + error.path = jsonPath; + throw error; + } + } + + result.exists = true; + + // ObjectPrototypeHasOwnProperty is used to avoid prototype pollution. + if (ObjectPrototypeHasOwnProperty(parsed, 'name') && typeof parsed.name === 'string') { + result.name = parsed.name; + } + + if (ObjectPrototypeHasOwnProperty(parsed, 'main') && typeof parsed.main === 'string') { + result.main = parsed.main; + } + + if (ObjectPrototypeHasOwnProperty(parsed, 'exports')) { + result.exports = parsed.exports; + } + + if (ObjectPrototypeHasOwnProperty(parsed, 'imports')) { + result.imports = parsed.imports; + } + + // Ignore unknown types for forwards compatibility + if (ObjectPrototypeHasOwnProperty(parsed, 'type') && (parsed.type === 'commonjs' || parsed.type === 'module')) { + result.type = parsed.type; + } + if (manifest === undefined) { + const { getOptionValue } = require('internal/options'); manifest = getOptionValue('--experimental-policy') ? require('internal/process/policy').manifest : null; diff --git a/lib/internal/url.js b/lib/internal/url.js index 1813b86962f4cd..302d3fa08753a5 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -18,6 +18,7 @@ const { ReflectOwnKeys, RegExpPrototypeSymbolReplace, SafeMap, + SafeSet, StringPrototypeCharAt, StringPrototypeCharCodeAt, StringPrototypeCodePointAt, @@ -88,6 +89,40 @@ const FORWARD_SLASH = /\//g; const contextForInspect = Symbol('context'); +// `unsafeProtocol`, `hostlessProtocol` and `slashedProtocol` is +// deliberately moved to `internal/url` rather than `url`. +// Workers does not bootstrap URL module. Therefore, `SafeSet` +// is not initialized on bootstrap. This case breaks the +// test-require-delete-array-iterator test. + +// Protocols that can allow "unsafe" and "unwise" chars. +const unsafeProtocol = new SafeSet([ + 'javascript', + 'javascript:', +]); +// Protocols that never have a hostname. +const hostlessProtocol = new SafeSet([ + 'javascript', + 'javascript:', +]); +// Protocols that always contain a // bit. +const slashedProtocol = new SafeSet([ + 'http', + 'http:', + 'https', + 'https:', + 'ftp', + 'ftp:', + 'gopher', + 'gopher:', + 'file', + 'file:', + 'ws', + 'ws:', + 'wss', + 'wss:', +]); + const updateActions = { kProtocol: 0, kHost: 1, @@ -1464,4 +1499,7 @@ module.exports = { urlUpdateActions: updateActions, getURLOrigin, + unsafeProtocol, + hostlessProtocol, + slashedProtocol, }; diff --git a/lib/url.js b/lib/url.js index 96a34babebb029..2cb51ff362a295 100644 --- a/lib/url.js +++ b/lib/url.js @@ -25,7 +25,6 @@ const { Boolean, Int8Array, ObjectKeys, - SafeSet, StringPrototypeCharCodeAt, decodeURIComponent, } = primordials; @@ -56,6 +55,9 @@ const { fileURLToPath, pathToFileURL, urlToHttpOptions, + unsafeProtocol, + hostlessProtocol, + slashedProtocol, } = require('internal/url'); const bindingUrl = internalBinding('url'); @@ -91,33 +93,6 @@ const hostPattern = /^\/\/[^@/]+@[^@/]+/; const simplePathPattern = /^(\/\/?(?!\/)[^?\s]*)(\?[^\s]*)?$/; const hostnameMaxLen = 255; -// Protocols that can allow "unsafe" and "unwise" chars. -const unsafeProtocol = new SafeSet([ - 'javascript', - 'javascript:', -]); -// Protocols that never have a hostname. -const hostlessProtocol = new SafeSet([ - 'javascript', - 'javascript:', -]); -// Protocols that always contain a // bit. -const slashedProtocol = new SafeSet([ - 'http', - 'http:', - 'https', - 'https:', - 'ftp', - 'ftp:', - 'gopher', - 'gopher:', - 'file', - 'file:', - 'ws', - 'ws:', - 'wss', - 'wss:', -]); const { CHAR_SPACE, CHAR_TAB, diff --git a/src/node_file.cc b/src/node_file.cc index 5a92432019dbb1..66a7a8f55dd4f1 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1077,8 +1077,9 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { if (offset >= 3 && 0 == memcmp(chars.data(), "\xEF\xBB\xBF", 3)) { start = 3; // Skip UTF-8 BOM. } - const size_t size = offset - start; + + // TODO(anonrig): Follow-up on removing the following changes for AIX. char* p = &chars[start]; char* pe = &chars[size]; char* pos[2]; @@ -1106,16 +1107,14 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { } } - Local return_value[] = { - String::NewFromUtf8(isolate, - &chars[start], - v8::NewStringType::kNormal, - size).ToLocalChecked(), - Boolean::New(isolate, p < pe ? true : false) - }; + String::NewFromUtf8( + isolate, &chars[start], v8::NewStringType::kNormal, size) + .ToLocalChecked(), + Boolean::New(isolate, p < pe ? true : false)}; + args.GetReturnValue().Set( - Array::New(isolate, return_value, arraysize(return_value))); + Array::New(isolate, return_value, arraysize(return_value))); } // Used to speed up module loading. Returns 0 if the path refers to diff --git a/test/fixtures/errors/force_colors.snapshot b/test/fixtures/errors/force_colors.snapshot index b52735370ea0f3..daef7905a6be19 100644 --- a/test/fixtures/errors/force_colors.snapshot +++ b/test/fixtures/errors/force_colors.snapshot @@ -4,10 +4,10 @@ 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:1257:14) - at Module._extensions..js (node:internal*modules*cjs*loader:1311:10) - at Module.load (node:internal*modules*cjs*loader:1115:32) - at Module._load (node:internal*modules*cjs*loader:962:12) + at Module._compile (node:internal*modules*cjs*loader:1233:14) + at Module._extensions..js (node:internal*modules*cjs*loader:1287: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 node:internal*main*run_main_module:23:47 diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 0a671eb95eb6d4..78db466a95b38e 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -75,7 +75,6 @@ const expectedModules = new Set([ 'NativeModule internal/blob', 'NativeModule internal/fs/utils', 'NativeModule fs', - 'NativeModule internal/idna', 'Internal Binding options', 'NativeModule internal/options', 'NativeModule internal/source_map/source_map_cache', @@ -95,7 +94,12 @@ const expectedModules = new Set([ 'NativeModule internal/process/pre_execution', ]); -if (!common.isMainThread) { +if (common.isMainThread) { + [ + 'NativeModule internal/idna', + 'NativeModule url', + ].forEach(expectedModules.add.bind(expectedModules)); +} else { [ 'NativeModule diagnostics_channel', 'NativeModule internal/abort_controller', @@ -134,9 +138,6 @@ if (common.isWindows) { if (common.hasIntl) { expectedModules.add('Internal Binding icu'); - expectedModules.add('NativeModule url'); -} else { - expectedModules.add('NativeModule url'); } if (process.features.inspector) { diff --git a/test/parallel/test-module-binding.js b/test/parallel/test-module-binding.js index a0fa0a038990f0..47170785434099 100644 --- a/test/parallel/test-module-binding.js +++ b/test/parallel/test-module-binding.js @@ -3,35 +3,27 @@ require('../common'); const fixtures = require('../common/fixtures'); const { internalBinding } = require('internal/test/binding'); +const { filterOwnProperties } = require('internal/util'); const { internalModuleReadJSON } = internalBinding('fs'); const { readFileSync } = require('fs'); -const { strictEqual } = require('assert'); +const { strictEqual, deepStrictEqual } = require('assert'); + { - const [string, containsKeys] = internalModuleReadJSON('nosuchfile'); - strictEqual(string, undefined); - strictEqual(containsKeys, undefined); + strictEqual(internalModuleReadJSON('nosuchfile')[0], undefined); } { - const [string, containsKeys] = - internalModuleReadJSON(fixtures.path('empty.txt')); - strictEqual(string, ''); - strictEqual(containsKeys, false); + strictEqual(internalModuleReadJSON(fixtures.path('empty.txt'))[0], ''); } { - const [string, containsKeys] = - internalModuleReadJSON(fixtures.path('empty.txt')); - strictEqual(string, ''); - strictEqual(containsKeys, false); -} -{ - const [string, containsKeys] = - internalModuleReadJSON(fixtures.path('empty-with-bom.txt')); - strictEqual(string, ''); - strictEqual(containsKeys, false); + strictEqual(internalModuleReadJSON(fixtures.path('empty-with-bom.txt'))[0], ''); } { const filename = fixtures.path('require-bin/package.json'); - const [string, containsKeys] = internalModuleReadJSON(filename); - strictEqual(string, readFileSync(filename, 'utf8')); - strictEqual(containsKeys, true); + const returnValue = JSON.parse(internalModuleReadJSON(filename)[0]); + const file = JSON.parse(readFileSync(filename, 'utf-8')); + const expectedValue = filterOwnProperties(file, ['name', 'main', 'exports', 'imports', 'type']); + deepStrictEqual({ + __proto__: null, + ...returnValue, + }, expectedValue); }