From 4518f59a324de6b0d592c2b4c752ca568dc00f32 Mon Sep 17 00:00:00 2001 From: Michael Rose Date: Wed, 26 May 2021 14:26:09 +0200 Subject: [PATCH 1/4] allow rephrasing errors --- packages/shell-api/src/decorators.ts | 21 ++++++-- packages/shell-api/src/mongo-errors.spec.ts | 57 +++++++++++++++++++++ packages/shell-api/src/mongo-errors.ts | 34 ++++++++++++ 3 files changed, 107 insertions(+), 5 deletions(-) create mode 100644 packages/shell-api/src/mongo-errors.spec.ts create mode 100644 packages/shell-api/src/mongo-errors.ts diff --git a/packages/shell-api/src/decorators.ts b/packages/shell-api/src/decorators.ts index 9751de5c59..39ed6115c5 100644 --- a/packages/shell-api/src/decorators.ts +++ b/packages/shell-api/src/decorators.ts @@ -12,6 +12,7 @@ import { import Help from './help'; import { addHiddenDataProperty } from './helpers'; import { checkInterrupted } from './interruptor'; +import { rephraseMongoError } from './mongo-errors'; const addSourceToResultsSymbol = Symbol.for('@@mongosh.addSourceToResults'); const resultSource = Symbol.for('@@mongosh.resultSource'); @@ -161,17 +162,27 @@ function wrapWithApiChecks any>(fn: T, className: s const internalState = getShellInternalState(this); checkForDeprecation(internalState, className, fn); const interrupted = checkInterrupted(internalState); - const result = await Promise.race([ - interrupted ? interrupted.asPromise() : new Promise(() => {}), - fn.call(this, ...args) - ]); + let result: any; + try { + result = await Promise.race([ + interrupted ? interrupted.asPromise() : new Promise(() => {}), + fn.call(this, ...args) + ]); + } catch (e) { + throw rephraseMongoError(e); + } checkInterrupted(internalState); return result; }) : function(this: any, ...args: any[]): any { const internalState = getShellInternalState(this); checkForDeprecation(internalState, className, fn); checkInterrupted(internalState); - const result = fn.call(this, ...args); + let result: any; + try { + result = fn.call(this, ...args); + } catch (e) { + throw rephraseMongoError(e); + } checkInterrupted(internalState); return result; }; diff --git a/packages/shell-api/src/mongo-errors.spec.ts b/packages/shell-api/src/mongo-errors.spec.ts new file mode 100644 index 0000000000..a5408ef680 --- /dev/null +++ b/packages/shell-api/src/mongo-errors.spec.ts @@ -0,0 +1,57 @@ +import { MongoError } from 'mongodb'; +import { expect } from 'chai'; +import { rephraseMongoError } from './mongo-errors'; + +class MongoshInternalError extends Error { + constructor(message: string) { + super(message); + this.name = 'MongoshInternalError'; + } +} + +describe('mongo-errors', () => { + describe('rephraseMongoError', () => { + context('for primitive "errors"', () => { + [ + true, + 42, + 'a message', + { some: 'object' } + ].forEach(e => { + it(`skips ${JSON.stringify(e)}`, () => { + expect(rephraseMongoError(e)).to.equal(e); + }); + }); + }); + + context('for non-MongoError errors', () => { + [ + new Error('an error'), + new MongoshInternalError("The apiVersion parameter is required, please configure your MongoClient's API version") + ].forEach(e => { + it(`ignores ${e.constructor.name} ${JSON.stringify(e)}`, () => { + const origMessage = e.message; + const r = rephraseMongoError(e); + expect(r).to.equal(r); + expect(r.message).to.equal(origMessage); + }); + }); + }); + + context('for MongoError errors', () => { + it('ignores an irrelevant error', () => { + const e = new MongoError('ignored'); + const r = rephraseMongoError(e); + expect(r).to.equal(e); + expect(r.message).to.equal('ignored'); + }); + + it('rephrases an apiVersion error', () => { + const e = new MongoError("The apiVersion parameter is required, please configure your MongoClient's API version"); + const r = rephraseMongoError(e); + expect(r).to.equal(e); + expect(r.message).to.contain('mongosh'); + }); + }); + }); +}); diff --git a/packages/shell-api/src/mongo-errors.ts b/packages/shell-api/src/mongo-errors.ts new file mode 100644 index 0000000000..023a5c23b9 --- /dev/null +++ b/packages/shell-api/src/mongo-errors.ts @@ -0,0 +1,34 @@ + +interface MongoErrorRephrase { + match: RegExp | string; + replacement: ((message: string) => string) | string; +} +const ERROR_REPHRASES: MongoErrorRephrase[] = [ + { + match: 'apiVersion parameter is required', + replacement: 'The apiVersion parameter is required, please run mongosh with the --apiVersion argument and make sure to specify the apiVersion option for Mongo(..) instances.' + } +]; + +export function rephraseMongoError(error: any): any { + if (!isMongoError(error)) { + return error; + } + + const e = error as Error; + const message = e.message; + + const rephrase = ERROR_REPHRASES.find(m => { + return typeof m.match === 'string' ? message.includes(m.match) : m.match.test(message); + }); + + if (rephrase) { + e.message = typeof rephrase.replacement === 'function' ? rephrase.replacement(message) : rephrase.replacement; + } + + return e; +} + +function isMongoError(error: any): boolean { + return /^Mongo([A-Z].*)?Error$/.test(Object.getPrototypeOf(error).constructor.name ?? ''); +} From 139502cf29bd26597e69746ea084bd8491937150 Mon Sep 17 00:00:00 2001 From: Michael Rose Date: Wed, 26 May 2021 15:29:28 +0200 Subject: [PATCH 2/4] rephrase NotPrimaryNoSecondaryOk error --- packages/shell-api/src/mongo-errors.spec.ts | 55 +++++++++++++++++++-- packages/shell-api/src/mongo-errors.ts | 13 +++-- 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/packages/shell-api/src/mongo-errors.spec.ts b/packages/shell-api/src/mongo-errors.spec.ts index a5408ef680..3cb809dc01 100644 --- a/packages/shell-api/src/mongo-errors.spec.ts +++ b/packages/shell-api/src/mongo-errors.spec.ts @@ -1,6 +1,13 @@ import { MongoError } from 'mongodb'; import { expect } from 'chai'; import { rephraseMongoError } from './mongo-errors'; +import Mongo from './mongo'; +import { StubbedInstance, stubInterface } from 'ts-sinon'; +import { bson, ServiceProvider } from '@mongosh/service-provider-core'; +import Database from './database'; +import { EventEmitter } from 'events'; +import ShellInternalState from './shell-internal-state'; +import Collection from './collection'; class MongoshInternalError extends Error { constructor(message: string) { @@ -27,7 +34,7 @@ describe('mongo-errors', () => { context('for non-MongoError errors', () => { [ new Error('an error'), - new MongoshInternalError("The apiVersion parameter is required, please configure your MongoClient's API version") + Object.assign(new MongoshInternalError('Dummy error'), { code: 13435 }) ].forEach(e => { it(`ignores ${e.constructor.name} ${JSON.stringify(e)}`, () => { const origMessage = e.message; @@ -46,12 +53,52 @@ describe('mongo-errors', () => { expect(r.message).to.equal('ignored'); }); - it('rephrases an apiVersion error', () => { - const e = new MongoError("The apiVersion parameter is required, please configure your MongoClient's API version"); + it('rephrases a NotPrimaryNoSecondaryOk error', () => { + const e = new MongoError('not master and slaveOk=false'); + e.code = 13435; const r = rephraseMongoError(e); expect(r).to.equal(e); - expect(r.message).to.contain('mongosh'); + expect(r.code).to.equal(13435); + expect(r.message).to.contain('setReadPref'); }); }); }); + + describe('intercepts shell API calls', () => { + let mongo: Mongo; + let serviceProvider: StubbedInstance; + let database: Database; + let bus: StubbedInstance; + let internalState: ShellInternalState; + let collection: Collection; + + beforeEach(() => { + bus = stubInterface(); + serviceProvider = stubInterface(); + serviceProvider.runCommand.resolves({ ok: 1 }); + serviceProvider.runCommandWithCheck.resolves({ ok: 1 }); + serviceProvider.initialDb = 'test'; + serviceProvider.bsonLibrary = bson; + internalState = new ShellInternalState(serviceProvider, bus); + mongo = new Mongo(internalState, undefined, undefined, undefined, serviceProvider); + database = new Database(mongo, 'db1'); + collection = new Collection(mongo, database, 'coll1'); + }); + + it('on collection.find error', async() => { + const error = new MongoError('not master and slaveOk=false'); + error.code = 13435; + serviceProvider.insertOne.rejects(error); + + try { + await collection.insertOne({ fails: true }); + expect.fail('expected error'); + } catch (e) { + expect(e).to.equal(error); + expect(e.message).to.contain('not primary and secondaryOk=false'); + expect(e.message).to.contain('db.getMongo().setReadPref()'); + expect(e.message).to.contain('readPref'); + } + }); + }); }); diff --git a/packages/shell-api/src/mongo-errors.ts b/packages/shell-api/src/mongo-errors.ts index 023a5c23b9..bd1fd713cd 100644 --- a/packages/shell-api/src/mongo-errors.ts +++ b/packages/shell-api/src/mongo-errors.ts @@ -1,12 +1,14 @@ interface MongoErrorRephrase { - match: RegExp | string; + matchMessage?: RegExp | string; + code?: number; replacement: ((message: string) => string) | string; } const ERROR_REPHRASES: MongoErrorRephrase[] = [ { - match: 'apiVersion parameter is required', - replacement: 'The apiVersion parameter is required, please run mongosh with the --apiVersion argument and make sure to specify the apiVersion option for Mongo(..) instances.' + // NotPrimaryNoSecondaryOk (also used for old terminology) + code: 13435, + replacement: 'not primary and secondaryOk=false - consider using db.getMongo().setReadPref() or readPref in the connection string' } ]; @@ -19,7 +21,10 @@ export function rephraseMongoError(error: any): any { const message = e.message; const rephrase = ERROR_REPHRASES.find(m => { - return typeof m.match === 'string' ? message.includes(m.match) : m.match.test(message); + if (m.matchMessage) { + return typeof m.matchMessage === 'string' ? message.includes(m.matchMessage) : m.matchMessage.test(message); + } + return m.code !== undefined && (e as any).code === m.code; }); if (rephrase) { From 18debeee6dccbade004e6bfdbda14597422ebc27 Mon Sep 17 00:00:00 2001 From: Michael Rose Date: Wed, 26 May 2021 16:59:13 +0200 Subject: [PATCH 3/4] fixup: replace error assertion --- packages/cli-repl/test/e2e-direct.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-repl/test/e2e-direct.spec.ts b/packages/cli-repl/test/e2e-direct.spec.ts index d9dbaec5f8..bc0af4f6ae 100644 --- a/packages/cli-repl/test/e2e-direct.spec.ts +++ b/packages/cli-repl/test/e2e-direct.spec.ts @@ -94,7 +94,7 @@ describe('e2e direct connection', () => { await shell.waitForPrompt(); await shell.executeLine('use admin'); await shell.executeLine('db.runCommand({ listCollections: 1 })'); - shell.assertContainsError('MongoError: not master'); + shell.assertContainsError('MongoError: not primary'); }); it('lists collections when readPreference is in the connection string', async() => { From 4a1f7a5d7d5d15fd0ec91f307c007c1fa0ac8a6f Mon Sep 17 00:00:00 2001 From: Michael Rose Date: Wed, 26 May 2021 17:40:37 +0200 Subject: [PATCH 4/4] fixup --- packages/shell-api/src/mongo-errors.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shell-api/src/mongo-errors.ts b/packages/shell-api/src/mongo-errors.ts index bd1fd713cd..58a71f349e 100644 --- a/packages/shell-api/src/mongo-errors.ts +++ b/packages/shell-api/src/mongo-errors.ts @@ -35,5 +35,5 @@ export function rephraseMongoError(error: any): any { } function isMongoError(error: any): boolean { - return /^Mongo([A-Z].*)?Error$/.test(Object.getPrototypeOf(error).constructor.name ?? ''); + return /^Mongo([A-Z].*)?Error$/.test(Object.getPrototypeOf(error)?.constructor?.name ?? ''); }