From 4e5dcb917c5f3fbbe95e24c1a41e2e3a962c967d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 30 Mar 2021 19:02:12 +0200 Subject: [PATCH 1/3] chore(shell-api,java-shell): returnsPromise implies async MONGOSH-655 - Make sure that all methods marked with `@returnsPromise` actually return promises - Update the java-shell code to account for that - Remove special handling in the Promise-marking code that was previously necessary as a workaround to get the new async-rewriter going --- .../kotlin/com/mongodb/mongosh/result/Cursor.kt | 8 ++++---- .../aggregateWithNegativeBatchSize.expected.txt | 2 +- .../test/resources/cursor/tailable.expected.txt | 2 +- packages/shell-api/src/aggregation-cursor.ts | 10 +++++----- packages/shell-api/src/change-stream-cursor.ts | 3 +-- packages/shell-api/src/collection.ts | 3 +-- packages/shell-api/src/cursor.ts | 14 +++++++------- packages/shell-api/src/decorators.ts | 10 +++++----- packages/shell-api/src/field-level-encryption.ts | 10 +++++++--- packages/shell-api/src/shell-api.spec.ts | 7 +++++++ 10 files changed, 39 insertions(+), 30 deletions(-) diff --git a/packages/java-shell/src/main/kotlin/com/mongodb/mongosh/result/Cursor.kt b/packages/java-shell/src/main/kotlin/com/mongodb/mongosh/result/Cursor.kt index 9d78032c8c..143a945799 100644 --- a/packages/java-shell/src/main/kotlin/com/mongodb/mongosh/result/Cursor.kt +++ b/packages/java-shell/src/main/kotlin/com/mongodb/mongosh/result/Cursor.kt @@ -19,19 +19,19 @@ open class Cursor internal constructor(protected var cursor: Value?, priv } override fun hasNext(): Boolean { - val (cursor, _) = checkClosed() - return cursor.invokeMember("hasNext").asBoolean() + val (cursor, converter) = checkClosed() + return converter.unwrapPromise(cursor.invokeMember("hasNext")).asBoolean() } override fun next(): T { val (cursor, converter) = checkClosed() if (!hasNext()) throw NoSuchElementException() - return converter.toJava(cursor.invokeMember("next")).value as T + return converter.toJava(converter.unwrapPromise(cursor.invokeMember("next"))).value as T } fun tryNext(): T { val (cursor, converter) = checkClosed() - return converter.toJava(cursor.invokeMember("tryNext")).value as T + return converter.toJava(converter.unwrapPromise(cursor.invokeMember("tryNext"))).value as T } fun close() { diff --git a/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.txt b/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.txt index e6f991917d..ab7fc94649 100644 --- a/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.txt +++ b/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.txt @@ -1 +1 @@ -org.graalvm.polyglot.PolyglotException: Command failed with error 2 (BadValue): 'cursor.batchSize must not be negative' on server %mongohostport%. The full response is {"ok": 0.0, "errmsg": "cursor.batchSize must not be negative", "code": 2, "codeName": "BadValue"} +com.mongodb.MongoCommandException: Command failed with error 2 (BadValue): 'cursor.batchSize must not be negative' on server %mongohostport%. The full response is {"ok": 0.0, "errmsg": "cursor.batchSize must not be negative", "code": 2, "codeName": "BadValue"} diff --git a/packages/java-shell/src/test/resources/cursor/tailable.expected.txt b/packages/java-shell/src/test/resources/cursor/tailable.expected.txt index 2b514d9c75..2876431e2f 100644 --- a/packages/java-shell/src/test/resources/cursor/tailable.expected.txt +++ b/packages/java-shell/src/test/resources/cursor/tailable.expected.txt @@ -1 +1 @@ -org.graalvm.polyglot.PolyglotException: Query failed with error code 2 and error message 'error processing query: ns=admin.collTree: $and \ No newline at end of file +com.mongodb.MongoQueryException: Query failed with error code 2 and error message 'error processing query: ns=admin.collTree: $and diff --git a/packages/shell-api/src/aggregation-cursor.ts b/packages/shell-api/src/aggregation-cursor.ts index b1d359e750..e3e707d66d 100644 --- a/packages/shell-api/src/aggregation-cursor.ts +++ b/packages/shell-api/src/aggregation-cursor.ts @@ -50,17 +50,17 @@ export default class AggregationCursor extends ShellApiClass { } @returnsPromise - forEach(f: (doc: Document) => void): Promise { + async forEach(f: (doc: Document) => void): Promise { return this._cursor.forEach(f); } @returnsPromise - hasNext(): Promise { + async hasNext(): Promise { return this._cursor.hasNext(); } @returnsPromise - tryNext(): Promise { + async tryNext(): Promise { return this._cursor.tryNext(); } @@ -99,12 +99,12 @@ export default class AggregationCursor extends ShellApiClass { } @returnsPromise - next(): Promise { + async next(): Promise { return this._cursor.next(); } @returnsPromise - toArray(): Promise { + async toArray(): Promise { return this._cursor.toArray(); } diff --git a/packages/shell-api/src/change-stream-cursor.ts b/packages/shell-api/src/change-stream-cursor.ts index 58183aaaed..79a824ece2 100644 --- a/packages/shell-api/src/change-stream-cursor.ts +++ b/packages/shell-api/src/change-stream-cursor.ts @@ -88,8 +88,7 @@ export default class ChangeStreamCursor extends ShellApiClass { return this._cursor.closed; } - @returnsPromise - isExhausted(): Promise { + isExhausted(): never { throw new MongoshInvalidInputError('isExhausted is not implemented for ChangeStreams because after closing a cursor, the remaining documents in the batch are no longer accessible. If you want to see if the cursor is closed use isClosed. If you want to see if there are documents left in the batch, use tryNext.'); } diff --git a/packages/shell-api/src/collection.ts b/packages/shell-api/src/collection.ts index 1454a476b8..bd8f95fd9a 100644 --- a/packages/shell-api/src/collection.ts +++ b/packages/shell-api/src/collection.ts @@ -753,9 +753,8 @@ export default class Collection extends ShellApiClass { ); } - @returnsPromise @deprecated - save(): Promise { + save(): never { throw new MongoshInvalidInputError( 'Collection.save() is deprecated. Use insertOne, insertMany, updateOne, or updateMany.' ); diff --git a/packages/shell-api/src/cursor.ts b/packages/shell-api/src/cursor.ts index ae541cc54d..6500dffb2e 100644 --- a/packages/shell-api/src/cursor.ts +++ b/packages/shell-api/src/cursor.ts @@ -128,7 +128,7 @@ export default class Cursor extends ShellApiClass { @serverVersions([ServerVersions.earliest, '4.0.0']) @returnsPromise - count(): Promise { + async count(): Promise { return this._cursor.count(); } @@ -164,12 +164,12 @@ export default class Cursor extends ShellApiClass { } @returnsPromise - forEach(f: (doc: Document) => void): Promise { + async forEach(f: (doc: Document) => void): Promise { return this._cursor.forEach(f); } @returnsPromise - hasNext(): Promise { + async hasNext(): Promise { if (this._tailable) { printWarning( 'If this is a tailable cursor with awaitData, and there are no documents in the batch, this method ' + @@ -181,7 +181,7 @@ export default class Cursor extends ShellApiClass { } @returnsPromise - tryNext(): Promise { + async tryNext(): Promise { return this._cursor.tryNext(); } @@ -253,7 +253,7 @@ export default class Cursor extends ShellApiClass { } @returnsPromise - next(): Promise { + async next(): Promise { if (this._tailable) { printWarning( 'If this is a tailable cursor with awaitData, and there are no documents in the batch, this' + @@ -308,7 +308,7 @@ export default class Cursor extends ShellApiClass { } @returnsPromise - size(): Promise { + async size(): Promise { return this._cursor.count(); } @@ -336,7 +336,7 @@ export default class Cursor extends ShellApiClass { } @returnsPromise - toArray(): Promise { + async toArray(): Promise { return this._cursor.toArray(); } diff --git a/packages/shell-api/src/decorators.ts b/packages/shell-api/src/decorators.ts index cf203d1184..cc19823893 100644 --- a/packages/shell-api/src/decorators.ts +++ b/packages/shell-api/src/decorators.ts @@ -295,11 +295,7 @@ export function shellApiClassDefault(constructor: Function): void { function markImplicitlyAwaited Promise>(orig: T): ((...args: Parameters) => Promise) { function wrapper(this: any, ...args: any[]) { const origResult = orig.call(this, ...args); - return Object.prototype.toString.call(origResult) === '[object Promise]' ? addHiddenDataProperty( - origResult, - Symbol.for('@@mongosh.syntheticPromise'), - true - ) : origResult; + return addHiddenDataProperty(origResult, Symbol.for('@@mongosh.syntheticPromise'), true); } Object.setPrototypeOf(wrapper, Object.getPrototypeOf(orig)); Object.defineProperties(wrapper, Object.getOwnPropertyDescriptors(orig)); @@ -328,10 +324,14 @@ export function topologies(topologiesArray: Topologies[]): Function { descriptor.value.topologies = topologiesArray; }; } +export const nonAsyncFunctionsReturningPromises: string[] = []; // For testing. export function returnsPromise(_target: any, _propertyKey: string, descriptor: PropertyDescriptor): void { const orig = descriptor.value; orig.returnsPromise = true; descriptor.value = markImplicitlyAwaited(descriptor.value); + if (orig.constructor.name !== 'AsyncFunction') { + nonAsyncFunctionsReturningPromises.push(orig.name); + } } export function directShellCommand(_target: any, _propertyKey: string, descriptor: PropertyDescriptor): void { descriptor.value.isDirectShellCommand = true; diff --git a/packages/shell-api/src/field-level-encryption.ts b/packages/shell-api/src/field-level-encryption.ts index aab26b98d3..1ee6aee94b 100644 --- a/packages/shell-api/src/field-level-encryption.ts +++ b/packages/shell-api/src/field-level-encryption.ts @@ -2,6 +2,7 @@ import { classPlatforms, hasAsyncChild, returnsPromise, + returnType, ShellApiClass, shellApiClassDefault } from './decorators'; @@ -122,7 +123,7 @@ export class KeyVault extends ShellApiClass { createKey(kms: ClientEncryptionDataKeyProvider, options: AWSEncryptionKeyOptions | AzureEncryptionKeyOptions | GCPEncryptionKeyOptions | undefined): Promise createKey(kms: ClientEncryptionDataKeyProvider, options: AWSEncryptionKeyOptions | AzureEncryptionKeyOptions | GCPEncryptionKeyOptions | undefined, keyAltNames: string[]): Promise @returnsPromise - createKey( + async createKey( kms: ClientEncryptionDataKeyProvider, masterKeyOrAltNames?: AWSEncryptionKeyOptions | AzureEncryptionKeyOptions | GCPEncryptionKeyOptions | string | undefined | string[], keyAltNames?: string[] @@ -177,28 +178,31 @@ export class KeyVault extends ShellApiClass { return this._clientEncryption._libmongocrypt.createDataKey(kms, options as ClientEncryptionCreateDataKeyProviderOptions); } + @returnType('Cursor') getKey(keyId: BinaryType): Cursor { assertArgsDefinedType([keyId], [true], 'KeyVault.getKey'); return this._keyColl.find({ '_id': keyId }); } + @returnType('Cursor') getKeyByAltName(keyAltName: string): Cursor { assertArgsDefinedType([keyAltName], ['string'], 'KeyVault.getKeyByAltName'); return this._keyColl.find({ 'keyAltNames': keyAltName }); } + @returnType('Cursor') getKeys(): Cursor { return this._keyColl.find({}); } @returnsPromise - deleteKey(keyId: BinaryType): Promise { + async deleteKey(keyId: BinaryType): Promise { assertArgsDefinedType([keyId], [true], 'KeyVault.deleteKey'); return this._keyColl.deleteOne({ '_id': keyId }); } @returnsPromise - addKeyAlternateName(keyId: BinaryType, keyAltName: string): Promise { + async addKeyAlternateName(keyId: BinaryType, keyAltName: string): Promise { assertArgsDefinedType([keyId, keyAltName], [true, 'string'], 'KeyVault.addKeyAlternateName'); return this._keyColl.findAndModify({ query: { '_id': keyId }, diff --git a/packages/shell-api/src/shell-api.spec.ts b/packages/shell-api/src/shell-api.spec.ts index 0b54ae93a5..ba3871f096 100644 --- a/packages/shell-api/src/shell-api.spec.ts +++ b/packages/shell-api/src/shell-api.spec.ts @@ -3,6 +3,7 @@ import { expect } from 'chai'; import ShellApi from './shell-api'; import { signatures, toShellResult } from './index'; import Cursor from './cursor'; +import { nonAsyncFunctionsReturningPromises } from './decorators'; import { ALL_PLATFORMS, ALL_SERVER_VERSIONS, ALL_TOPOLOGIES } from './enums'; import sinon, { StubbedInstance, stubInterface } from 'ts-sinon'; import Mongo from './mongo'; @@ -625,3 +626,9 @@ describe('ShellApi', () => { } }); }); + +describe('returnsPromise marks async functions', () => { + it('no non-async functions are marked returnsPromise', () => { + expect(nonAsyncFunctionsReturningPromises).to.deep.equal([]); + }); +}); From 2a08ee093bacd61752c3b2f481f4314e5450629c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 31 Mar 2021 10:59:33 +0200 Subject: [PATCH 2/3] fixup! chore(shell-api,java-shell): returnsPromise implies async MONGOSH-655 --- .../collection/aggregateWithNegativeBatchSize.expected.1.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.1.txt b/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.1.txt index 4dbc964e20..707f50a0a5 100644 --- a/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.1.txt +++ b/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.1.txt @@ -1 +1 @@ -org.graalvm.polyglot.PolyglotException: Command failed with error 51024 (Location51024): 'BSON field 'batchSize' value must be >= 0, actual value '-1'' on server %mongohostport%. The full response is {"ok": 0.0, "errmsg": "BSON field 'batchSize' value must be >= 0, actual value '-1'", "code": 51024, "codeName": "Location51024"} +com.mongodb.MongoCommandException: Command failed with error 51024 (Location51024): 'BSON field 'batchSize' value must be >= 0, actual value '-1'' on server %mongohostport%. The full response is {"ok": 0.0, "errmsg": "BSON field 'batchSize]' value must be >= 0, actual value '-1'", "code": 51024, "codeName": "Location51024"} From 784faf9982bd80e29e175ecf633503ce76a2bfb1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 31 Mar 2021 11:38:26 +0200 Subject: [PATCH 3/3] fixup! fixup! chore(shell-api,java-shell): returnsPromise implies async MONGOSH-655 --- .../collection/aggregateWithNegativeBatchSize.expected.3.txt | 1 + 1 file changed, 1 insertion(+) create mode 100644 packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.3.txt diff --git a/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.3.txt b/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.3.txt new file mode 100644 index 0000000000..8903b2c57f --- /dev/null +++ b/packages/java-shell/src/test/resources/collection/aggregateWithNegativeBatchSize.expected.3.txt @@ -0,0 +1 @@ +com.mongodb.MongoCommandException: Command failed with error 51024 (Location51024): 'BSON field 'batchSize' value must be >= 0, actual value '-1'' on server %mongohostport%. The full response is {"ok": 0.0, "errmsg": "BSON field 'batchSize' value must be >= 0, actual value '-1'", "code": 51024, "codeName": "Location51024"}