From d7f1c1232eb25d254b39c04cb16ea5080de12004 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 19 Mar 2018 17:34:43 -0700 Subject: [PATCH 1/6] refactor: remove trace-plugin-loader.ts --- src/trace-plugin-loader.ts | 293 ------------------------------------- 1 file changed, 293 deletions(-) delete mode 100644 src/trace-plugin-loader.ts diff --git a/src/trace-plugin-loader.ts b/src/trace-plugin-loader.ts deleted file mode 100644 index fdabe269f..000000000 --- a/src/trace-plugin-loader.ts +++ /dev/null @@ -1,293 +0,0 @@ -/** - * Copyright 2017 Google Inc. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import {Logger} from '@google-cloud/common'; -import Module = require('module'); -import * as path from 'path'; -import * as semver from 'semver'; -import * as shimmer from 'shimmer'; -import * as util from './util'; -import {TraceAgent, TraceAgentConfig} from './trace-api'; -import {Patch, Intercept, Instrumentation, Plugin} from './plugin-types'; - -/** - * An interface representing config options read by the plugin loader, which - * includes TraceAgent configuration as well. - */ -export interface PluginLoaderConfig extends TraceAgentConfig { - plugins: {[pluginName: string]: string;}; -} - -interface InternalPatch extends Patch { - file: string; - module?: T; -} - -interface InternalIntercept extends Intercept { - file: string; - module?: T; -} - -type InternalInstrumentation = InternalPatch|InternalIntercept; - -// tslint:disable:no-any -interface InternalPlugin { - file: string; - patches: - {[patchName: string]: {[file: string]: InternalInstrumentation;}}; - agent: TraceAgent; -} -// tslint:enable:no-any - -interface PluginStore { - [pluginName: string]: InternalPlugin; -} - -// type guards - -function isPatch(obj: Instrumentation): obj is Patch { - return !!(obj as Patch).patch; -} - -function isIntercept(obj: Instrumentation): obj is Intercept { - return !!(obj as Intercept).intercept; -} - -function isInternalPatch(obj: InternalInstrumentation): - obj is InternalPatch { - return !!(obj as InternalPatch).patch; -} - -function isInternalIntercept(obj: InternalInstrumentation): - obj is InternalIntercept { - return !!(obj as InternalIntercept).intercept; -} - -let plugins: PluginStore = Object.create(null); -// tslint:disable-next-line:no-any -let intercepts: {[moduleName: string]: {interceptedValue: any}} = - Object.create(null); -let activated = false; -// TODO(kjin): Make plugin loader a singleton, so we can avoid shadowing. -// tslint:disable-next-line:variable-name -let logger_: Logger; - -function checkLoadedModules(): void { - for (const moduleName of Object.keys(plugins)) { - // \\ is benign on unix and escapes \\ on windows - const regex = - new RegExp('node_modules\\' + path.sep + moduleName + '\\' + path.sep); - for (const file of Object.keys(require.cache)) { - if (file.match(regex)) { - logger_.error( - moduleName + ' tracing might not work as ' + file + - ' was loaded before the trace agent was initialized.'); - break; - } - } - } - if (process._preload_modules && process._preload_modules.length > 0) { - const first = process._preload_modules[0]; - if (first !== '@google-cloud/trace-agent') { - logger_.error( - 'Tracing might not work as ' + first + - ' was loaded with --require before the trace agent was initialized.'); - } - } -} - -function checkPatch(patch: Instrumentation) { - if (!(patch as Patch).patch && !(patch as Intercept).intercept) { - throw new Error( - 'Plugin for ' + patch.file + ' doesn\'t patch ' + - 'anything.'); - } else if ((patch as Patch).patch && (patch as Intercept).intercept) { - throw new Error( - 'Plugin for ' + patch.file + ' has ' + - 'both intercept and patch functions.'); - } else if ((patch as Patch).unpatch && (patch as Intercept).intercept) { - logger_.warn( - 'Plugin for ' + patch.file + ': unpatch is not compatible ' + - 'with intercept.'); - } else if ((patch as Patch).patch && !(patch as Patch).unpatch) { - logger_.warn( - 'Plugin for ' + patch.file + ': patch method given without ' + - 'accompanying unpatch.'); - } -} - -export function activate(logger: Logger, config: PluginLoaderConfig): void { - if (activated) { - logger.error('Plugins activated more than once.'); - return; - } - activated = true; - - logger_ = logger; - - const pluginConfig = config.plugins; - for (const moduleName of Object.keys(pluginConfig)) { - if (!pluginConfig[moduleName]) { - continue; - } - const agent = new TraceAgent(moduleName); - agent.enable(logger, config); - plugins[moduleName] = {file: pluginConfig[moduleName], patches: {}, agent}; - } - - checkLoadedModules(); - - // hook into Module._load so that we can hook into userspace frameworks - shimmer.wrap( - Module, '_load', - (originalModuleLoad: typeof Module._load): typeof Module._load => { - // tslint:disable:no-any - function loadAndPatch( - instrumentation: InternalPlugin, moduleRoot: string, - version: string): any { - // tslint:enable:no-any - let patchSet = instrumentation.patches[moduleRoot]; - if (!patchSet) { - // Load the plugin object - const plugin: Plugin = - originalModuleLoad(instrumentation.file, module, false); - patchSet = {}; - if (semver.valid(version)) { - // strip version of pre-release tags. - // warn if they exist. - if (!!semver.prerelease(version)) { - const originalVersion = version; - version = version.split('-')[0]; - logger.warn(`${moduleRoot}: Using patch for version ${ - version} for pre-release version ${originalVersion}`); - } - if (plugin.length > 0) { - plugin.forEach((patch) => { - if (!patch.versions || - semver.satisfies(version, patch.versions)) { - const file = patch.file || ''; - if (isPatch(patch)) { - patchSet[file] = { - file, - patch: patch.patch, - unpatch: patch.unpatch - }; - } - if (isIntercept(patch)) { - patchSet[file] = {file, intercept: patch.intercept}; - } - // The conditionals exhaustively cover types for the patch - // object, but throw an error in JavaScript anyway - checkPatch(patch); - } - }); - if (Object.keys(patchSet).length === 0) { - logger.warn(`${moduleRoot}: version ${ - version} not supported by plugin.`); - } - } - } else { - logger.warn( - `${moduleRoot}: invalid version ${version}; not patching.`); - } - instrumentation.patches[moduleRoot] = patchSet; - } - - for (const file of Object.keys(patchSet)) { - const patch = patchSet[file]; - const loadPath = - moduleRoot ? path.join(moduleRoot, patch.file) : patch.file; - if (!patch.module) { - patch.module = originalModuleLoad(loadPath, module, false); - } - if (isInternalPatch(patch)) { - patch.patch(patch.module, instrumentation.agent); - } - if (isInternalIntercept(patch)) { - patch.module = - patch.intercept(patch.module, instrumentation.agent); - intercepts[loadPath] = {interceptedValue: patch.module}; - } - } - const rootPatch = patchSet['']; - if (rootPatch && isInternalIntercept(rootPatch)) { - return rootPatch.module; - } else { - return null; - } - } - - function moduleAlreadyPatched( - instrumentation: InternalPlugin, moduleRoot: string) { - return instrumentation.patches[moduleRoot]; - } - - // Future requires get patched as they get loaded. - // tslint:disable:no-any - return function Module_load( - this: any, request: string, parent?: NodeModule, - isMain?: boolean): any { - // tslint:enable:no-any - const instrumentation = plugins[request]; - if (instrumentation) { - const moduleRoot = util.findModulePath(request, parent); - const moduleVersion = - util.findModuleVersion(moduleRoot, originalModuleLoad); - if (moduleAlreadyPatched(instrumentation, moduleRoot)) { - return originalModuleLoad.apply(this, arguments); - } - logger.info('Patching ' + request + ' at version ' + moduleVersion); - const patchedRoot = - loadAndPatch(instrumentation, moduleRoot, moduleVersion); - if (patchedRoot !== null) { - return patchedRoot; - } - } else { - const modulePath = - Module._resolveFilename(request, parent).replace('/', path.sep); - if (intercepts[modulePath]) { - return intercepts[modulePath].interceptedValue; - } - } - return originalModuleLoad.apply(this, arguments); - }; - }); -} - -export function deactivate(): void { - if (activated) { - activated = false; - for (const moduleName of Object.keys(plugins)) { - const instrumentation = plugins[moduleName]; - instrumentation.agent.disable(); - for (const moduleRoot of Object.keys(instrumentation.patches)) { - const patchSet = instrumentation.patches[moduleRoot]; - for (const file of Object.keys(patchSet)) { - const patch = patchSet[file]; - if (isInternalPatch(patch) && patch.unpatch !== undefined) { - logger_.info('Unpatching ' + moduleName); - patch.unpatch(patch.module); - } - } - } - } - plugins = Object.create(null); - intercepts = Object.create(null); - - // unhook module.load - shimmer.unwrap(Module, '_load'); - } -} From 9ba58b55185b36f64b3872c774381d56eedeaddb Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 19 Mar 2018 17:35:11 -0700 Subject: [PATCH 2/6] refactor: re-implement plugin loader --- package.json | 3 + src/index.ts | 11 +- src/plugins/plugin-knex.ts | 4 +- src/trace-plugin-loader.ts | 504 +++++++++++++++++++++++++++++++++++++ src/types.d.ts | 13 + src/util.ts | 44 +--- 6 files changed, 532 insertions(+), 47 deletions(-) create mode 100644 src/trace-plugin-loader.ts diff --git a/package.json b/package.json index 6486bcc50..28947894e 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,7 @@ "node": ">=0.12" }, "devDependencies": { + "@types/builtin-modules": "^2.0.0", "@types/connect": "^3.4.31", "@types/continuation-local-storage": "^3.2.1", "@types/express": "^4.11.0", @@ -97,6 +98,7 @@ }, "dependencies": { "@google-cloud/common": "^0.16.2", + "builtin-modules": "^2.0.0", "continuation-local-storage": "^3.2.1", "extend": "^3.0.0", "gcp-metadata": "^0.4.1", @@ -104,6 +106,7 @@ "lodash.findindex": "^4.4.0", "lodash.isequal": "^4.0.0", "methods": "^1.1.1", + "require-in-the-middle": "^2.2.1", "semver": "^5.4.1", "shimmer": "^1.2.0", "uuid": "^3.0.1" diff --git a/src/index.ts b/src/index.ts index 1e3faee4a..7e7bbac8a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -31,7 +31,7 @@ import * as extend from 'extend'; import * as path from 'path'; import * as PluginTypes from './plugin-types'; import {PluginLoaderConfig} from './trace-plugin-loader'; -import * as pluginLoader from './trace-plugin-loader'; +import {pluginLoader} from './trace-plugin-loader'; import {TraceAgent} from './trace-api'; import {traceWriter, TraceWriterConfig} from './trace-writer'; import * as traceUtil from './util'; @@ -116,7 +116,12 @@ function stop() { if (traceAgent && traceAgent.isActive()) { traceWriter.get().stop(); traceAgent.disable(); - pluginLoader.deactivate(); + try { + const loader = pluginLoader.get(); + loader.deactivate(); + } catch (e) { + // Plugin loader wasn't even created. No need to de-activate + } cls.destroyNamespace(); } } @@ -168,7 +173,7 @@ export function start(projectConfig?: Config): PluginTypes.TraceAgent { }); traceAgent.enable(logger, config); - pluginLoader.activate(logger, config); + pluginLoader.create(logger, config).activate(); if (typeof config.projectId !== 'string' && typeof config.projectId !== 'undefined') { diff --git a/src/plugins/plugin-knex.ts b/src/plugins/plugin-knex.ts index bec2d3249..4f5b926dd 100644 --- a/src/plugins/plugin-knex.ts +++ b/src/plugins/plugin-knex.ts @@ -18,7 +18,6 @@ var shimmer = require('shimmer'); var util = require('util'); -// knex 0.10.x and 0.11.x do not need patching var VERSIONS = '>=0.12 <=0.13'; function patchClient(Client, api) { @@ -54,6 +53,9 @@ module.exports = [ versions: VERSIONS, patch: patchClient, unpatch: unpatchClient + }, + { + versions: '>=0.10 <=0.11' } ]; diff --git a/src/trace-plugin-loader.ts b/src/trace-plugin-loader.ts new file mode 100644 index 000000000..31b0dea5b --- /dev/null +++ b/src/trace-plugin-loader.ts @@ -0,0 +1,504 @@ +/** + * Copyright 2017 Google Inc. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {Logger} from '@google-cloud/common'; +import Module = require('module'); +import * as hook from 'require-in-the-middle'; +import * as path from 'path'; +import * as semver from 'semver'; +import * as shimmer from 'shimmer'; +import * as util from './util'; +import * as builtinModules from 'builtin-modules'; +import {TraceAgent, TraceAgentConfig} from './trace-api'; +import {Patch, Intercept, Plugin, Instrumentation} from './plugin-types'; +import {Singleton} from './util'; + +/** + * Plugins are user-provided objects containing functions that should be run + * when a module is loaded, with the intent of monkeypatching a module to be + * loaded. Each plugin is specific to a module. + * + * Plugin objects are a list of load hooks, each of which consists + * of a file path of a module-internal file to patch, a patch/intercept/hook + * function, as well as the version range of the module for which that file + * should be patched. (See ./plugin-types for the exact interface.) + */ + +export interface PluginLoaderConfig extends TraceAgentConfig { + // An object which contains paths to files that should be loaded as plugins + // upon loading a module with a given name. + plugins: {[pluginName: string]: string}; +} + +/** + * An interface representing configuration passed to the plugin loader, which + * includes TraceAgent configuration as well. + */ +export interface PluginLoaderSingletonConfig extends PluginLoaderConfig { + forceNewAgent_: boolean; +} + +export interface ModulePluginWrapperOptions { + name: string; + path: string; +} + +export interface CorePluginWrapperOptions { + children: ModulePluginWrapperOptions[]; +} + +/** + * An interface that represents a wrapper around user-provided plugin objects. + */ +export interface PluginWrapper { + /** + * Returns whether the given version of the module is supported by this + * plugin. This may load the plugin into memory. + * @param version A semver version string. + */ + isSupported(version: string): boolean; + + /** + * Call unpatch methods when they were provided. + */ + unapplyAll(): void; + + /** + * Applies this object's underlying plugin patches to a file, returning the + * patched or intercepted value. + * @param moduleExports The module exports of the file. + * @param file The file path, relative to the module root. + * @param version The module version. + */ + applyPlugin(moduleExports: T, file: string, version: string): T; +} + +/** + * A class that represents wrapper logic around a user-provided plugin object + * to be applied to a single module. + */ +export class ModulePluginWrapper implements PluginWrapper { + // Sentinel value to indicate that a plugin has not been loaded into memory + // yet. + private static readonly NOT_LOADED: Plugin = []; + private unpatchFns: Array<() => void> = []; + // A logger. + private logger: Logger; + // Configuration for a TraceAgent instance. + private traceConfig: TraceAgentConfig; + // Display-friendly name of the module being patched by this plugin. + private name: string; + // The path to the plugin. + private path: string; + // The exported value of the plugin, or NOT_LOADED if it hasn't been + // loaded yet. + private pluginExportedValue: Plugin = ModulePluginWrapper.NOT_LOADED; + private traceApiInstances: TraceAgent[] = []; + + /** + * Constructs a new PluginWrapper instance. + * @param logger The logger to use. + * @param options Initialization fields for this object. + * @param traceConfig Configuration for a TraceAgent instance. + */ + constructor( + logger: Logger, options: ModulePluginWrapperOptions, + traceConfig: TraceAgentConfig) { + this.logger = logger; + this.name = options.name; + this.path = options.path; + this.traceConfig = traceConfig; + } + + isSupported(version: string): boolean { + // The plugin is lazily loaded here. + const plugin = this.getPluginExportedValue(); + // Count the number of Patch/Intercept objects with compatible version + // ranges + let numFiles = 0; + plugin.forEach(instrumentation => { + const postLoadVersions = instrumentation.versions; + if (!postLoadVersions || semver.satisfies(version, postLoadVersions)) { + numFiles++; + } + }); + // We consider a module to be unsupported if there are no Patch/Intercept + // objects with compatible version ranges at all. + return numFiles > 0; + } + + unapplyAll() { + this.unpatchFns.reverse().forEach(fn => fn()); + this.unpatchFns.length = 0; + this.traceApiInstances.forEach(traceApi => traceApi.disable()); + this.traceApiInstances.length = 0; + } + + applyPlugin(moduleExports: T, file: string, version: string): T { + // Pre-compute a string used in logs for code clarity. + const logString = `${this.name}@${version}${file ? `:${file}` : ''}`; + // Get the exported value of the plugin value (loading it if it doesn't + // exist) + const plugin = this.getPluginExportedValue(); + // Get a list of supported patches. This is the subset of objects in the + // plugin exported value with matching file/version fields. + const supportedPatches: Array&Intercept>> = + plugin.filter( + instrumentation => + semver.satisfies(version, instrumentation.versions || '*') && + (file === instrumentation.file || + (!file && !instrumentation.file))); + if (supportedPatches.length > 1) { + this.logger.warn(`PluginWrapper#applyPlugin: [${ + logString}] Plugin has more than one patch/intercept object for this file. Applying all.`); + } + + // Apply each patch object. + return supportedPatches.reduce((exportedValue, instrumentation) => { + // TODO(kjin): The only benefit of creating a new TraceAgent object per + // patched file is to give us granularity in log messages. See if we can + // refactor the TraceAgent class to avoid this. + + this.logger.info( + `PluginWrapper#applyPlugin: [${logString}] Applying plugin.`); + if (instrumentation.patch) { + instrumentation.patch( + exportedValue, this.createTraceAgentInstance(logString)); + // Queue a function to run if the plugin gets disabled. + if (instrumentation.unpatch) { + this.unpatchFns.push(() => { + this.logger.info( + `PluginWrapper#unapplyAll: [${logString}] Unpatching file.`); + instrumentation.unpatch!(exportedValue); + }); + } + // The patch object should only have either patch() or intercept(). + if (instrumentation.intercept) { + this.logger.warn(`PluginWrapper#applyPlugin: [${ + logString}] Patch object has both patch() and intercept() for this file. Only applying patch().`); + } + } else if (instrumentation.intercept) { + exportedValue = instrumentation.intercept( + exportedValue, this.createTraceAgentInstance(file)); + } else { + this.logger.warn(`PluginWrapper#applyPlugin: [${ + logString}] Patch object has no known functions for patching this file.`); + } + return exportedValue; + }, moduleExports as T); + } + + // Helper function to get the cached plugin value if it wasn't loaded yet. + getPluginExportedValue(): Plugin { + if (this.pluginExportedValue === ModulePluginWrapper.NOT_LOADED) { + this.pluginExportedValue = require(this.path); + } + return this.pluginExportedValue; + } + + private createTraceAgentInstance(file: string) { + const traceApi = new TraceAgent(file); + traceApi.enable(this.logger, this.traceConfig); + this.traceApiInstances.push(traceApi); + return traceApi; + } +} + +/** + * A class that represents wrapper logic on top of plugins that patch core + * modules. Core modules are different because (1) they can be required by the + * plugins that patch them, and (2) the core module being patched doesn't + * necessarily correspond to the name of the plugin. + */ +export class CorePluginWrapper implements PluginWrapper { + private logger: Logger; + private children: ModulePluginWrapper[]; + + constructor( + logger: Logger, config: CorePluginWrapperOptions, + traceConfig: TraceAgentConfig) { + this.logger = logger; + this.children = config.children.map( + config => new ModulePluginWrapper(logger, config, traceConfig)); + // Eagerly load core plugins into memory. + // This prevents issues related to circular dependencies. + this.children.forEach(child => child.getPluginExportedValue()); + } + + /** + * Returns whether the given version of the module is supported by this + * plugin. This may load the plugin into memory. + * @param version A semver version string. + */ + isSupported(version: string): boolean { + return this.children.some(child => child.isSupported(version)); + } + /** + * Call unpatch methods when they were provided. + */ + unapplyAll(): void { + this.children.forEach(child => child.unapplyAll()); + } + /** + * Applies this object's underlying plugin patches to a file, returning the + * patched or intercepted value. + * @param moduleExports The module exports of the file. + * @param file The file path, relative to the module root. + * @param version The module version. + */ + applyPlugin(moduleExports: T, file: string, version: string): T { + return this.children.reduce((exportedValue, child) => { + return child.applyPlugin(exportedValue, file, version); + }, moduleExports); + } +} + +// States for the Plugin Loader +export enum PluginLoaderState { + NO_HOOK, + ACTIVATED, + DEACTIVATED +} + +/** + * A class providing functionality to hook into module loading and apply + * plugins to enable tracing. + */ +export class PluginLoader { + // Key on which core modules are stored. + static readonly CORE_MODULE = '[core]'; + // The function to call to register a require hook. + private enableRequireHook: (onRequire: hook.OnRequireFn) => void; + // A logger. + private logger: Logger; + // A map mapping module names to their respective plugins. + private pluginMap: Map = new Map(); + // A map caching version strings for a module based on their base path. + private moduleVersionCache: Map = new Map(); + // The current state of the plugin loader. + private internalState: PluginLoaderState = PluginLoaderState.NO_HOOK; + + /** + * Constructs a new PluginLoader instance. + * @param logger The logger to use. + * @param config The configuration for this instance. + */ + constructor(logger: Logger, config: PluginLoaderConfig) { + this.logger = logger; + + const nonCoreModules: string[] = []; + // Initialize ALL of the PluginWrapper objects here. + // See CorePluginWrapper docs for why core modules are processed + // differently. + const coreWrapperConfig: CorePluginWrapperOptions = {children: []}; + Object.keys(config.plugins).forEach(key => { + const value = config.plugins[key]; + // Core module plugins share a common key. + const coreModule = builtinModules.indexOf(key) !== -1 || + key === PluginLoader.CORE_MODULE; + + if (value) { + // Convert the given string value to a PluginConfigEntry + // (unless it's falsey). + if (coreModule) { + coreWrapperConfig.children.push({name: key, path: value}); + } else { + this.pluginMap.set( + key, + new ModulePluginWrapper( + logger, {name: key, path: value}, config)); + } + nonCoreModules.push(key); + } + }); + if (coreWrapperConfig.children.length > 0) { + this.pluginMap.set( + PluginLoader.CORE_MODULE, + new CorePluginWrapper(logger, coreWrapperConfig, config)); + } + + // Define the function that will attach a require hook upon activate. + // This must register the hook in the following way: + // * The hook is only called the first time a file is loaded. + // * This hook is called at least for each file that is loaded for + // modules with associated plugins. + // TODO(kjin): This should be encapsulated in a class. + this.enableRequireHook = (onRequire) => { + const builtins = + this.pluginMap.has(PluginLoader.CORE_MODULE) ? builtinModules : []; + hook(builtins.concat(nonCoreModules), {internals: true}, onRequire); + }; + } + + get state(): PluginLoaderState { + return this.internalState; + } + + /** + * Activates plugin loading/patching by hooking into the require method. + */ + activate(): PluginLoader { + if (this.internalState === PluginLoaderState.NO_HOOK) { + this.logger.info(`PluginLoader#activate: Adding require hook.`); + // Enable the require hook. + this.enableRequireHook((exportedValue, moduleStr, baseDir) => { + if (this.internalState === PluginLoaderState.ACTIVATED) { + // Skip processing for package.json + if (!baseDir || path.basename(moduleStr) !== 'package.json') { + // Get module name and internal file path (if exists). + const parsedModuleStr = PluginLoader.parseModuleString(moduleStr); + let name = parsedModuleStr.name; + let file = parsedModuleStr.file; + + // For core modules, use [core] as the name, and the core module as + // the "file". + const isCoreModule = builtinModules.indexOf(name) !== -1; + if (isCoreModule) { + file = name; + name = PluginLoader.CORE_MODULE; + } + + // Check if the module has an associated plugin. + if (this.pluginMap.has(name)) { + // Determine whether this is the main module. Only used to prevent + // logspam for modules that aren't supported and have a lot of + // internal files. + const isMainModule = file.length === 0 && !isCoreModule; + + // Get the module version. + let version = this.getVersion(baseDir); + if (version) { + // Warn for pre-releases. + if (!!semver.prerelease(version)) { + if (isMainModule) { + this.logger.warn(`PluginLoader#onRequire: [${name}@${ + version}] This module is in pre-release. Applying plugin anyways.`); + } + version = version.split('-')[0]; + } + + // Apply each supported plugin. + const plugin = this.pluginMap.get(name); + if (plugin) { + if (plugin.isSupported(version)) { + exportedValue = + plugin.applyPlugin(exportedValue, file, version!); + } else { + this.logger.warn(`PluginLoader#onRequire: [${name}@${ + version}] This module is not supported by the configured set of plugins.`); + } + } + } else if (isMainModule) { + this.logger.error(`PluginLoader#activate: [${ + name}] This module's version could not be determined. Not applying plugins.`); + } + } + } + } + return exportedValue; + }); + this.internalState = PluginLoaderState.ACTIVATED; + this.logger.info(`PluginLoader#activate: Activated.`); + } else if (this.internalState === PluginLoaderState.DEACTIVATED) { + throw new Error('Currently cannot re-activate plugin loader.'); + } else { + throw new Error('Plugin loader already activated.'); + } + return this; + } + + /** + * Deactivates the plugin loader, preventing additional plugins from getting + * loaded or applied, as well as unpatching any modules for which plugins + * specified an unpatching method. + */ + deactivate(): PluginLoader { + if (this.internalState === PluginLoaderState.ACTIVATED) { + // Unpatch the unpatchable functions. + for (const pluginWrapper of this.pluginMap.values()) { + pluginWrapper.unapplyAll(); + } + this.internalState = PluginLoaderState.DEACTIVATED; + this.logger.info(`PluginLoader#deactivate: Deactivated.`); + } + return this; + } + + /** + * Adds a search path for plugin modules. Intended for testing purposes only. + * @param searchPath The path to add. + */ + static setPluginSearchPath(searchPath: string) { + module.paths = [searchPath]; + } + + /** + * Separates the internal file path from the name of a module in a module + * string, returning both (or just the name if it's the main module). + * @param moduleStr The module string; in the form of either `${module}` or + * `${module}/${relPath}` + */ + static parseModuleString(moduleStr: string): {name: string, file: string} { + // Canonicalize the name by using only '/'. + const parts = moduleStr.replace(/\\/g, '/').split('/'); + let indexOfFile = 1; + if (parts[0].startsWith('@')) { // for @org/package + indexOfFile = 2; + } + return { + name: parts.slice(0, indexOfFile).join('/'), + file: parts.slice(indexOfFile).join('/') + }; + } + + // Get the version for a module at a given directory from its package.json + // file, or null if it can't be read or parsed. + // A falsey baseDir suggests a core module, for which the running Node + // version is returned instead. + private getVersion(baseDir?: string): string|null { + if (baseDir) { + if (this.moduleVersionCache.has(baseDir)) { + return this.moduleVersionCache.get(baseDir)!; + } else { + const pjsonPath = path.join(baseDir, 'package.json'); + let version: string|null; + try { + version = require(pjsonPath).version; + // Treat the version as if it's not there if it can't be parsed, + // since for our purposes it's all the same. + if (!semver.parse(version!)) { + this.logger.error(`PluginLoader#getVersion: [${pjsonPath}|${ + version}] Version string could not be parsed.`); + version = null; + } + } catch (e) { + this.logger.error(`PluginLoader#getVersion: [${ + pjsonPath}] An error occurred while retrieving version string. ${ + e.message}`); + version = null; + } + // Cache this value for future lookups. + // This happens if a plugin has multiple internal files patched. + this.moduleVersionCache.set(baseDir, version); + return version; + } + } else { // core module + return process.version.slice(1); // starts with v + } + } +} + +export const pluginLoader = new Singleton(PluginLoader); diff --git a/src/types.d.ts b/src/types.d.ts index d30562884..50f6b0fe1 100644 --- a/src/types.d.ts +++ b/src/types.d.ts @@ -69,3 +69,16 @@ declare module '@google-cloud/common' { }; } } + +declare module 'require-in-the-middle' { + namespace hook { + type Options = { + internals?: boolean; + }; + type OnRequireFn = (exports: T, name: string, basedir?: string) => T; + } + function hook(modules: string[]|null, options: hook.Options|null, onRequire: hook.OnRequireFn): void; + function hook(modules: string[]|null, onRequire: hook.OnRequireFn): void; + function hook(onRequire: hook.OnRequireFn): void; + export = hook; +} diff --git a/src/util.ts b/src/util.ts index 3c1af36a5..1cdb31e5a 100644 --- a/src/util.ts +++ b/src/util.ts @@ -14,10 +14,8 @@ * limitations under the License. */ -import * as fs from 'fs'; -import Module = require('module'); -import * as path from 'path'; import {Logger} from '@google-cloud/common'; // for types only. +import * as path from 'path'; // This symbol must be exported (for now). // See: https://github.com/Microsoft/TypeScript/issues/20080 @@ -169,46 +167,6 @@ export function packageNameFromPath(importPath: string) { return matches && matches.length > 1 ? matches[1] : null; } -/** - * Determines the path at which the requested module will be loaded given - * the provided parent module. - * - * @param request The name of the module to be loaded. - * @param parent The module into which the requested module will be loaded. - */ -export function findModulePath(request: string, parent?: NodeModule): string { - const mainScriptDir = path.dirname(Module._resolveFilename(request, parent)); - const resolvedModule = Module._resolveLookupPaths(request, parent); - const paths = resolvedModule[1]; - for (let i = 0, PL = paths.length; i < PL; i++) { - if (mainScriptDir.indexOf(paths[i]) === 0) { - return path.join(paths[i], request.replace('/', path.sep)); - } - } - return ''; -} - -/** - * Determines the version of the module located at `modulePath`. - * - * @param modulePath The absolute path to the root directory of the - * module being loaded. This may be an empty string if we are loading an - * internal module such as http. - */ -export function findModuleVersion( - modulePath: string, load: (path: string) => {}): string { - if (!load) { - load = Module._load; - } - if (modulePath !== '') { - const pjson = path.join(modulePath, 'package.json'); - if (fs.existsSync(pjson)) { - return (load(pjson) as PackageJson).version; - } - } - return process.version; -} - /** * Creates a StackFrame object containing a structured stack trace. * @param numFrames The number of frames to retain. From e0e65999ef026387bef2e9067042d7409ecd55f4 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 19 Feb 2018 17:27:16 -0800 Subject: [PATCH 3/6] test: rewrite plugin loader tests --- .gitignore | 1 + .../loader/node_modules/large-number/base.js | 1 + .../loader/node_modules/large-number/exp.js | 1 + .../loader/node_modules/large-number/index.js | 4 + .../node_modules/large-number/package.json | 4 + .../node_modules/my-version-1.0-pre/index.js | 1 + .../node_modules/my-version/index.js | 1 + .../node_modules/my-version/package.json | 4 + .../my-version-1.0-pre/package.json | 4 + .../node_modules/my-version-1.0/index.js | 1 + .../node_modules/my-version/index.js | 1 + .../node_modules/my-version/package.json | 4 + .../node_modules/my-version-1.0/package.json | 4 + .../node_modules/my-version-1.1/index.js | 1 + .../node_modules/my-version/lib/index.js | 1 + .../node_modules/my-version/lib/patch.js | 1 + .../node_modules/my-version/package.json | 5 + .../node_modules/my-version-1.1/package.json | 4 + .../node_modules/my-version-2.0/index.js | 1 + .../node_modules/my-version/lib/index.js | 1 + .../node_modules/my-version/package.json | 5 + .../node_modules/my-version-2.0/package.json | 4 + .../loader/node_modules/new-keyboard/index.js | 14 + .../new-keyboard/lib/adjectives.js | 3 + .../node_modules/new-keyboard/lib/animals.js | 3 + .../node_modules/new-keyboard/lib/utils.js | 9 + .../node_modules/new-keyboard/package.json | 4 + .../loader/node_modules/plugin-core/index.js | 34 + .../node_modules/plugin-core/package.json | 4 + .../node_modules/plugin-large-number/index.js | 6 + .../plugin-large-number/package.json | 4 + .../node_modules/plugin-my-version-1/index.js | 19 + .../plugin-my-version-1/package.json | 4 + .../node_modules/plugin-my-version-2/index.js | 9 + .../plugin-my-version-2/package.json | 4 + .../node_modules/plugin-new-keyboard/index.js | 19 + .../plugin-new-keyboard/package.json | 4 + .../node_modules/plugin-small-number/index.js | 8 + .../plugin-small-number/package.json | 4 + .../loader/node_modules/small-number/index.js | 3 + .../node_modules/small-number/package.json | 4 + test/test-plugin-loader.ts | 662 ++++++++---------- tsconfig.json | 1 + 43 files changed, 494 insertions(+), 382 deletions(-) create mode 100644 test/fixtures/loader/node_modules/large-number/base.js create mode 100644 test/fixtures/loader/node_modules/large-number/exp.js create mode 100644 test/fixtures/loader/node_modules/large-number/index.js create mode 100644 test/fixtures/loader/node_modules/large-number/package.json create mode 100644 test/fixtures/loader/node_modules/my-version-1.0-pre/index.js create mode 100644 test/fixtures/loader/node_modules/my-version-1.0-pre/node_modules/my-version/index.js create mode 100644 test/fixtures/loader/node_modules/my-version-1.0-pre/node_modules/my-version/package.json create mode 100644 test/fixtures/loader/node_modules/my-version-1.0-pre/package.json create mode 100644 test/fixtures/loader/node_modules/my-version-1.0/index.js create mode 100644 test/fixtures/loader/node_modules/my-version-1.0/node_modules/my-version/index.js create mode 100644 test/fixtures/loader/node_modules/my-version-1.0/node_modules/my-version/package.json create mode 100644 test/fixtures/loader/node_modules/my-version-1.0/package.json create mode 100644 test/fixtures/loader/node_modules/my-version-1.1/index.js create mode 100644 test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/lib/index.js create mode 100644 test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/lib/patch.js create mode 100644 test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/package.json create mode 100644 test/fixtures/loader/node_modules/my-version-1.1/package.json create mode 100644 test/fixtures/loader/node_modules/my-version-2.0/index.js create mode 100644 test/fixtures/loader/node_modules/my-version-2.0/node_modules/my-version/lib/index.js create mode 100644 test/fixtures/loader/node_modules/my-version-2.0/node_modules/my-version/package.json create mode 100644 test/fixtures/loader/node_modules/my-version-2.0/package.json create mode 100644 test/fixtures/loader/node_modules/new-keyboard/index.js create mode 100644 test/fixtures/loader/node_modules/new-keyboard/lib/adjectives.js create mode 100644 test/fixtures/loader/node_modules/new-keyboard/lib/animals.js create mode 100644 test/fixtures/loader/node_modules/new-keyboard/lib/utils.js create mode 100644 test/fixtures/loader/node_modules/new-keyboard/package.json create mode 100644 test/fixtures/loader/node_modules/plugin-core/index.js create mode 100644 test/fixtures/loader/node_modules/plugin-core/package.json create mode 100644 test/fixtures/loader/node_modules/plugin-large-number/index.js create mode 100644 test/fixtures/loader/node_modules/plugin-large-number/package.json create mode 100644 test/fixtures/loader/node_modules/plugin-my-version-1/index.js create mode 100644 test/fixtures/loader/node_modules/plugin-my-version-1/package.json create mode 100644 test/fixtures/loader/node_modules/plugin-my-version-2/index.js create mode 100644 test/fixtures/loader/node_modules/plugin-my-version-2/package.json create mode 100644 test/fixtures/loader/node_modules/plugin-new-keyboard/index.js create mode 100644 test/fixtures/loader/node_modules/plugin-new-keyboard/package.json create mode 100644 test/fixtures/loader/node_modules/plugin-small-number/index.js create mode 100644 test/fixtures/loader/node_modules/plugin-small-number/package.json create mode 100644 test/fixtures/loader/node_modules/small-number/index.js create mode 100644 test/fixtures/loader/node_modules/small-number/package.json diff --git a/.gitignore b/.gitignore index 384b0c916..dc5ff97c4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ node_modules/ +!test/fixtures/loader/node_modules/ npm-debug.log .DS_Store .nyc_output/ diff --git a/test/fixtures/loader/node_modules/large-number/base.js b/test/fixtures/loader/node_modules/large-number/base.js new file mode 100644 index 000000000..bd816eaba --- /dev/null +++ b/test/fixtures/loader/node_modules/large-number/base.js @@ -0,0 +1 @@ +module.exports = 1; diff --git a/test/fixtures/loader/node_modules/large-number/exp.js b/test/fixtures/loader/node_modules/large-number/exp.js new file mode 100644 index 000000000..554366b14 --- /dev/null +++ b/test/fixtures/loader/node_modules/large-number/exp.js @@ -0,0 +1 @@ +module.exports = 100; diff --git a/test/fixtures/loader/node_modules/large-number/index.js b/test/fixtures/loader/node_modules/large-number/index.js new file mode 100644 index 000000000..168628874 --- /dev/null +++ b/test/fixtures/loader/node_modules/large-number/index.js @@ -0,0 +1,4 @@ +const base = require('./base'); +const exp = require('./exp'); + +module.exports = Number(`${base}e${exp}`); diff --git a/test/fixtures/loader/node_modules/large-number/package.json b/test/fixtures/loader/node_modules/large-number/package.json new file mode 100644 index 000000000..f9ee0c57f --- /dev/null +++ b/test/fixtures/loader/node_modules/large-number/package.json @@ -0,0 +1,4 @@ +{ + "name": "large-number", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/my-version-1.0-pre/index.js b/test/fixtures/loader/node_modules/my-version-1.0-pre/index.js new file mode 100644 index 000000000..6886d9aa7 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.0-pre/index.js @@ -0,0 +1 @@ +module.exports = require('my-version'); diff --git a/test/fixtures/loader/node_modules/my-version-1.0-pre/node_modules/my-version/index.js b/test/fixtures/loader/node_modules/my-version-1.0-pre/node_modules/my-version/index.js new file mode 100644 index 000000000..c2d8dcb4e --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.0-pre/node_modules/my-version/index.js @@ -0,0 +1 @@ +module.exports = "1.0.0-pre"; diff --git a/test/fixtures/loader/node_modules/my-version-1.0-pre/node_modules/my-version/package.json b/test/fixtures/loader/node_modules/my-version-1.0-pre/node_modules/my-version/package.json new file mode 100644 index 000000000..4a6889897 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.0-pre/node_modules/my-version/package.json @@ -0,0 +1,4 @@ +{ + "name": "my-version", + "version": "1.0.0-pre" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/my-version-1.0-pre/package.json b/test/fixtures/loader/node_modules/my-version-1.0-pre/package.json new file mode 100644 index 000000000..2f4d86174 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.0-pre/package.json @@ -0,0 +1,4 @@ +{ + "name": "my-version-1.0-pre", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/my-version-1.0/index.js b/test/fixtures/loader/node_modules/my-version-1.0/index.js new file mode 100644 index 000000000..6886d9aa7 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.0/index.js @@ -0,0 +1 @@ +module.exports = require('my-version'); diff --git a/test/fixtures/loader/node_modules/my-version-1.0/node_modules/my-version/index.js b/test/fixtures/loader/node_modules/my-version-1.0/node_modules/my-version/index.js new file mode 100644 index 000000000..13581111b --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.0/node_modules/my-version/index.js @@ -0,0 +1 @@ +module.exports = "1.0.0"; diff --git a/test/fixtures/loader/node_modules/my-version-1.0/node_modules/my-version/package.json b/test/fixtures/loader/node_modules/my-version-1.0/node_modules/my-version/package.json new file mode 100644 index 000000000..fe5ca9ebd --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.0/node_modules/my-version/package.json @@ -0,0 +1,4 @@ +{ + "name": "my-version", + "version": "1.0.0" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/my-version-1.0/package.json b/test/fixtures/loader/node_modules/my-version-1.0/package.json new file mode 100644 index 000000000..1543f6c62 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.0/package.json @@ -0,0 +1,4 @@ +{ + "name": "my-version-1.0", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/my-version-1.1/index.js b/test/fixtures/loader/node_modules/my-version-1.1/index.js new file mode 100644 index 000000000..6886d9aa7 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.1/index.js @@ -0,0 +1 @@ +module.exports = require('my-version'); diff --git a/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/lib/index.js b/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/lib/index.js new file mode 100644 index 000000000..a2d89eae9 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/lib/index.js @@ -0,0 +1 @@ +module.exports = `1.1.${require('./patch')}`; diff --git a/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/lib/patch.js b/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/lib/patch.js new file mode 100644 index 000000000..d5b2e8bb3 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/lib/patch.js @@ -0,0 +1 @@ +module.exports = 0; diff --git a/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/package.json b/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/package.json new file mode 100644 index 000000000..4be704d8e --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.1/node_modules/my-version/package.json @@ -0,0 +1,5 @@ +{ + "name": "my-version", + "version": "1.1.0", + "main": "lib/index.js" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/my-version-1.1/package.json b/test/fixtures/loader/node_modules/my-version-1.1/package.json new file mode 100644 index 000000000..1eb2c67ea --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-1.1/package.json @@ -0,0 +1,4 @@ +{ + "name": "my-version-1.1", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/my-version-2.0/index.js b/test/fixtures/loader/node_modules/my-version-2.0/index.js new file mode 100644 index 000000000..404842707 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-2.0/index.js @@ -0,0 +1 @@ +module.exports = require('my-version').version; diff --git a/test/fixtures/loader/node_modules/my-version-2.0/node_modules/my-version/lib/index.js b/test/fixtures/loader/node_modules/my-version-2.0/node_modules/my-version/lib/index.js new file mode 100644 index 000000000..746416be6 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-2.0/node_modules/my-version/lib/index.js @@ -0,0 +1 @@ +module.exports = { version: "2.0.0" }; diff --git a/test/fixtures/loader/node_modules/my-version-2.0/node_modules/my-version/package.json b/test/fixtures/loader/node_modules/my-version-2.0/node_modules/my-version/package.json new file mode 100644 index 000000000..fcc6ea0b1 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-2.0/node_modules/my-version/package.json @@ -0,0 +1,5 @@ +{ + "name": "my-version", + "version": "2.0.0", + "main": "lib/index.js" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/my-version-2.0/package.json b/test/fixtures/loader/node_modules/my-version-2.0/package.json new file mode 100644 index 000000000..93f683ba5 --- /dev/null +++ b/test/fixtures/loader/node_modules/my-version-2.0/package.json @@ -0,0 +1,4 @@ +{ + "name": "my-version-2.0", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/new-keyboard/index.js b/test/fixtures/loader/node_modules/new-keyboard/index.js new file mode 100644 index 000000000..13adff389 --- /dev/null +++ b/test/fixtures/loader/node_modules/new-keyboard/index.js @@ -0,0 +1,14 @@ +const adjectives = require('./lib/adjectives').entries(); +const animals = require('./lib/animals').entries(); + +module.exports = `The ${ + adjectives.next().value[1] +} ${ + adjectives.next().value[1] +} ${ + animals.next().value[1] +} jumps over the ${ + adjectives.next().value[1] +} ${ + animals.next().value[1] +}`; diff --git a/test/fixtures/loader/node_modules/new-keyboard/lib/adjectives.js b/test/fixtures/loader/node_modules/new-keyboard/lib/adjectives.js new file mode 100644 index 000000000..5fa0a5fe9 --- /dev/null +++ b/test/fixtures/loader/node_modules/new-keyboard/lib/adjectives.js @@ -0,0 +1,3 @@ +const utils = require('./utils'); + +module.exports = utils.toUpperCase(['quick', 'brown', 'lazy']); diff --git a/test/fixtures/loader/node_modules/new-keyboard/lib/animals.js b/test/fixtures/loader/node_modules/new-keyboard/lib/animals.js new file mode 100644 index 000000000..ce0d6d2cc --- /dev/null +++ b/test/fixtures/loader/node_modules/new-keyboard/lib/animals.js @@ -0,0 +1,3 @@ +const utils = require('./utils'); + +module.exports = utils.toUpperCase(['fox', 'dog']); diff --git a/test/fixtures/loader/node_modules/new-keyboard/lib/utils.js b/test/fixtures/loader/node_modules/new-keyboard/lib/utils.js new file mode 100644 index 000000000..93fe0812b --- /dev/null +++ b/test/fixtures/loader/node_modules/new-keyboard/lib/utils.js @@ -0,0 +1,9 @@ +module.exports = { + toUpperCase: (arr) => { + return arr.map(e => e.toUpperCase()); + } +}; + +// require all of the other modules, because why not +require('./adjectives'); +require('./animals'); diff --git a/test/fixtures/loader/node_modules/new-keyboard/package.json b/test/fixtures/loader/node_modules/new-keyboard/package.json new file mode 100644 index 000000000..8a908f565 --- /dev/null +++ b/test/fixtures/loader/node_modules/new-keyboard/package.json @@ -0,0 +1,4 @@ +{ + "name": "new-keyboard", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/plugin-core/index.js b/test/fixtures/loader/node_modules/plugin-core/index.js new file mode 100644 index 000000000..50b848622 --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-core/index.js @@ -0,0 +1,34 @@ +// This file contains valuable monkeypatches to Node core. + +const ORIGINAL = Symbol(); + +module.exports = [{ + file: 'url', + patch: (url) => { + url.format = Object.assign(() => 'patched-value', { [ORIGINAL]: url.format }); + }, + unpatch: (url) => { + if (url.format[ORIGINAL]) { + url.format = url.format[ORIGINAL]; + } + } +}, { + file: 'crypto', + unpatch: () => { + // forgot to patch + } +}, { + file: 'os', + patch: () => {}, + intercept: () => { + // why not have both + } +}, { + file: 'dns', + patch: () => {} +}, { + file: 'dns', + patch: () => { + // one patch was not enough + } +}]; diff --git a/test/fixtures/loader/node_modules/plugin-core/package.json b/test/fixtures/loader/node_modules/plugin-core/package.json new file mode 100644 index 000000000..5ed8a43eb --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-core/package.json @@ -0,0 +1,4 @@ +{ + "name": "plugin-core", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/plugin-large-number/index.js b/test/fixtures/loader/node_modules/plugin-large-number/index.js new file mode 100644 index 000000000..5aa7b223c --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-large-number/index.js @@ -0,0 +1,6 @@ +module.exports = [{ + file: 'base.js', + intercept: (value) => { + return 2 * value; + } +}]; diff --git a/test/fixtures/loader/node_modules/plugin-large-number/package.json b/test/fixtures/loader/node_modules/plugin-large-number/package.json new file mode 100644 index 000000000..62eddda2f --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-large-number/package.json @@ -0,0 +1,4 @@ +{ + "name": "plugin-large-number", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/plugin-my-version-1/index.js b/test/fixtures/loader/node_modules/plugin-my-version-1/index.js new file mode 100644 index 000000000..92b723269 --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-my-version-1/index.js @@ -0,0 +1,19 @@ +module.exports = [{ + file: '', + versions: '1.0.x', + intercept: (myVersion) => { + if (typeof myVersion === 'object') { + throw new Error(`You're likely using my-version v2!`); + } + return `${myVersion}-patched`; + } +}, { + file: 'lib/patch.js', + versions: '1.1.x', + intercept: (myVersion) => { + if (typeof myVersion === 'object') { + throw new Error(`You're likely using my-version v2!`); + } + return `${myVersion}-patched`; + } +}]; diff --git a/test/fixtures/loader/node_modules/plugin-my-version-1/package.json b/test/fixtures/loader/node_modules/plugin-my-version-1/package.json new file mode 100644 index 000000000..b6d10ce27 --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-my-version-1/package.json @@ -0,0 +1,4 @@ +{ + "name": "plugin-my-version-1", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/plugin-my-version-2/index.js b/test/fixtures/loader/node_modules/plugin-my-version-2/index.js new file mode 100644 index 000000000..1b241ee4e --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-my-version-2/index.js @@ -0,0 +1,9 @@ +module.exports = [{ + versions: '*', + patch: (myVersion) => { + if (typeof myVersion !== 'object') { + throw new Error(`You're likely using my-version v1!`); + } + myVersion.version = '2.0.0-patched'; + } +}]; diff --git a/test/fixtures/loader/node_modules/plugin-my-version-2/package.json b/test/fixtures/loader/node_modules/plugin-my-version-2/package.json new file mode 100644 index 000000000..8ff31c082 --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-my-version-2/package.json @@ -0,0 +1,4 @@ +{ + "name": "plugin-my-version-2", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/plugin-new-keyboard/index.js b/test/fixtures/loader/node_modules/plugin-new-keyboard/index.js new file mode 100644 index 000000000..fee69c01b --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-new-keyboard/index.js @@ -0,0 +1,19 @@ +const kOldValue = Symbol(); + +module.exports = [{ + file: 'lib/adjectives.js', + intercept: (adjectives) => { + return Object.assign(['lab-grown', 'ketchup', 'chili'], { [kOldValue]: adjectives }); + } +}, { + file: 'lib/utils.js', + patch: (utils) => { + utils.toUpperCase = arr => { + return arr.map(str => { + const r = str.split(''); + r[0] = r[0].toUpperCase(); + return r.join(''); + }); + }; + } +}]; diff --git a/test/fixtures/loader/node_modules/plugin-new-keyboard/package.json b/test/fixtures/loader/node_modules/plugin-new-keyboard/package.json new file mode 100644 index 000000000..522d6c99c --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-new-keyboard/package.json @@ -0,0 +1,4 @@ +{ + "name": "plugin-new-keyboard", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/plugin-small-number/index.js b/test/fixtures/loader/node_modules/plugin-small-number/index.js new file mode 100644 index 000000000..469ce0492 --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-small-number/index.js @@ -0,0 +1,8 @@ +module.exports = [{ + patch: (smallNumber) => { + smallNumber.value++; + }, + unpatch: (smallNumber) => { + smallNumber.value--; + } +}]; diff --git a/test/fixtures/loader/node_modules/plugin-small-number/package.json b/test/fixtures/loader/node_modules/plugin-small-number/package.json new file mode 100644 index 000000000..0bca5e9a0 --- /dev/null +++ b/test/fixtures/loader/node_modules/plugin-small-number/package.json @@ -0,0 +1,4 @@ +{ + "name": "plugin-small-number", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/fixtures/loader/node_modules/small-number/index.js b/test/fixtures/loader/node_modules/small-number/index.js new file mode 100644 index 000000000..a4bc3dcf5 --- /dev/null +++ b/test/fixtures/loader/node_modules/small-number/index.js @@ -0,0 +1,3 @@ +module.exports = { + value: 0 +}; diff --git a/test/fixtures/loader/node_modules/small-number/package.json b/test/fixtures/loader/node_modules/small-number/package.json new file mode 100644 index 000000000..c411202c5 --- /dev/null +++ b/test/fixtures/loader/node_modules/small-number/package.json @@ -0,0 +1,4 @@ +{ + "name": "small-number", + "version": "0.0.1" +} \ No newline at end of file diff --git a/test/test-plugin-loader.ts b/test/test-plugin-loader.ts index 2efba54f3..c6e5237d9 100644 --- a/test/test-plugin-loader.ts +++ b/test/test-plugin-loader.ts @@ -1,5 +1,5 @@ /** - * Copyright 2017 Google Inc. All Rights Reserved. + * Copyright 2018 Google Inc. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -13,430 +13,328 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -'use strict'; - -var shimmer = require('shimmer'); -var Module = require('module'); -var assert = require('assert'); -var proxyquire = require('proxyquire'); -var path = require('path'); - -// Save logs because in some cases we want to verify that something was logged. -var logs = { - error: '', - warn: '', - info: '' -}; -function writeToLog(log, data) { - logs[log] += data + '\n'; -} -var logger = { - error: writeToLog.bind(null, 'error'), - warn: writeToLog.bind(null, 'warn'), - info: writeToLog.bind(null, 'info') -}; -// Facilitates loading "fake" modules upon calling require(). -var fakeModules = {}; +import * as assert from 'assert'; +import * as path from 'path'; +import * as hook from 'require-in-the-middle'; +import * as shimmer from 'shimmer'; -// Adds module moduleName to the set of fake modules, using mock as the object -// being "exported" by this module. In addition, providing version makes it -// accessible by calling findModuleVersion. -function addModuleMock(moduleName, version, mock) { - fakeModules[moduleName.replace('/', path.sep)] = { - exports: mock, - version: version - }; -} +import {PluginLoader, PluginLoaderState, PluginWrapper} from '../src/trace-plugin-loader'; -describe('Trace Plugin Loader', function() { - var pluginLoader; +import {TestLogger} from './logger'; - before(function() { - // Wrap Module._load so that it loads from our fake module set rather than the - // real thing - shimmer.wrap(Module, '_load', function(originalModuleLoad) { - return function wrappedModuleLoad(modulePath) { - if (fakeModules[modulePath.replace('/', path.sep)]) { - return fakeModules[modulePath.replace('/', path.sep)].exports; - } - return originalModuleLoad.apply(this, arguments); - }; - }); +export interface SimplePluginLoaderConfig { + // An object which contains paths to files that should be loaded as plugins + // upon loading a module with a given name. + plugins: {[pluginName: string]: string}; +} - // Prevent filename resolution from happening to fake modules - shimmer.wrap(Module, '_resolveFilename', function(originalResolve) { - return function wrappedResolveFilename(request) { - if (fakeModules[request.replace('/', path.sep)]) { - return request; - } - return originalResolve.apply(this, arguments); - }; - }); +const SEARCH_PATH = `${__dirname}/fixtures/loader/node_modules`; +const PROCESS_VERSION = process.version.slice(1); - // proxyquire the plugin loader with stubbed module utility methods - pluginLoader = proxyquire('../src/trace-plugin-loader', { - './util': { - findModulePath: function(request) { - return request.replace('/', path.sep); - }, - findModuleVersion: function(modulePath) { - return fakeModules[modulePath].version; - } - } - }); - }); +const clearRequireCache = () => { + Object.keys(require.cache).forEach(key => delete require.cache[key]); +}; - after(function() { - shimmer.unwrap(Module, '_load'); - }); +describe('Trace Plugin Loader', () => { + let logger: TestLogger; + const makePluginLoader = (config: SimplePluginLoaderConfig) => { + return new PluginLoader( + logger, + Object.assign( + { + samplingRate: 0, + ignoreUrls: [], + enhancedDatabaseReporting: false, + ignoreContextHeader: false, + projectId: '0' + }, + config)); + }; - afterEach(function() { - pluginLoader.deactivate(); - logs.error = ''; - logs.warn = ''; - logs.info = ''; - fakeModules = {}; + before(() => { + module.paths.push(SEARCH_PATH); + PluginLoader.setPluginSearchPath(SEARCH_PATH); + logger = new TestLogger(); }); - /** - * Loads two modules (one of them twice), and makes sure that plugins are - * applied correctly. - */ - it('loads plugins no more than once', function() { - var patched: any[] = []; - addModuleMock('module-a', '1.0.0', {}); - addModuleMock('module-b', '1.0.0', {}); - addModuleMock('module-a-plugin', '', [ - { patch: function() { patched.push('a'); } } - ]); - addModuleMock('module-b-plugin', '', [ - { file: '', patch: function() { patched.push('b'); } } - ]); - // Activate plugin loader - pluginLoader.activate(logger, { - plugins: { - 'module-a': 'module-a-plugin', - 'module-b': 'module-b-plugin' - } - }); - assert.deepEqual(patched, [], - 'No patches are initially loaded'); - require('module-a'); - assert.deepEqual(patched, ['a'], - 'Patches are applied when the relevant patch is loaded'); - assert(logs.info.indexOf('Patching module-a at version 1.0.0') !== -1, - 'Info log is emitted when a module if patched'); - require('module-a'); - assert.deepEqual(patched, ['a'], - 'Patches aren\'t applied twice'); - require('module-b'); - assert.deepEqual(patched, ['a', 'b'], - 'Multiple plugins can be loaded, and file can be set to an empty string'); + afterEach(() => { + logger.clearLogs(); + clearRequireCache(); }); - /** - * Loads two plugins that each monkeypatch modules, and checks that they are - * actually monkeypatched. - */ - it('applies patches', function() { - addModuleMock('module-c', '1.0.0', { - getStatus: function() { return 'not wrapped'; } + describe('interface', () => { + describe('state', () => { + it('returns NO_HOOK when first called', () => { + const pluginLoader = makePluginLoader({plugins: {}}); + assert.strictEqual(pluginLoader.state, PluginLoaderState.NO_HOOK); + }); }); - addModuleMock('module-d', '1.0.0', { - getStatus: function() { return 'not wrapped'; } + + describe('activate', () => { + it('transitions from NO_HOOK to ACTIVATED, enabling require hook', () => { + let requireHookCalled = false; + const pluginLoader = makePluginLoader({plugins: {}}); + // TODO(kjin): Stop using index properties. + pluginLoader['enableRequireHook'] = () => requireHookCalled = true; + pluginLoader.activate(); + + assert.strictEqual(pluginLoader.state, PluginLoaderState.ACTIVATED); + assert.ok(requireHookCalled); + }); + + it('throws if internal state is already ACTIVATED', () => { + let requireHookCalled = false; + const pluginLoader = makePluginLoader({plugins: {}}).activate(); + assert.strictEqual(pluginLoader.state, PluginLoaderState.ACTIVATED); + // TODO(kjin): Stop using index properties. + pluginLoader['enableRequireHook'] = () => requireHookCalled = true; + + assert.throws(() => pluginLoader.activate()); + assert.ok(!requireHookCalled); + }); + + it('throws if internal state is DEACTIVATED', () => { + // There is currently no reason to transition back and forth. + // This behavior may change in the future. + let requireHookCalled = false; + const pluginLoader = + makePluginLoader({plugins: {}}).activate().deactivate(); + assert.strictEqual(pluginLoader.state, PluginLoaderState.DEACTIVATED); + // TODO(kjin): Stop using index properties. + pluginLoader['enableRequireHook'] = () => requireHookCalled = true; + + assert.throws(() => pluginLoader.activate()); + assert.ok(!requireHookCalled); + }); }); - addModuleMock('module-c-plugin', '', [ - { - patch: function(originalModule, api) { - shimmer.wrap(originalModule, 'getStatus', function() { - return function() { return 'wrapped'; }; - }); + + describe('deactivate', () => { + class TestPluginWrapper implements PluginWrapper { + unapplyCalled = false; + isSupported(version: string): boolean { + return false; + } + unapplyAll(): void { + this.unapplyCalled = true; + } + applyPlugin(moduleExports: T, file: string, version: string): T { + return moduleExports; } } - ]); - assert.strictEqual(require('module-c').getStatus(), 'not wrapped', - 'Plugin loader shouldn\'t affect module before plugin is loaded'); - // Activate plugin loader - pluginLoader.activate(logger, { - plugins: { - 'module-c': 'module-c-plugin' - } + + it('transitions state from ACTIVATED to DEACTIVATED, unapplying plugins', + () => { + const pluginLoader = makePluginLoader({plugins: {}}).activate(); + assert.strictEqual(pluginLoader.state, PluginLoaderState.ACTIVATED); + const plugin = new TestPluginWrapper(); + // TODO(kjin): Stop using index properties. + pluginLoader['pluginMap'].set('foo', plugin); + pluginLoader.deactivate(); + + assert.strictEqual( + pluginLoader.state, PluginLoaderState.DEACTIVATED); + assert.ok(plugin.unapplyCalled); + }); + + it('does nothing when already deactivated', () => { + const pluginLoader = makePluginLoader({plugins: {}}); + assert.strictEqual(pluginLoader.state, PluginLoaderState.NO_HOOK); + const plugin = new TestPluginWrapper(); + // TODO(kjin): Stop using index properties. + pluginLoader['pluginMap'].set('foo', plugin); + pluginLoader.deactivate(); + + assert.strictEqual(pluginLoader.state, PluginLoaderState.NO_HOOK); + assert.ok(!plugin.unapplyCalled); + + pluginLoader.activate().deactivate(); + assert.strictEqual(pluginLoader.state, PluginLoaderState.DEACTIVATED); + + plugin.unapplyCalled = false; + pluginLoader.deactivate(); + assert.strictEqual(pluginLoader.state, PluginLoaderState.DEACTIVATED); + assert.ok(!plugin.unapplyCalled); + }); }); - assert.strictEqual(require('module-c').getStatus(), 'wrapped', - 'Plugin patch() method is called the right arguments'); - assert.strictEqual(require('module-d').getStatus(), 'not wrapped', - 'Modules for which there aren\'t plugins won\'t be patched'); }); - /** - * Loads one module to check that plugin patches that aren't compatible don't - * get applied. Then, loads another module with no compatible patches to check - * that nothing gets patched at all. - */ - it('respects patch set semver conditions', function() { - var patched: any[] = []; - addModuleMock('module-e', '1.0.0', {}); - addModuleMock('module-f', '2.0.0', {}); - addModuleMock('module-e-plugin', '', [ - { versions: '1.x', patch: function() { patched.push('e-1.x'); } }, - { versions: '2.x', patch: function() { patched.push('e-2.x'); } } - ]); - addModuleMock('module-f-plugin', '', [ - { versions: '1.x', patch: function() { patched.push('f-1.x'); } } - ]); - // Activate plugin loader - pluginLoader.activate(logger, { - plugins: { - 'module-e': 'module-e-plugin', - 'module-f': 'module-f-plugin' - } + describe('static interface', () => { + describe('parseModuleString', () => { + it('parses module strings', () => { + const p = PluginLoader.parseModuleString; + const sep = path.sep; + assert.deepStrictEqual(p('m'), {name: 'm', file: ''}); + assert.deepStrictEqual(p('m/f'), {name: 'm', file: 'f'}); + assert.deepStrictEqual(p('m/d/f'), {name: 'm', file: 'd/f'}); + assert.deepStrictEqual(p(`m\\d\\f`), {name: 'm', file: 'd/f'}); + assert.deepStrictEqual(p(`@o\\m\\d\\f`), {name: '@o/m', file: 'd/f'}); + assert.deepStrictEqual(p('@o/m/d/f'), {name: '@o/m', file: 'd/f'}); + assert.deepStrictEqual(p('@o/m/d/f'), {name: '@o/m', file: 'd/f'}); + }); }); - assert.deepEqual(patched, []); - require('module-e'); - assert.deepEqual(patched, ['e-1.x'], - 'Only patches with a correct semver condition are loaded'); - require('module-f'); - assert.deepEqual(patched, ['e-1.x'], - 'No patches are loaded if the module version isn\'t supported at all'); - assert(logs.warn.indexOf('module-f: version 2.0.0 not supported') !== -1, - 'A warning is printed if the module version isn\'t supported at all'); }); - /** - * Loads a module with internal exports and patches them, and then makes sure - * that they are actually patched. - */ - it('patches internal files in modules', function() { - addModuleMock('module-g', '1.0.0', { - createSentence: function() { - return require('module-g/subject').get() + ' ' + - require('module-g/predicate').get() + '.'; - } + describe('patching behavior', () => { + it('[sanity check]', () => { + // Ensure that module fixtures contain values that we expect. + assert.strictEqual(require('small-number').value, 0); + assert.strictEqual(require('large-number'), 1e100); + assert.strictEqual( + require('new-keyboard'), + 'The QUICK BROWN FOX jumps over the LAZY DOG'); + assert.strictEqual(require('my-version-1.0'), '1.0.0'); + assert.strictEqual(require('my-version-1.0-pre'), '1.0.0-pre'); + assert.strictEqual(require('my-version-1.1'), '1.1.0'); + assert.strictEqual(require('my-version-2.0'), '2.0.0'); }); - addModuleMock('module-g/subject', '', { - get: function() { - return 'bad tests'; - } - }); - addModuleMock('module-g/predicate', '', { - get: function() { - return 'don\'t make sense'; - } - }); - addModuleMock('module-g-plugin', '', [ - { - file: 'subject', - patch: function(originalModule, api) { - shimmer.wrap(originalModule, 'get', function() { - return function() { - return 'good tests'; - }; - }); - } - }, - { - file: 'predicate', - patch: function(originalModule, api) { - shimmer.wrap(originalModule, 'get', function() { - return function() { - return 'make sense'; - }; - }); - } - } - ]); - assert.strictEqual(require('module-g').createSentence(), - 'bad tests don\'t make sense.'); - // Activate plugin loader - pluginLoader.activate(logger, { - plugins: { - 'module-g': 'module-g-plugin' - } + + it('doesn\'t patch before activation', () => { + makePluginLoader({plugins: {'small-number': 'plugin-small-number'}}); + assert.strictEqual(require('small-number').value, 0); }); - assert.strictEqual(require('module-g').createSentence(), - 'good tests make sense.', - 'Files internal to a module are patched'); - }); - /** - * Loads a module with internal exports and patches them, and then makes sure - * that they are actually patched. - */ - it('intercepts internal files in modules', function() { - addModuleMock('module-h', '1.0.0', { - createSentence: function() { - return require('module-h/subject').get() + ' ' + - require('module-h/predicate').get() + '.'; - } + it('doesn\'t patch modules for which plugins aren\'t specified', () => { + makePluginLoader({plugins: {}}).activate(); + assert.strictEqual(require('small-number').value, 0); }); - addModuleMock('module-h/subject', '', { - get: function() { - return 'bad tests'; - } + + it('patches modules when activated, with no plugin file field specifying the main file', + () => { + makePluginLoader({ + plugins: {'small-number': 'plugin-small-number'} + }).activate(); + assert.strictEqual(require('small-number').value, 1); + // Make sure requiring doesn't patch twice + assert.strictEqual(require('small-number').value, 1); + assert.strictEqual( + logger.getNumLogsWith('info', '[small-number@0.0.1]'), 1); + }); + + it('accepts absolute paths in configuration', () => { + makePluginLoader({ + plugins: {'small-number': `${SEARCH_PATH}/plugin-small-number`} + }).activate(); + assert.strictEqual(require('small-number').value, 1); + assert.strictEqual( + logger.getNumLogsWith('info', '[small-number@0.0.1]'), 1); }); - addModuleMock('module-h/predicate', '', { - get: function() { - return 'don\'t make sense'; - } + + it('unpatches modules when deactivated', () => { + const loader = makePluginLoader({ + plugins: {'small-number': 'plugin-small-number'} + }).activate(); + require('small-number'); + loader.deactivate(); + assert.strictEqual(require('small-number').value, 0); + // One each for activate/deactivate + assert.strictEqual( + logger.getNumLogsWith('info', '[small-number@0.0.1]'), 2); }); - addModuleMock('module-h-plugin', '', [ - { - file: 'subject', - intercept: function(originalModule, api) { - return { - get: function() { - return 'good tests'; - } - }; - } - }, - { - file: 'predicate', - intercept: function(originalModule, api) { - return { - get: function() { - return 'make sense'; - } - }; - } - } - ]); - assert.strictEqual(require('module-h').createSentence(), - 'bad tests don\'t make sense.'); - // Activate plugin loader - pluginLoader.activate(logger, { - plugins: { - 'module-h': 'module-h-plugin' - } + + it('doesn\'t unpatch twice', () => { + const loader = makePluginLoader({ + plugins: {'small-number': 'plugin-small-number'} + }).activate(); + require('small-number'); + loader.deactivate().deactivate(); + assert.strictEqual(require('small-number').value, 0); + // One each for activate/deactivate + assert.strictEqual( + logger.getNumLogsWith('info', '[small-number@0.0.1]'), 2); }); - assert.strictEqual(require('module-h').createSentence(), - 'good tests make sense.', - 'Files internal to a module are patched'); - }); - /** - * Uses module interception to completely replace a module export - */ - it('can intercept modules', function() { - addModuleMock('module-i', '1.0.0', function() { return 1; }); - addModuleMock('module-i-plugin', '', [{ - intercept: function(originalModule, api) { - return function() { return originalModule() + 1; }; - } - }]); - assert.strictEqual(require('module-i')(), 1); - // Activate plugin loader - pluginLoader.activate(logger, { - plugins: { - 'module-i': 'module-i-plugin' - } + it('doesn\'t unpatch modules when deactivated immediately', () => { + makePluginLoader({ + plugins: {'small-number': 'plugin-small-number'} + }).deactivate(); + assert.strictEqual(require('small-number').value, 0); }); - assert.strictEqual(require('module-i')(), 2, - 'Module can be intercepted'); - }); - /** - * Patches a module, then immediately unpatches it, then patches it again to - * show that patching isn't irreversible (and neither is unpatching) - */ - it('can unpatch', function() { - addModuleMock('module-j', '1.0.0', { - getPatchMode: function() { return 'none'; } + it('intercepts and patches internal files', () => { + makePluginLoader({ + plugins: {'large-number': 'plugin-large-number'} + }).activate(); + assert.strictEqual(require('large-number'), 2e100); }); - addModuleMock('module-j-plugin', '', [{ - patch: function(originalModule, api) { - shimmer.wrap(originalModule, 'getPatchMode', function() { - return function() { return 'patch'; }; - }); - }, - unpatch: function(originalModule) { - shimmer.unwrap(originalModule, 'getPatchMode'); - } - }]); - assert.strictEqual(require('module-j').getPatchMode(), 'none'); - pluginLoader.activate(logger, { - plugins: { - 'module-j': 'module-j-plugin' - } + + ['http', 'url', '[core]'].forEach(key => { + it(`intercepts and patches core modules with key "${key}"`, () => { + const loader = + makePluginLoader({plugins: {[key]: 'plugin-core'}}).activate(); + const input = {protocol: 'http:', host: 'hi'}; + assert.strictEqual(require('url').format(input), 'patched-value'); + loader.deactivate(); + assert.strictEqual(require('url').format(input), 'http://hi'); + // One each for activate/deactivate + assert.strictEqual( + logger.getNumLogsWith('info', `[${key}@${PROCESS_VERSION}:url]`), + 2); + }); }); - assert.strictEqual(require('module-j').getPatchMode(), 'patch'); - pluginLoader.deactivate(); - assert.strictEqual(require('module-j').getPatchMode(), 'none', - 'Module gets unpatched'); - pluginLoader.activate(logger, { - plugins: { - 'module-j': 'module-j-plugin' - } + + it('intercepts and patches files with circular dependencies', () => { + makePluginLoader({ + plugins: {'new-keyboard': 'plugin-new-keyboard'} + }).activate(); + assert.strictEqual( + require('new-keyboard'), + 'The lab-grown ketchup Fox jumps over the chili Dog'); }); - assert.strictEqual(require('module-j').getPatchMode(), 'patch', - 'Patches still work after unpatching'); - }); - it('doesn\'t load plugins with falsey paths', function() { - var moduleExports = {}; - addModuleMock('module-k', '1.0.0', moduleExports); - assert(require('module-k') === moduleExports); - pluginLoader.activate(logger, { plugins: { 'module-k': '' } }); - assert(require('module-k') === moduleExports, - 'Module exports the same thing as before'); - }); + it('doesn\'t load plugins with falsey paths', () => { + makePluginLoader({plugins: {'small-number': ''}}).activate(); + assert.strictEqual(require('small-number').value, 0); + }); - /** - * Loads plugins with bad patches and ensures that they throw/log - */ - it('throws and warns for serious problems', function() { - addModuleMock('module-l', '1.0.0', {}); - addModuleMock('module-l-plugin-noop', '', [{}]); - addModuleMock('module-l-plugin-pi', '', [{ - patch: function() {}, - intercept: function() { return 'intercepted'; } - }]); - addModuleMock('module-l-plugin-upi', '', [{ - unpatch: function() {}, - intercept: function() { return 'intercepted'; } - }]); - addModuleMock('module-l-plugin-noup', '', [{ - patch: function(m) { m.patched = true; } - }]); - - pluginLoader.activate(logger, { - plugins: { - 'module-l': 'module-l-plugin-noop' - } + it('uses version ranges to determine how to patch internals', () => { + makePluginLoader({ + plugins: {'my-version': 'plugin-my-version-1'} + }).activate(); + assert.strictEqual(require('my-version-1.0'), '1.0.0-patched'); + // v1.1 has different internals. + assert.strictEqual(require('my-version-1.1'), '1.1.0-patched'); + assert.strictEqual(require('my-version-2.0'), '2.0.0'); + // warns for my-version-2.0 that nothing matches + assert.strictEqual( + logger.getNumLogsWith('warn', '[my-version@2.0.0]'), 1); }); - assert.throws(function() { require('module-l'); }, - 'Loading patch object with no patch/intercept function throws'); - pluginLoader.deactivate(); - pluginLoader.activate(logger, { - plugins: { - 'module-l': 'module-l-plugin-pi' - } + it('patches pre-releases, but warns', () => { + makePluginLoader({ + plugins: {'my-version': 'plugin-my-version-1'} + }).activate(); + assert.strictEqual(require('my-version-1.0-pre'), '1.0.0-pre-patched'); + assert.strictEqual( + logger.getNumLogsWith('warn', '[my-version@1.0.0-pre]'), 1); }); - assert.throws(function() { require('module-l'); }, - 'Loading patch object with both patch/intercept functions throws'); - pluginLoader.deactivate(); - pluginLoader.activate(logger, { - plugins: { - 'module-l': 'module-l-plugin-upi' + it('throws when the plugin throws', () => { + makePluginLoader({ + plugins: {'my-version': 'plugin-my-version-2'} + }).activate(); + let threw = false; + try { + require('my-version-1.0'); + } catch (e) { + threw = true; } + assert.ok(threw); }); - assert.strictEqual(require('module-l'), 'intercepted'); - assert(logs.warn.indexOf('unpatch is not compatible with intercept') !== -1, - 'Warn when plugin has both unpatch and intercept'); - pluginLoader.deactivate(); - - pluginLoader.activate(logger, { - plugins: { - 'module-l': 'module-l-plugin-noup' - } + + it('warns when a module is patched by a non-conformant plugin', () => { + makePluginLoader({plugins: {'[core]': 'plugin-core'}}).activate(); + // Reasons for warnings issued are listed as comments. + require('crypto'); // neither patch nor intercept + require('os'); // both patch and intercept + require('dns'); // two Patch objects for a single file + assert.strictEqual( + logger.getNumLogsWith('warn', `[[core]@${PROCESS_VERSION}:crypto]`), + 1); + assert.strictEqual( + logger.getNumLogsWith('warn', `[[core]@${PROCESS_VERSION}:os]`), 1); + assert.strictEqual( + logger.getNumLogsWith('warn', `[[core]@${PROCESS_VERSION}:dns]`), 1); }); - assert.ok(require('module-l').patched); - assert(logs.warn.indexOf('without accompanying unpatch') !== -1, - 'Warn when plugin has both patch and no unpatch'); }); }); - -export default {}; diff --git a/tsconfig.json b/tsconfig.json index eab7683b6..4fc4d503a 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -21,6 +21,7 @@ "test/nocks.ts", "test/test-config-credentials.ts", "test/test-modules-loaded-before-agent.ts", + "test/test-plugin-loader.ts", "test/test-trace-cluster.ts", "test/test-trace-web-frameworks.ts", "test/trace.ts", From c40a9fd878adb2bfb66e4d658783227134f3a1bc Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 19 Feb 2018 17:27:16 -0800 Subject: [PATCH 4/6] test: update other tests foo --- test/plugins/common.ts | 2 +- test/test-agent-stopped.ts | 6 + test/test-config-credentials.ts | 4 +- test/test-config-plugins.ts | 96 +++++++-------- test/test-grpc-context.ts | 16 +-- test/test-trace-cluster.ts | 5 + test/test-trace-web-frameworks.ts | 5 + test/test-unpatch.ts | 4 - test/test-util.ts | 196 ++++++++++-------------------- test/trace.ts | 29 ++++- tsconfig.json | 2 + 11 files changed, 164 insertions(+), 201 deletions(-) diff --git a/test/plugins/common.ts b/test/plugins/common.ts index 73bd8d9ce..b05a4359c 100644 --- a/test/plugins/common.ts +++ b/test/plugins/common.ts @@ -143,7 +143,7 @@ function assertDurationCorrect(expectedDuration, predicate) { function runInTransaction(fn) { testTraceAgent.runInRootSpan({ name: 'outer' }, function(span) { - fn(function() { + return fn(function() { assert.strictEqual(span.type, SpanDataType.ROOT); span.endSpan(); }); diff --git a/test/test-agent-stopped.ts b/test/test-agent-stopped.ts index 3d95c836c..b8facefe0 100644 --- a/test/test-agent-stopped.ts +++ b/test/test-agent-stopped.ts @@ -22,6 +22,8 @@ var assert = require('assert'); var http = require('http'); var nock = require('nock'); var trace = require('../..'); +var pluginLoader = require('../src/trace-plugin-loader').pluginLoader; +var PluginLoaderState = require('../src/trace-plugin-loader').PluginLoaderState; describe('test-agent-stopped', function() { var agent; @@ -47,6 +49,10 @@ describe('test-agent-stopped', function() { process.env.GCLOUD_PROJECT = savedProject; }); + it('deactivates the plugin loader', () => { + assert.notStrictEqual(pluginLoader.get()!.state, PluginLoaderState.ACTIVATED); + }); + describe('express', function() { it('should not break if no project number is found', function(done) { assert.ok(!agent.isActive()); diff --git a/test/test-config-credentials.ts b/test/test-config-credentials.ts index f26d739b9..c828cf2ec 100644 --- a/test/test-config-credentials.ts +++ b/test/test-config-credentials.ts @@ -47,13 +47,13 @@ describe('Credentials Configuration', () => { before(() => { savedProject = process.env.GCLOUD_PROJECT; process.env.GCLOUD_PROJECT = '0'; - trace.setTraceWriterEnabled(true); + trace.setTraceWriter(); disableNetConnect(); }); after(() => { process.env.GCLOUD_PROJECT = savedProject; - trace.setTraceWriterEnabled(false); + trace.setTraceWriter(trace.TestTraceWriter); enableNetConnect(); }); diff --git a/test/test-config-plugins.ts b/test/test-config-plugins.ts index 7580806da..ff3741543 100644 --- a/test/test-config-plugins.ts +++ b/test/test-config-plugins.ts @@ -14,73 +14,65 @@ * limitations under the License. */ -'use strict'; +import {Logger} from '@google-cloud/common'; +import * as assert from 'assert'; -import * as pluginLoader from '../src/trace-plugin-loader'; +import {defaultConfig} from '../src/config'; +import {PluginLoader, PluginLoaderConfig} from '../src/trace-plugin-loader'; -var assert = require('assert'); -var shimmer = require('shimmer'); -var trace = require('../..'); +import * as trace from './trace'; -var instrumentedModules = [ - 'connect', 'express', 'generic-pool', 'grpc', 'hapi', 'http', 'http2', - 'https', 'knex', 'koa', 'mongodb-core', 'mysql', 'mysql2', 'pg', 'redis', - 'restify', -]; +describe('Configuration: Plugins', () => { + const instrumentedModules = Object.keys(defaultConfig.plugins); + let plugins: {[pluginName: string]: string}|null; -describe('plugin configuration', function() { - var plugins; + class ConfigTestPluginLoader extends PluginLoader { + constructor(logger: Logger, config: PluginLoaderConfig) { + super(logger, config); + plugins = config.plugins; + } + } - before(function() { - // When the plugin loader is activated, save the incoming value of plugins. - shimmer.wrap(pluginLoader, 'activate', function(original) { - return function(logger, config) { - plugins = config.plugins; - return original.apply(this, arguments); - }; - }); + before(() => { + trace.setPluginLoader(ConfigTestPluginLoader); }); - after(function() { - shimmer.unwrap(pluginLoader, 'activate'); + after(() => { + trace.setPluginLoader(trace.TestPluginLoader); }); - afterEach(function() { + afterEach(() => { plugins = null; }); - it('should have correct defaults', function() { - trace.start({forceNewAgent_: true}); - assert.strictEqual(JSON.stringify(Object.keys(plugins)), - JSON.stringify(instrumentedModules)); - for (var i = 0; i < instrumentedModules.length; i++) { - var name = instrumentedModules[i]; - assert.ok(plugins[name].indexOf('plugin-' + name + '.js') !== -1); - } + it('should have correct defaults', () => { + trace.start(); + assert.ok(plugins); + assert.strictEqual( + JSON.stringify(Object.keys(plugins!)), + JSON.stringify(instrumentedModules)); + instrumentedModules.forEach( + e => assert.ok(plugins![e].includes(`plugin-${e}.js`))); }); - it('should handle empty object', function() { - trace.start({forceNewAgent_: true, plugins: {}}); - assert.strictEqual(JSON.stringify(Object.keys(plugins)), - JSON.stringify(instrumentedModules)); - assert.ok(instrumentedModules.every(function(e) { - return plugins[e].indexOf('plugin-' + e + '.js') !== -1; - })); + it('should handle empty object', () => { + trace.start({plugins: {}}); + assert.ok(plugins); + assert.strictEqual( + JSON.stringify(Object.keys(plugins!)), + JSON.stringify(instrumentedModules)); + instrumentedModules.forEach( + e => assert.ok(plugins![e].includes(`plugin-${e}.js`))); }); - it('should overwrite builtin plugins correctly', function() { - trace.start({forceNewAgent_: true, plugins: { - express: 'foo' - }}); - assert.strictEqual(JSON.stringify(Object.keys(plugins)), - JSON.stringify(instrumentedModules)); - assert.ok(instrumentedModules.filter(function(e) { - return e !== 'express'; - }).every(function(e) { - return plugins[e].indexOf('plugin-' + e + '.js') !== -1; - })); - assert.strictEqual(plugins.express, 'foo'); + it('should overwrite builtin plugins correctly', () => { + trace.start({plugins: {express: 'foo'}}); + assert.ok(plugins); + assert.strictEqual( + JSON.stringify(Object.keys(plugins!)), + JSON.stringify(instrumentedModules)); + instrumentedModules.filter(e => e !== 'express') + .forEach(e => assert.ok(plugins![e].includes(`plugin-${e}.js`))); + assert.strictEqual(plugins!.express, 'foo'); }); }); - -export default {}; diff --git a/test/test-grpc-context.ts b/test/test-grpc-context.ts index 2b593c6c5..9cc06d215 100644 --- a/test/test-grpc-context.ts +++ b/test/test-grpc-context.ts @@ -15,9 +15,17 @@ */ 'use strict'; +// Trace agent must be started out of the loop over gRPC versions, +// because express can't be re-patched. +var agent = require('../..').start({ + projectId: '0', + samplingRate: 0 +}); + var common = require('./plugins/common'/*.js*/); var assert = require('assert'); +var express = require('./plugins/fixtures/express4'); var http = require('http'); var versions = { @@ -49,14 +57,6 @@ function requestAndSendHTTPStatus(res, expectedReqs) { }, expectedReqs); } -// Trace agent must be started out of the loop over gRPC versions, -// because express can't be re-patched. -var agent = require('../..').start({ - projectId: '0', - samplingRate: 0 -}); -var express = require('./plugins/fixtures/express4'); - Object.keys(versions).forEach(function(version) { var grpc; var httpLogCount; diff --git a/test/test-trace-cluster.ts b/test/test-trace-cluster.ts index df8f736d3..184d375a6 100644 --- a/test/test-trace-cluster.ts +++ b/test/test-trace-cluster.ts @@ -29,11 +29,16 @@ describe('test-trace-cluster', () => { let axios: typeof axiosModule; let express: typeof expressModule; before(() => { + trace.setPluginLoader(); trace.start(); express = require('express'); axios = require('axios'); }); + after(() => { + trace.setPluginLoader(trace.TestPluginLoader); + }); + it('should not interfere with express span', async () => { if (cluster.isMaster) { await new Promise(resolve => { diff --git a/test/test-trace-web-frameworks.ts b/test/test-trace-web-frameworks.ts index f3c9259ec..89ce6e299 100644 --- a/test/test-trace-web-frameworks.ts +++ b/test/test-trace-web-frameworks.ts @@ -55,10 +55,15 @@ const FRAMEWORKS: WebFrameworkConstructor[] = [ describe('Web framework tracing', () => { let axios: typeof axiosModule; before(() => { + trace.setPluginLoader(); trace.start({ignoreUrls: [/ignore-me/]}); axios = require('axios'); }); + after(() => { + trace.setPluginLoader(trace.TestPluginLoader); + }); + FRAMEWORKS.forEach((webFrameworkConstructor) => { const commonName = webFrameworkConstructor.commonName; const expectedTopStackFrame = webFrameworkConstructor.expectedTopStackFrame; diff --git a/test/test-unpatch.ts b/test/test-unpatch.ts index 4e66be1e3..67d13c737 100644 --- a/test/test-unpatch.ts +++ b/test/test-unpatch.ts @@ -66,10 +66,6 @@ describe('index.js', function() { }); } - it('should wrap/unwrap module._load on start/stop', function() { - wrapTest(require('module'), '_load'); - }); - it('should wrap/unwrap http on start/stop', function() { var http = require('http'); wrapTest(http, 'request'); diff --git a/test/test-util.ts b/test/test-util.ts index f4a0259b3..cc4400945 100644 --- a/test/test-util.ts +++ b/test/test-util.ts @@ -14,29 +14,23 @@ * limitations under the License. */ -'use strict'; +import {Logger} from '@google-cloud/common'; +import * as assert from 'assert'; +import {inspect} from 'util'; -import { Constants } from '../src/constants'; +import {Constants} from '../src/constants'; import * as util from '../src/util'; -import { Logger } from '@google-cloud/common'; - -var assert = require('assert'); -var common = require('./plugins/common'/*.js*/); -var inspect = require('util').inspect; -var Module = require('module'); -var semver = require('semver'); -var path = require('path'); - -function notNull(arg: T|null): T { - assert.ok(arg); - return arg as T; -} - -// TODO(kjin): Use TypeScript in the rest of this file. This is already done -// in PR #686. + +import {TestLogger} from './logger'; + +const notNull = (x: T|null|undefined): T => { + assert.notStrictEqual(x, null); + assert.notStrictEqual(x, undefined); + return x as T; +}; + describe('Singleton', () => { - // A real test logger class is also introduced as part of #686. - const logger = {} as any as Logger; + const logger = new TestLogger(); class MyClass { constructor(public logger: Logger, public config: {}) {} } @@ -60,12 +54,13 @@ describe('Singleton', () => { assert.throws(() => singleton.create(logger, {})); }); - it('creates a new instance when forceNewAgent_ is true in the config', () => { - const singleton = new util.Singleton(MyClass); - const createResult1 = singleton.create(logger, {}); - const createResult2 = singleton.create(logger, { forceNewAgent_: true }); - assert.notStrictEqual(createResult1, createResult2); - }); + it('creates a new instance when forceNewAgent_ is true in the config', + () => { + const singleton = new util.Singleton(MyClass); + const createResult1 = singleton.create(logger, {}); + const createResult2 = singleton.create(logger, {forceNewAgent_: true}); + assert.notStrictEqual(createResult1, createResult2); + }); }); describe('get', () => { @@ -83,148 +78,91 @@ describe('Singleton', () => { it('does not return a stale value', () => { const singleton = new util.Singleton(MyClass); singleton.create(logger, {}); - const createResult = singleton.create(logger, { forceNewAgent_: true }); + const createResult = singleton.create(logger, {forceNewAgent_: true}); const getResult = singleton.get(); assert.strictEqual(getResult, createResult); }); }); }); -describe('util.truncate', function() { - it('should truncate objects larger than size', function() { +describe('util.truncate', () => { + it('should truncate objects larger than size', () => { assert.strictEqual(util.truncate('abcdefghijklmno', 5), 'ab...'); }); - it('should not truncate objects smaller than size', function() { + it('should not truncate objects smaller than size', () => { assert.strictEqual(util.truncate('abcdefghijklmno', 50), 'abcdefghijklmno'); }); - it('should handle unicode characters', function() { - var longName = Array(120).join('☃'); - assert.strictEqual(util.truncate(longName, Constants.TRACE_SERVICE_SPAN_NAME_LIMIT), - Array(42).join('☃') + '...'); - }); -}); - -describe('util.packageNameFromPath', function() { - it('should work for standard packages', function() { - var p = path.join('.', - 'appengine-sails', - 'node_modules', - 'testmodule', - 'index.js'); - assert.equal(util.packageNameFromPath(p), - 'testmodule'); - }); - - it('should work for namespaced packages', function() { - var p = path.join('.', - 'appengine-sails', - 'node_modules', - '@google', - 'cloud-trace', - 'index.js'); - assert.equal(util.packageNameFromPath(p), - path.join('@google','cloud-trace')); - }); -}); - -describe('util.findModuleVersion', function() { - it('should correctly find package.json for userspace packages', function() { - var pjson = require('../../package.json'); - var modulePath = util.findModulePath('glob', module); - assert(semver.satisfies(util.findModuleVersion(modulePath, Module._load), - pjson.devDependencies.glob)); - }); - - it('should not break for core packages', function() { - var modulePath = util.findModulePath('http', module); - assert.equal(util.findModuleVersion(modulePath, Module._load), process.version); - }); - - it('should work with namespaces', function() { - var modulePath = util.findModulePath('@google-cloud/common', module); - var truePackage = - require('../../node_modules/@google-cloud/common/package.json'); - assert.equal(util.findModuleVersion(modulePath, Module._load), truePackage.version); + it('should handle unicode characters', () => { + const longName = new Array(120).join('☃'); + assert.strictEqual( + util.truncate(longName, Constants.TRACE_SERVICE_SPAN_NAME_LIMIT), + `${new Array(42).join('☃')}...`); }); }); -describe('util.parseContextFromHeader', function() { - describe('valid inputs', function() { - it('should return expected values: 123456/667;o=1', function() { - var result = notNull(util.parseContextFromHeader( - '123456/667;o=1')); +describe('util.parseContextFromHeader', () => { + describe('valid inputs', () => { + it('should return expected values: 123456/667;o=1', () => { + const result = notNull(util.parseContextFromHeader('123456/667;o=1')); assert.strictEqual(result.traceId, '123456'); assert.strictEqual(result.spanId, '667'); assert.strictEqual(result.options, 1); }); it('should return expected values:' + - '123456/123456123456123456123456123456123456;o=1', function() { - var result = notNull(util.parseContextFromHeader( - '123456/123456123456123456123456123456123456;o=1')); - assert.strictEqual(result.traceId, '123456'); - assert.strictEqual(result.spanId, '123456123456123456123456123456123456'); - assert.strictEqual(result.options, 1); - }); - - it('should return expected values: 123456/667', function() { - var result = notNull(util.parseContextFromHeader( - '123456/667')); + '123456/123456123456123456123456123456123456;o=1', + () => { + const result = notNull(util.parseContextFromHeader( + '123456/123456123456123456123456123456123456;o=1')); + assert.strictEqual(result.traceId, '123456'); + assert.strictEqual( + result.spanId, '123456123456123456123456123456123456'); + assert.strictEqual(result.options, 1); + }); + + it('should return expected values: 123456/667', () => { + const result = notNull(util.parseContextFromHeader('123456/667')); assert.strictEqual(result.traceId, '123456'); assert.strictEqual(result.spanId, '667'); assert.strictEqual(result.options, undefined); }); }); - describe('invalid inputs', function() { - var inputs = [ - '', - null, - undefined, - '123456', - '123456;o=1', - 'o=1;123456', - '123;456;o=1', - '123/o=1;456', - '123/abc/o=1' + describe('invalid inputs', () => { + const inputs = [ + '', null, undefined, '123456', '123456;o=1', 'o=1;123456', '123;456;o=1', + '123/o=1;456', '123/abc/o=1' ]; - inputs.forEach(function(s: any) { - it('should reject ' + s, function() { - var result = util.parseContextFromHeader(s); + inputs.forEach(s => { + it(`should reject ${s}`, () => { + // TS: Cast s as any rather than coerce it to a value + // tslint:disable-next-line:no-any + const result = util.parseContextFromHeader(s as any); assert.ok(!result); }); }); }); }); -describe('util.generateTraceContext', function() { - var inputs = [ - { - traceId: '123456', - spanId: '667', - options: 1 - }, - { - traceId: '123456', - spanId: '667', - options: undefined - } +describe('util.generateTraceContext', () => { + const inputs: util.TraceContext[] = [ + {traceId: '123456', spanId: '667', options: 1}, + {traceId: '123456', spanId: '667', options: undefined} ]; - inputs.forEach(function(s) { - it('returns well-formatted trace context for ' + inspect(s), function() { - var context = util.generateTraceContext(s); - var parsed = util.parseContextFromHeader(context); + inputs.forEach(s => { + it(`returns well-formatted trace context for ${inspect(s)}`, () => { + const context = util.generateTraceContext(s); + const parsed = util.parseContextFromHeader(context); assert.deepEqual(parsed, s); }); }); - it('returns an empty string if passed a falsy value', function() { - var context = util.generateTraceContext(null as any); - assert.equal(context, ''); + it('returns an empty string if passed a falsy value', () => { + // tslint:disable-next-line:no-any + const context = util.generateTraceContext(null as any); + assert.strictEqual(context, ''); }); }); - -export default {}; diff --git a/test/trace.ts b/test/trace.ts index 30358ae7d..39ef7f56a 100644 --- a/test/trace.ts +++ b/test/trace.ts @@ -19,9 +19,11 @@ * for testing purposes. The differences are that: * - The Trace Writer singleton is mocked to make no network requests, writing * traces to a local store instead. + * - The Plugin Loader singleton is mocked to do nothing. * - When started, the Trace Agent is initialized with a samplingRate of zero by * default (but this can be overridden). * - Additional methods to query/delete spans written locally are exposed. + * - Additional methods to override singleton values are exposed. * * Most tests should include this file instead of the main module root. */ @@ -38,6 +40,7 @@ import * as trace from '../src'; import {Config, PluginTypes} from '../src'; import {RootSpanData} from '../src/span-data'; import {Trace, TraceSpan} from '../src/trace'; +import {PluginLoader, pluginLoader, PluginLoaderConfig} from '../src/trace-plugin-loader'; import {LabelObject, TraceWriter, traceWriter, TraceWriterConfig} from '../src/trace-writer'; export {Config, PluginTypes}; @@ -45,7 +48,7 @@ export {Config, PluginTypes}; const traces = new Map(); const allSpans: TraceSpan[] = []; -class TestTraceWriter extends TraceWriter { +export class TestTraceWriter extends TraceWriter { initialize(cb: (err?: Error) => void): void { this.getConfig().projectId = '0'; cb(); @@ -61,16 +64,24 @@ class TestTraceWriter extends TraceWriter { }); } } -setTraceWriterEnabled(false); -export function setTraceWriterEnabled(enabled: boolean) { - traceWriter['implementation'] = enabled ? TraceWriter : TestTraceWriter; +export class TestPluginLoader extends PluginLoader { + activate(): PluginLoader { + return this; + } + deactivate(): PluginLoader { + return this; + } } +setTraceWriter(TestTraceWriter); +setPluginLoader(TestPluginLoader); + export type Predicate = (value: T) => boolean; export function start(projectConfig?: Config): PluginTypes.TraceAgent { - const agent = trace.start(Object.assign({samplingRate: 0}, projectConfig)); + const agent = trace.start( + Object.assign({samplingRate: 0, forceNewAgent_: true}, projectConfig)); return agent; } @@ -78,6 +89,14 @@ export function get(): PluginTypes.TraceAgent { return trace.get(); } +export function setTraceWriter(impl?: typeof TraceWriter) { + traceWriter['implementation'] = impl || TraceWriter; +} + +export function setPluginLoader(impl?: typeof PluginLoader) { + pluginLoader['implementation'] = impl || PluginLoader; +} + export function getTraces(predicate?: Predicate): string[] { if (!predicate) { predicate = () => true; diff --git a/tsconfig.json b/tsconfig.json index 4fc4d503a..bae1c394e 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -19,11 +19,13 @@ "test/plugins/test-trace-http2.ts", "test/logger.ts", "test/nocks.ts", + "test/test-config-plugins.ts", "test/test-config-credentials.ts", "test/test-modules-loaded-before-agent.ts", "test/test-plugin-loader.ts", "test/test-trace-cluster.ts", "test/test-trace-web-frameworks.ts", + "test/test-util.ts", "test/trace.ts", "test/utils.ts", "test/web-frameworks/*.ts" From bd9160e779342a3bf83998f21bcc5ad63191c5e5 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 19 Mar 2018 13:55:20 -0700 Subject: [PATCH 5/6] fix: drop warning message for non-patched modules --- src/trace-plugin-loader.ts | 3 --- test/test-plugin-loader.ts | 5 +++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/trace-plugin-loader.ts b/src/trace-plugin-loader.ts index 31b0dea5b..15557eb6d 100644 --- a/src/trace-plugin-loader.ts +++ b/src/trace-plugin-loader.ts @@ -193,9 +193,6 @@ export class ModulePluginWrapper implements PluginWrapper { } else if (instrumentation.intercept) { exportedValue = instrumentation.intercept( exportedValue, this.createTraceAgentInstance(file)); - } else { - this.logger.warn(`PluginWrapper#applyPlugin: [${ - logString}] Patch object has no known functions for patching this file.`); } return exportedValue; }, moduleExports as T); diff --git a/test/test-plugin-loader.ts b/test/test-plugin-loader.ts index c6e5237d9..afa5c332c 100644 --- a/test/test-plugin-loader.ts +++ b/test/test-plugin-loader.ts @@ -324,13 +324,14 @@ describe('Trace Plugin Loader', () => { it('warns when a module is patched by a non-conformant plugin', () => { makePluginLoader({plugins: {'[core]': 'plugin-core'}}).activate(); - // Reasons for warnings issued are listed as comments. + // Reasons for possible warnings issued are listed as comments. require('crypto'); // neither patch nor intercept require('os'); // both patch and intercept require('dns'); // two Patch objects for a single file + // Do not warn when there is no patch/intercept function. assert.strictEqual( logger.getNumLogsWith('warn', `[[core]@${PROCESS_VERSION}:crypto]`), - 1); + 0); assert.strictEqual( logger.getNumLogsWith('warn', `[[core]@${PROCESS_VERSION}:os]`), 1); assert.strictEqual( From eb986a3e91711ab907cd7232b85114017e9b66c5 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Tue, 20 Mar 2018 17:13:54 -0700 Subject: [PATCH 6/6] refactor: address comments --- src/index.ts | 5 +-- src/plugins/plugin-knex.ts | 1 + src/trace-plugin-loader.ts | 91 +++++++++++++++++++------------------- test/logger.ts | 2 +- test/test-plugin-loader.ts | 37 ++++------------ 5 files changed, 57 insertions(+), 79 deletions(-) diff --git a/src/index.ts b/src/index.ts index 7e7bbac8a..0d8b86937 100644 --- a/src/index.ts +++ b/src/index.ts @@ -117,9 +117,8 @@ function stop() { traceWriter.get().stop(); traceAgent.disable(); try { - const loader = pluginLoader.get(); - loader.deactivate(); - } catch (e) { + const loader = pluginLoader.get().deactivate(); + } catch { // Plugin loader wasn't even created. No need to de-activate } cls.destroyNamespace(); diff --git a/src/plugins/plugin-knex.ts b/src/plugins/plugin-knex.ts index 4f5b926dd..746fbe612 100644 --- a/src/plugins/plugin-knex.ts +++ b/src/plugins/plugin-knex.ts @@ -55,6 +55,7 @@ module.exports = [ unpatch: unpatchClient }, { + // knex 0.10.x and 0.11.x do not need patching versions: '>=0.10 <=0.11' } ]; diff --git a/src/trace-plugin-loader.ts b/src/trace-plugin-loader.ts index 15557eb6d..3687f4617 100644 --- a/src/trace-plugin-loader.ts +++ b/src/trace-plugin-loader.ts @@ -1,5 +1,5 @@ /** - * Copyright 2017 Google Inc. All Rights Reserved. + * Copyright 2018 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -94,19 +94,19 @@ export class ModulePluginWrapper implements PluginWrapper { // Sentinel value to indicate that a plugin has not been loaded into memory // yet. private static readonly NOT_LOADED: Plugin = []; - private unpatchFns: Array<() => void> = []; + private readonly unpatchFns: Array<() => void> = []; // A logger. - private logger: Logger; + private readonly logger: Logger; // Configuration for a TraceAgent instance. - private traceConfig: TraceAgentConfig; + private readonly traceConfig: TraceAgentConfig; // Display-friendly name of the module being patched by this plugin. - private name: string; + private readonly name: string; // The path to the plugin. - private path: string; + private readonly path: string; // The exported value of the plugin, or NOT_LOADED if it hasn't been // loaded yet. private pluginExportedValue: Plugin = ModulePluginWrapper.NOT_LOADED; - private traceApiInstances: TraceAgent[] = []; + private readonly traceApiInstances: TraceAgent[] = []; /** * Constructs a new PluginWrapper instance. @@ -129,9 +129,9 @@ export class ModulePluginWrapper implements PluginWrapper { // Count the number of Patch/Intercept objects with compatible version // ranges let numFiles = 0; - plugin.forEach(instrumentation => { - const postLoadVersions = instrumentation.versions; - if (!postLoadVersions || semver.satisfies(version, postLoadVersions)) { + plugin.forEach(patch => { + const versionRange = patch.versions; + if (!versionRange || semver.satisfies(version, versionRange)) { numFiles++; } }); @@ -141,6 +141,9 @@ export class ModulePluginWrapper implements PluginWrapper { } unapplyAll() { + // Unpatch in reverse order of when patches were applied, because each + // unpatch function expects the state of the module to be as its associated + // patch function left it. this.unpatchFns.reverse().forEach(fn => fn()); this.unpatchFns.length = 0; this.traceApiInstances.forEach(traceApi => traceApi.disable()); @@ -157,42 +160,40 @@ export class ModulePluginWrapper implements PluginWrapper { // plugin exported value with matching file/version fields. const supportedPatches: Array&Intercept>> = plugin.filter( - instrumentation => - semver.satisfies(version, instrumentation.versions || '*') && - (file === instrumentation.file || - (!file && !instrumentation.file))); + patch => semver.satisfies(version, patch.versions || '*') && + (file === patch.file || (!file && !patch.file))); if (supportedPatches.length > 1) { this.logger.warn(`PluginWrapper#applyPlugin: [${ logString}] Plugin has more than one patch/intercept object for this file. Applying all.`); } // Apply each patch object. - return supportedPatches.reduce((exportedValue, instrumentation) => { + return supportedPatches.reduce((exportedValue, patch) => { // TODO(kjin): The only benefit of creating a new TraceAgent object per // patched file is to give us granularity in log messages. See if we can // refactor the TraceAgent class to avoid this. this.logger.info( `PluginWrapper#applyPlugin: [${logString}] Applying plugin.`); - if (instrumentation.patch) { - instrumentation.patch( - exportedValue, this.createTraceAgentInstance(logString)); + if (patch.patch) { + patch.patch(exportedValue, this.createTraceAgentInstance(logString)); // Queue a function to run if the plugin gets disabled. - if (instrumentation.unpatch) { + if (patch.unpatch) { + const unpatch = patch.unpatch; this.unpatchFns.push(() => { this.logger.info( `PluginWrapper#unapplyAll: [${logString}] Unpatching file.`); - instrumentation.unpatch!(exportedValue); + unpatch(exportedValue); }); } // The patch object should only have either patch() or intercept(). - if (instrumentation.intercept) { + if (patch.intercept) { this.logger.warn(`PluginWrapper#applyPlugin: [${ logString}] Patch object has both patch() and intercept() for this file. Only applying patch().`); } - } else if (instrumentation.intercept) { - exportedValue = instrumentation.intercept( - exportedValue, this.createTraceAgentInstance(file)); + } else if (patch.intercept) { + exportedValue = + patch.intercept(exportedValue, this.createTraceAgentInstance(file)); } return exportedValue; }, moduleExports as T); @@ -216,13 +217,13 @@ export class ModulePluginWrapper implements PluginWrapper { /** * A class that represents wrapper logic on top of plugins that patch core - * modules. Core modules are different because (1) they can be required by the - * plugins that patch them, and (2) the core module being patched doesn't - * necessarily correspond to the name of the plugin. + * (built-in) modules. Core modules are different because (1) they can be + * required by the plugins that patch them, and (2) the core module being + * patched doesn't necessarily correspond to the name of the plugin. */ export class CorePluginWrapper implements PluginWrapper { - private logger: Logger; - private children: ModulePluginWrapper[]; + private readonly logger: Logger; + private readonly children: ModulePluginWrapper[]; constructor( logger: Logger, config: CorePluginWrapperOptions, @@ -257,9 +258,10 @@ export class CorePluginWrapper implements PluginWrapper { * @param version The module version. */ applyPlugin(moduleExports: T, file: string, version: string): T { - return this.children.reduce((exportedValue, child) => { - return child.applyPlugin(exportedValue, file, version); - }, moduleExports); + return this.children.reduce( + (exportedValue, child) => + child.applyPlugin(exportedValue, file, version), + moduleExports); } } @@ -279,12 +281,10 @@ export class PluginLoader { static readonly CORE_MODULE = '[core]'; // The function to call to register a require hook. private enableRequireHook: (onRequire: hook.OnRequireFn) => void; - // A logger. - private logger: Logger; // A map mapping module names to their respective plugins. - private pluginMap: Map = new Map(); + private readonly pluginMap: Map = new Map(); // A map caching version strings for a module based on their base path. - private moduleVersionCache: Map = new Map(); + private readonly moduleVersionCache: Map = new Map(); // The current state of the plugin loader. private internalState: PluginLoaderState = PluginLoaderState.NO_HOOK; @@ -293,9 +293,7 @@ export class PluginLoader { * @param logger The logger to use. * @param config The configuration for this instance. */ - constructor(logger: Logger, config: PluginLoaderConfig) { - this.logger = logger; - + constructor(private readonly logger: Logger, config: PluginLoaderConfig) { const nonCoreModules: string[] = []; // Initialize ALL of the PluginWrapper objects here. // See CorePluginWrapper docs for why core modules are processed @@ -304,8 +302,8 @@ export class PluginLoader { Object.keys(config.plugins).forEach(key => { const value = config.plugins[key]; // Core module plugins share a common key. - const coreModule = builtinModules.indexOf(key) !== -1 || - key === PluginLoader.CORE_MODULE; + const coreModule = key === PluginLoader.CORE_MODULE || + builtinModules.indexOf(key) !== -1; if (value) { // Convert the given string value to a PluginConfigEntry @@ -430,6 +428,8 @@ export class PluginLoader { } this.internalState = PluginLoaderState.DEACTIVATED; this.logger.info(`PluginLoader#deactivate: Deactivated.`); + } else { + throw new Error('Plugin loader is not activated.'); } return this; } @@ -438,7 +438,7 @@ export class PluginLoader { * Adds a search path for plugin modules. Intended for testing purposes only. * @param searchPath The path to add. */ - static setPluginSearchPath(searchPath: string) { + static setPluginSearchPathForTestingOnly(searchPath: string) { module.paths = [searchPath]; } @@ -451,10 +451,9 @@ export class PluginLoader { static parseModuleString(moduleStr: string): {name: string, file: string} { // Canonicalize the name by using only '/'. const parts = moduleStr.replace(/\\/g, '/').split('/'); - let indexOfFile = 1; - if (parts[0].startsWith('@')) { // for @org/package - indexOfFile = 2; - } + // The separation index between name/file depends on whether the module + // is namespaced. + const indexOfFile = parts[0].startsWith('@') ? 2 : 1; return { name: parts.slice(0, indexOfFile).join('/'), file: parts.slice(indexOfFile).join('/') diff --git a/test/logger.ts b/test/logger.ts index 51b0c40f3..b7ce4c178 100644 --- a/test/logger.ts +++ b/test/logger.ts @@ -1,5 +1,5 @@ /** - * Copyright 2018 Google Inc. All Rights Reserved. + * Copyright 2018 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/test/test-plugin-loader.ts b/test/test-plugin-loader.ts index afa5c332c..000d365e3 100644 --- a/test/test-plugin-loader.ts +++ b/test/test-plugin-loader.ts @@ -1,5 +1,5 @@ /** - * Copyright 2018 Google Inc. All Rights Reserved. + * Copyright 2018 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -54,7 +54,7 @@ describe('Trace Plugin Loader', () => { before(() => { module.paths.push(SEARCH_PATH); - PluginLoader.setPluginSearchPath(SEARCH_PATH); + PluginLoader.setPluginSearchPathForTestingOnly(SEARCH_PATH); logger = new TestLogger(); }); @@ -137,23 +137,21 @@ describe('Trace Plugin Loader', () => { assert.ok(plugin.unapplyCalled); }); - it('does nothing when already deactivated', () => { + it('throws when internal state is not ACTIVATED', () => { const pluginLoader = makePluginLoader({plugins: {}}); assert.strictEqual(pluginLoader.state, PluginLoaderState.NO_HOOK); const plugin = new TestPluginWrapper(); // TODO(kjin): Stop using index properties. pluginLoader['pluginMap'].set('foo', plugin); - pluginLoader.deactivate(); - assert.strictEqual(pluginLoader.state, PluginLoaderState.NO_HOOK); + assert.throws(() => pluginLoader.deactivate()); assert.ok(!plugin.unapplyCalled); pluginLoader.activate().deactivate(); assert.strictEqual(pluginLoader.state, PluginLoaderState.DEACTIVATED); plugin.unapplyCalled = false; - pluginLoader.deactivate(); - assert.strictEqual(pluginLoader.state, PluginLoaderState.DEACTIVATED); + assert.throws(() => pluginLoader.deactivate()); assert.ok(!plugin.unapplyCalled); }); }); @@ -189,12 +187,12 @@ describe('Trace Plugin Loader', () => { assert.strictEqual(require('my-version-2.0'), '2.0.0'); }); - it('doesn\'t patch before activation', () => { + it(`doesn't patch before activation`, () => { makePluginLoader({plugins: {'small-number': 'plugin-small-number'}}); assert.strictEqual(require('small-number').value, 0); }); - it('doesn\'t patch modules for which plugins aren\'t specified', () => { + it(`doesn't patch modules for which plugins aren't specified`, () => { makePluginLoader({plugins: {}}).activate(); assert.strictEqual(require('small-number').value, 0); }); @@ -232,25 +230,6 @@ describe('Trace Plugin Loader', () => { logger.getNumLogsWith('info', '[small-number@0.0.1]'), 2); }); - it('doesn\'t unpatch twice', () => { - const loader = makePluginLoader({ - plugins: {'small-number': 'plugin-small-number'} - }).activate(); - require('small-number'); - loader.deactivate().deactivate(); - assert.strictEqual(require('small-number').value, 0); - // One each for activate/deactivate - assert.strictEqual( - logger.getNumLogsWith('info', '[small-number@0.0.1]'), 2); - }); - - it('doesn\'t unpatch modules when deactivated immediately', () => { - makePluginLoader({ - plugins: {'small-number': 'plugin-small-number'} - }).deactivate(); - assert.strictEqual(require('small-number').value, 0); - }); - it('intercepts and patches internal files', () => { makePluginLoader({ plugins: {'large-number': 'plugin-large-number'} @@ -282,7 +261,7 @@ describe('Trace Plugin Loader', () => { 'The lab-grown ketchup Fox jumps over the chili Dog'); }); - it('doesn\'t load plugins with falsey paths', () => { + it(`doesn't load plugins with falsey paths`, () => { makePluginLoader({plugins: {'small-number': ''}}).activate(); assert.strictEqual(require('small-number').value, 0); });