From 4a4db5ec852fee4ce6aeb0edab5422a3861b5476 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 5 Oct 2020 12:25:29 +0200 Subject: [PATCH 1/2] MONGOSH-376 - Include metadata about data source in result For results from calls on collections, add metadata about the source of the returned data (database name, collection name, method, arguments) so that consumers of the shell can figure out from what source the result was generated, e.g. in order to understand how to modify that original data. This also adds a fake `Document` type to the `ShellResult` reported for e.g. `findOne()`, so that it is possible to tell those apart from other plain JavaScript objects, e.g. in order to be able to know whether the result is a modifiable object in the database or not. --- packages/i18n/src/locales/en_US.js | 7 ++ packages/shell-api/src/aggregation-cursor.ts | 6 +- packages/shell-api/src/collection.spec.ts | 62 +++++++++++++++ packages/shell-api/src/collection.ts | 14 +++- packages/shell-api/src/cursor.ts | 6 +- packages/shell-api/src/decorators.ts | 79 +++++++++++++++++++- packages/shell-api/src/helpers.ts | 9 +++ 7 files changed, 177 insertions(+), 6 deletions(-) diff --git a/packages/i18n/src/locales/en_US.js b/packages/i18n/src/locales/en_US.js index b84b54ac54..6f57b96d2a 100644 --- a/packages/i18n/src/locales/en_US.js +++ b/packages/i18n/src/locales/en_US.js @@ -1603,6 +1603,13 @@ const translations = { } } } + }, + Document: { + help: { + link: 'https://docs.mongodb.com/manual/core/document/', + description: 'A generic MongoDB document, without any methods.', + attributes: {} + } } } }, diff --git a/packages/shell-api/src/aggregation-cursor.ts b/packages/shell-api/src/aggregation-cursor.ts index 9c289f296f..907316fbe7 100644 --- a/packages/shell-api/src/aggregation-cursor.ts +++ b/packages/shell-api/src/aggregation-cursor.ts @@ -5,7 +5,8 @@ import { returnType, hasAsyncChild, ShellApiClass, - ShellResult + ShellResult, + resultSource } from './decorators'; import { Cursor as ServiceProviderCursor, @@ -47,7 +48,8 @@ export default class AggregationCursor extends ShellApiClass { async [asShellResult](): Promise { return { type: 'AggregationCursor', - value: this._mongo._serviceProvider.platform === ReplPlatform.JavaShell ? this : await this._asPrintable() + value: this._mongo._serviceProvider.platform === ReplPlatform.JavaShell ? this : await this._asPrintable(), + source: this[resultSource] ?? undefined }; } diff --git a/packages/shell-api/src/collection.spec.ts b/packages/shell-api/src/collection.spec.ts index b945691270..099fafb3bc 100644 --- a/packages/shell-api/src/collection.spec.ts +++ b/packages/shell-api/src/collection.spec.ts @@ -1114,5 +1114,67 @@ describe('Collection', () => { expect(catchedError).to.equal(expectedError); }); }); + + describe('return information about the collection as metadata', async() => { + let serviceProviderCursor: StubbedInstance; + + beforeEach(() => { + serviceProviderCursor = stubInterface(); + serviceProviderCursor.limit.returns(serviceProviderCursor); + serviceProviderCursor.hasNext.resolves(true); + serviceProviderCursor.next.resolves({ _id: 'abc' }); + }); + + it('works for find()', async() => { + serviceProvider.find.returns(serviceProviderCursor); + const cursor = await collection.find(); + const result = await cursor[asShellResult](); + expect(result.type).to.equal('Cursor'); + expect(result.value.length).to.not.equal(0); + expect(result.value[0]._id).to.equal('abc'); + expect(result.source).to.deep.equal({ + arguments: [], + call: 'find', + namespace: { + db: 'db1', + collection: 'coll1' + } + }); + }); + + it('works for findOne()', async() => { + serviceProvider.find.returns(serviceProviderCursor); + const document = await collection.findOne({ hasBanana: true }); + const result = await (document as any)[asShellResult](); + expect(result.type).to.equal('Document'); + expect(result.value._id).to.equal('abc'); + expect(result.source).to.deep.equal({ + arguments: [ { hasBanana: true } ], + call: 'findOne', + namespace: { + db: 'db1', + collection: 'coll1' + } + }); + }); + + it('works for getIndexes()', async() => { + const fakeIndex = { v: 2, key: { _id: 1 }, name: '_id_' }; + serviceProvider.getIndexes.resolves([fakeIndex]); + + const indexResult = await collection.getIndexes(); + const result = await (indexResult as any)[asShellResult](); + expect(result.type).to.equal(null); + expect(result.value).to.deep.equal([ fakeIndex ]); + expect(result.source).to.deep.equal({ + arguments: [], + call: 'getIndexes', + namespace: { + db: 'db1', + collection: 'coll1' + } + }); + }); + }); }); }); diff --git a/packages/shell-api/src/collection.ts b/packages/shell-api/src/collection.ts index b23e57d12b..cb2a92373b 100644 --- a/packages/shell-api/src/collection.ts +++ b/packages/shell-api/src/collection.ts @@ -6,7 +6,10 @@ import { ShellApiClass, returnsPromise, returnType, - serverVersions + serverVersions, + Namespace, + namespaceInfo, + addSourceToResults } from './decorators'; import { ADMIN_DB, ServerVersions } from './enums'; import { @@ -36,6 +39,7 @@ import PlanCache from './plan-cache'; @shellApiClassDefault @hasAsyncChild +@addSourceToResults export default class Collection extends ShellApiClass { _mongo: Mongo; _database: any; // to avoid circular ref @@ -64,6 +68,10 @@ export default class Collection extends ShellApiClass { return proxy; } + [namespaceInfo](): Namespace { + return { db: this._database.getName(), collection: this._name }; + } + /** * Internal method to determine what is printed for this class. */ @@ -408,6 +416,7 @@ export default class Collection extends ShellApiClass { * @returns {Cursor} The promise of the cursor. */ @returnsPromise + @returnType('Document') async findOne(query = {}, projection?): Promise { const options: any = {}; if (projection) { @@ -467,6 +476,7 @@ export default class Collection extends ShellApiClass { * @returns {Document} The promise of the result. */ @returnsPromise + @returnType('Document') @serverVersions(['3.2.0', ServerVersions.latest]) async findOneAndDelete(filter, options = {}): Promise { assertArgsDefined(filter); @@ -496,6 +506,7 @@ export default class Collection extends ShellApiClass { * @returns {Document} The promise of the result. */ @returnsPromise + @returnType('Document') @serverVersions(['3.2.0', ServerVersions.latest]) async findOneAndReplace(filter, replacement, options = {}): Promise { assertArgsDefined(filter); @@ -531,6 +542,7 @@ export default class Collection extends ShellApiClass { * @returns {Document} The promise of the result. */ @returnsPromise + @returnType('Document') @serverVersions(['3.2.0', ServerVersions.latest]) async findOneAndUpdate(filter, update, options = {}): Promise { assertArgsDefined(filter); diff --git a/packages/shell-api/src/cursor.ts b/packages/shell-api/src/cursor.ts index dcb8e53a0c..0eb9080abe 100644 --- a/packages/shell-api/src/cursor.ts +++ b/packages/shell-api/src/cursor.ts @@ -7,7 +7,8 @@ import { serverVersions, ShellApiClass, shellApiClassDefault, - ShellResult + ShellResult, + resultSource } from './decorators'; import { asShellResult, ServerVersions } from './enums'; import { @@ -33,7 +34,8 @@ export default class Cursor extends ShellApiClass { async [asShellResult](): Promise { return { type: 'Cursor', - value: this._mongo._serviceProvider.platform === ReplPlatform.JavaShell ? this : await this._asPrintable() + value: this._mongo._serviceProvider.platform === ReplPlatform.JavaShell ? this : await this._asPrintable(), + source: this[resultSource] ?? undefined }; } diff --git a/packages/shell-api/src/decorators.ts b/packages/shell-api/src/decorators.ts index 12e29991fd..68f6fbc87e 100644 --- a/packages/shell-api/src/decorators.ts +++ b/packages/shell-api/src/decorators.ts @@ -9,6 +9,13 @@ import { asShellResult } from './enums'; import { MongoshInternalError } from '@mongosh/errors'; +import { addHiddenDataProperty } from './helpers'; + +const addSourceToResultsSymbol = Symbol('@@mongosh.addSourceToResults'); +// The custom [asShellResult]() methods in Cursor and AggregationCursor require +// this, but ideally, this symbol would be local to this file. +export const resultSource = Symbol('@@mongosh.resultSource'); +export const namespaceInfo = Symbol('@@mongosh.namespaceInfo'); export interface ShellApiInterface { [asShellResult]: Function; @@ -19,9 +26,21 @@ export interface ShellApiInterface { [key: string]: any; } +export interface Namespace { + db: string; + collection: string; +} + +export interface ShellResultSourceInformation { + namespace: Namespace; + call: string; + arguments: any[]; +} + export interface ShellResult { value: any; type: string; + source?: ShellResultSourceInformation; } export class ShellApiClass implements ShellApiInterface { @@ -34,6 +53,55 @@ export class ShellApiClass implements ShellApiInterface { } } +// For classes like Collection, it can be useful to attach information to the +// result about the original data source, so that downstream consumers of the +// shell can e.g. figure out how to edit a document returned from the shell. +// To that end, we wrap the methods of a class, and report back how the +// result was generated. +// We also attach the `shellApiType` and `asShellResult` properties to the +// return type (if that is possible and they are not already present), so that +// we can also provide sensible information for methods that do not return +// shell classes, like db.coll.findOne() which returns a Document (i.e. a plain +// JavaScript object). +function wrapWithAddSourceToResult(fn: Function, functionName: string): Function { + function addSource(result: T, obj: any, args: any[]): T { + if (typeof result === 'object' && result !== null) { + const resultSourceInformation: ShellResultSourceInformation = { + namespace: obj[namespaceInfo](), + call: functionName, + arguments: args + }; + addHiddenDataProperty(result, resultSource, resultSourceInformation); + if (result[shellApiType] === undefined && (fn as any).returnType) { + addHiddenDataProperty(result, shellApiType, (fn as any).returnType); + } + if (result[asShellResult] === undefined) { + addHiddenDataProperty( + result, asShellResult, async function(): Promise { + return { + // Report { type: null } if the type is not available to match + // what the shell evaluator does when it encounters values + // that do not provide [asShellResult](). + type: this[shellApiType] || null, + value: this, + source: this[resultSource] + }; + }); + } + } + return result; + } + const wrapper = (fn as any).returnsPromise ? + async function(...args): Promise { + return addSource(await fn.call(this, ...args), this, args); + } : function(...args): any { + return addSource(fn.call(this, ...args), this, args); + }; + Object.setPrototypeOf(wrapper, Object.getPrototypeOf(fn)); + Object.defineProperties(wrapper, Object.getOwnPropertyDescriptors(fn)); + return wrapper; +} + interface TypeSignature { type: string; hasAsyncChild?: boolean; @@ -51,6 +119,7 @@ if (!global[signaturesGlobalIdentifier]) { } const signatures: Signatures = global[signaturesGlobalIdentifier]; +signatures.Document = { type: 'Document', attributes: {} }; export const toIgnore = [asShellResult, '_asPrintable', 'constructor']; export function shellApiClassDefault(constructor: Function): void { @@ -78,6 +147,10 @@ export function shellApiClassDefault(constructor: Function): void { propertyName.startsWith('_') ) continue; + if ((constructor as any)[addSourceToResultsSymbol]) { + descriptor.value = wrapWithAddSourceToResult(descriptor.value, propertyName); + } + descriptor.value.serverVersions = descriptor.value.serverVersions || ALL_SERVER_VERSIONS; descriptor.value.topologies = descriptor.value.topologies || ALL_TOPOLOGIES; descriptor.value.returnType = descriptor.value.returnType || { type: 'unknown', attributes: {} }; @@ -151,7 +224,8 @@ export function shellApiClassDefault(constructor: Function): void { constructor.prototype[asShellResult] = async function(): Promise { return { type: className, - value: await this._asPrintable() + value: await this._asPrintable(), + source: this[resultSource] ?? undefined }; }; } @@ -210,3 +284,6 @@ export function classPlatforms(platformsArray: any[]): Function { constructor.prototype.platforms = platformsArray; }; } +export function addSourceToResults(constructor: Function): void { + (constructor as any)[addSourceToResultsSymbol] = true; +} diff --git a/packages/shell-api/src/helpers.ts b/packages/shell-api/src/helpers.ts index 7b167b6cc0..d4834c8777 100644 --- a/packages/shell-api/src/helpers.ts +++ b/packages/shell-api/src/helpers.ts @@ -477,3 +477,12 @@ export function tsToSeconds(x): number { } return x / 4294967296; // low 32 bits are ordinal #s within a second } + +export function addHiddenDataProperty(target: object, key: string|symbol, value: any): void { + Object.defineProperty(target, key, { + value, + enumerable: false, + writable: true, + configurable: true + }); +} From 2aaa0ab994ab3097ff2857634118426f341be425 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 6 Oct 2020 17:18:11 +0200 Subject: [PATCH 2/2] Remove call and arguments from result source --- packages/shell-api/src/collection.spec.ts | 6 ------ packages/shell-api/src/decorators.ts | 14 +++++--------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/packages/shell-api/src/collection.spec.ts b/packages/shell-api/src/collection.spec.ts index 099fafb3bc..2ab4150f45 100644 --- a/packages/shell-api/src/collection.spec.ts +++ b/packages/shell-api/src/collection.spec.ts @@ -1133,8 +1133,6 @@ describe('Collection', () => { expect(result.value.length).to.not.equal(0); expect(result.value[0]._id).to.equal('abc'); expect(result.source).to.deep.equal({ - arguments: [], - call: 'find', namespace: { db: 'db1', collection: 'coll1' @@ -1149,8 +1147,6 @@ describe('Collection', () => { expect(result.type).to.equal('Document'); expect(result.value._id).to.equal('abc'); expect(result.source).to.deep.equal({ - arguments: [ { hasBanana: true } ], - call: 'findOne', namespace: { db: 'db1', collection: 'coll1' @@ -1167,8 +1163,6 @@ describe('Collection', () => { expect(result.type).to.equal(null); expect(result.value).to.deep.equal([ fakeIndex ]); expect(result.source).to.deep.equal({ - arguments: [], - call: 'getIndexes', namespace: { db: 'db1', collection: 'coll1' diff --git a/packages/shell-api/src/decorators.ts b/packages/shell-api/src/decorators.ts index 68f6fbc87e..f06e5525b5 100644 --- a/packages/shell-api/src/decorators.ts +++ b/packages/shell-api/src/decorators.ts @@ -33,8 +33,6 @@ export interface Namespace { export interface ShellResultSourceInformation { namespace: Namespace; - call: string; - arguments: any[]; } export interface ShellResult { @@ -63,13 +61,11 @@ export class ShellApiClass implements ShellApiInterface { // we can also provide sensible information for methods that do not return // shell classes, like db.coll.findOne() which returns a Document (i.e. a plain // JavaScript object). -function wrapWithAddSourceToResult(fn: Function, functionName: string): Function { - function addSource(result: T, obj: any, args: any[]): T { +function wrapWithAddSourceToResult(fn: Function): Function { + function addSource(result: T, obj: any): T { if (typeof result === 'object' && result !== null) { const resultSourceInformation: ShellResultSourceInformation = { namespace: obj[namespaceInfo](), - call: functionName, - arguments: args }; addHiddenDataProperty(result, resultSource, resultSourceInformation); if (result[shellApiType] === undefined && (fn as any).returnType) { @@ -93,9 +89,9 @@ function wrapWithAddSourceToResult(fn: Function, functionName: string): Function } const wrapper = (fn as any).returnsPromise ? async function(...args): Promise { - return addSource(await fn.call(this, ...args), this, args); + return addSource(await fn.call(this, ...args), this); } : function(...args): any { - return addSource(fn.call(this, ...args), this, args); + return addSource(fn.call(this, ...args), this); }; Object.setPrototypeOf(wrapper, Object.getPrototypeOf(fn)); Object.defineProperties(wrapper, Object.getOwnPropertyDescriptors(fn)); @@ -148,7 +144,7 @@ export function shellApiClassDefault(constructor: Function): void { ) continue; if ((constructor as any)[addSourceToResultsSymbol]) { - descriptor.value = wrapWithAddSourceToResult(descriptor.value, propertyName); + descriptor.value = wrapWithAddSourceToResult(descriptor.value); } descriptor.value.serverVersions = descriptor.value.serverVersions || ALL_SERVER_VERSIONS;