From fe26f8a8609f7af04f9149f2c515f423e483018b 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 Backport-PR-URL: https://github.com/nodejs/node/pull/50669 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 | 16 ++- lib/internal/modules/package_json_reader.js | 106 ++++++++++++++++++-- lib/internal/url.js | 38 +++++++ lib/url.js | 31 +----- src/node_file.cc | 17 ++-- test/parallel/test-bootstrap-modules.js | 11 +- test/parallel/test-module-binding.js | 34 +++---- 9 files changed, 196 insertions(+), 217 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 44fab4de4c0823..4afccdf68f0bd6 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -83,7 +83,6 @@ const { pendingDeprecate, emitExperimentalWarning, kEmptyObject, - filterOwnProperties, setOwnProperty, getLazy, } = require('internal/util'); @@ -354,36 +353,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; @@ -407,7 +382,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, }; @@ -416,7 +391,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); @@ -521,9 +496,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) { @@ -558,7 +534,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( @@ -1016,7 +992,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'); @@ -1262,9 +1238,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 7973865504b853..963e95da1f61c6 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -4,7 +4,6 @@ const { ArrayIsArray, ArrayPrototypeJoin, ArrayPrototypeShift, - JSONParse, JSONStringify, ObjectGetOwnPropertyNames, ObjectPrototypeHasOwnProperty, @@ -55,9 +54,9 @@ const { } = require('internal/errors').codes; const { Module: CJSModule } = require('internal/modules/cjs/loader'); -const packageJsonReader = require('internal/modules/package_json_reader'); -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'); /** @@ -231,8 +230,8 @@ function resolveDirectoryEntry(search) { const pkgJsonPath = resolve(dirPath, 'package.json'); if (fileExists(pkgJsonPath)) { const pkgJson = packageJsonReader.read(pkgJsonPath); - if (pkgJson.containsKeys) { - const { main } = JSONParse(pkgJson.string); + if (pkgJson.exists) { + const { main } = pkgJson; if (main != null) { const mainUrl = pathToFileURL(resolve(dirPath, main)); return resolveExtensionsWithTryExactName(mainUrl); @@ -810,8 +809,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); } @@ -835,8 +833,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 9e26e42b27a229..8e4a5e97b5d531 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -22,6 +22,7 @@ const { ReflectOwnKeys, RegExpPrototypeSymbolReplace, SafeMap, + SafeSet, SafeWeakMap, StringPrototypeCharAt, StringPrototypeCharCodeAt, @@ -106,6 +107,40 @@ const searchParams = Symbol('query'); */ const internalSearchParams = new SafeWeakMap(); +// `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, @@ -1534,4 +1569,7 @@ module.exports = { isURL, urlUpdateActions: updateActions, + unsafeProtocol, + hostlessProtocol, + slashedProtocol, }; diff --git a/lib/url.js b/lib/url.js index 2b32a7e8bc8843..c99bf384962f90 100644 --- a/lib/url.js +++ b/lib/url.js @@ -26,7 +26,6 @@ const { Int8Array, ObjectCreate, ObjectKeys, - SafeSet, StringPrototypeCharCodeAt, decodeURIComponent, } = primordials; @@ -57,6 +56,9 @@ const { fileURLToPath, pathToFileURL, urlToHttpOptions, + unsafeProtocol, + hostlessProtocol, + slashedProtocol, } = require('internal/url'); const bindingUrl = internalBinding('url'); @@ -92,33 +94,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 4dc4b91145d2b7..7f627ac458492c 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1052,8 +1052,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]; @@ -1081,16 +1082,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/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 3c4b44609c34a3..1ce0effe4fb02d 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -48,7 +48,6 @@ const expectedModules = new Set([ 'NativeModule internal/event_target', 'NativeModule internal/fixed_queue', 'NativeModule internal/fs/utils', - 'NativeModule internal/idna', 'NativeModule internal/linkedlist', 'NativeModule internal/modules/cjs/loader', 'NativeModule internal/modules/esm/utils', @@ -84,12 +83,16 @@ const expectedModules = new Set([ 'NativeModule path', 'NativeModule querystring', 'NativeModule timers', - 'NativeModule url', 'NativeModule internal/v8/startup_snapshot', 'NativeModule util', ]); -if (!common.isMainThread) { +if (common.isMainThread) { + [ + 'NativeModule internal/idna', + 'NativeModule url', + ].forEach(expectedModules.add.bind(expectedModules)); +} else { [ 'Internal Binding messaging', 'Internal Binding performance', @@ -132,8 +135,6 @@ if (common.isWindows) { if (common.hasIntl) { expectedModules.add('Internal Binding icu'); -} 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); }