Skip to content

feat(NODE-2014)!: return the result of provided callback in withTransaction and withSession #3524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

feat(NODE-2014)!: return the result of provided callback in withTransaction and withSession #3524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
029a610
feat(NODE-2014): return the result of provided callback
durran Jan 16, 2023
79c8caf
chore: update migration guide
durran Jan 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions etc/notes/CHANGES_5.0.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,3 +191,17 @@ await collection.insertMany([{ name: 'fido' }, { name: 'luna' }])

The `keepGoing` option was a legacy name for setting `ordered` to `false` for bulk inserts.
It was only supported by the legacy `collection.insert()` method which is now removed as noted above.

### `withSession` and `withTransaction` now return the result of the provided callback.

These two methods previously returned `Promise<void>` but now users can control what the return value
in the promise is:

```ts
const value = await client.withSession(async (session) => {
return session.withTransaction(async () => {
await collection.insertOne({ a: 1 });
return true;
});
}); // value is the boolean true;
```
18 changes: 10 additions & 8 deletions src/mongo_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ export interface MongoClientOptions extends BSONSerializeOptions, SupportedNodeC
}

/** @public */
export type WithSessionCallback = (session: ClientSession) => Promise<any>;
export type WithSessionCallback<T = any> = (session: ClientSession) => Promise<T>;

/** @internal */
export interface MongoClientPrivate {
Expand Down Expand Up @@ -643,12 +643,12 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
* @param options - Optional settings for the command
* @param callback - An callback to execute with an implicitly created session
*/
withSession(callback: WithSessionCallback): Promise<void>;
withSession(options: ClientSessionOptions, callback: WithSessionCallback): Promise<void>;
withSession(
optionsOrOperation?: ClientSessionOptions | WithSessionCallback,
callback?: WithSessionCallback
): Promise<void> {
withSession<T>(callback: WithSessionCallback<T>): Promise<T>;
withSession<T>(options: ClientSessionOptions, callback: WithSessionCallback<T>): Promise<T>;
withSession<T>(
optionsOrOperation?: ClientSessionOptions | WithSessionCallback<T>,
callback?: WithSessionCallback<T>
): Promise<T> {
const options = {
// Always define an owner
owner: Symbol(),
Expand All @@ -666,15 +666,17 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
const session = this.startSession(options);

return maybeCallback(async () => {
let value;
try {
await withSessionCallback(session);
value = await withSessionCallback(session);
} finally {
try {
await session.endSession();
} catch {
// We are not concerned with errors from endSession()
}
}
return value;
}, null);
}

Expand Down
15 changes: 7 additions & 8 deletions src/sessions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export interface ClientSessionOptions {
}

/** @public */
export type WithTransactionCallback<T = void> = (session: ClientSession) => Promise<T>;
export type WithTransactionCallback<T = any> = (session: ClientSession) => Promise<T>;

/** @public */
export type ClientSessionEvents = {
Expand Down Expand Up @@ -470,10 +470,7 @@ export class ClientSession extends TypedEventEmitter<ClientSessionEvents> {
* @param options - optional settings for the transaction
* @returns A raw command response or undefined
*/
withTransaction<T = void>(
fn: WithTransactionCallback<T>,
options?: TransactionOptions
): Promise<Document | undefined> {
withTransaction<T>(fn: WithTransactionCallback<T>, options?: TransactionOptions): Promise<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs need a little update too:

   * @remarks
   * This function:
   * - Will return the result of the withTransaction callback if every operation is successful
   * - Will return `undefined` if the transaction is intentionally aborted with `await session.abortTransaction()`
   * - Will throw if one of the operations throws or `throw` statement is used inside the `withTransaction` callback

Which makes me question, should we require that the callback return something truthy (via TS) or default to something truthy (maybe true) if the callback returns undefined? Otherwise, how can someone differentiate an intentionally aborted transaction and a committed one if they don't have a return value?

Spec ref

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning true sounds good if they haven't returned anything.

Copy link
Contributor

@nbbeeken nbbeeken Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, I'm thinking we can just add value ?? true (this would also cover return null, but I think that's ok?) and a test for that case along with docs. I'll update the ticket AC just so we keep track

const startTime = now();
return attemptTransaction(this, startTime, fn, options);
}
Expand Down Expand Up @@ -615,12 +612,14 @@ function attemptTransaction<TSchema>(
}

return promise.then(
() => {
value => {
if (userExplicitlyEndedTransaction(session)) {
return;
return value;
}

return attemptTransactionCommit(session, startTime, fn, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it still be possible to get the legacy return value somehow (i.e. the Document returned by session.commitTransaction())?

I think it would be an acceptable breaking change in mongosh if that goes away (right now our own withTransaction helper returns it), but it might be nice to avoid this.

To avoid this, you could do some hackery to attach this secondary result to the returned Promise itself or return a tuple, e.g.

  • withTransaction<T>(fn: WithTransactionCallback<T>, options?: …): Promise<T> & { transactionResult: Promise<Document | undefined> }
  • withTransaction<T>(fn: WithTransactionCallback<T>, options?: …): Promise<[callbackResult: T, transactionResult: Document|undefined]>

(that would also maybe solve the problem mentioned above about distinguishing the callback returning undefined from an aborted transaction?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only worry here is with this change withTransaction and withSession now diverge, and one of the goals of the ticket was to keep those two APIs the same for consistency. But I'm flexible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could just be a separate function or item in options then that makes it return a tuple?

return attemptTransactionCommit(session, startTime, fn, options).then(() => {
return value;
});
},
err => {
function maybeRetryOrThrow(err: MongoError): Promise<any> {
Expand Down
6 changes: 3 additions & 3 deletions test/integration/transactions/transactions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,19 +90,19 @@ describe('Transactions', function () {
expect(withTransactionResult).to.be.undefined;
});

it('should return raw command when transaction is successfully committed', async () => {
it('should return callback return value when transaction is successfully committed', async () => {
const session = client.startSession();

const withTransactionResult = await session
.withTransaction(async session => {
await collection.insertOne({ a: 1 }, { session });
await collection.findOne({ a: 1 }, { session });
return await collection.findOne({ a: 1 }, { session });
})
.finally(async () => await session.endSession());

expect(withTransactionResult).to.exist;
expect(withTransactionResult).to.be.an('object');
expect(withTransactionResult).to.have.property('ok', 1);
expect(withTransactionResult).to.have.property('a', 1);
});

it('should throw when transaction is aborted due to an error', async () => {
Expand Down
22 changes: 22 additions & 0 deletions test/unit/mongo_client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,3 +873,25 @@ describe('MongoOptions', function () {
});
});
});

describe('MongoClient', function () {
describe('#withSession', function () {
const client = new MongoClient('mongodb://localhost:27017');

context('when the callback returns a value', function () {
it('returns the value in the promise', async function () {
const value = await client.withSession(async () => {
return 'test';
});
expect(value).to.equal('test');
});
});

context('when the callback does not return a value', function () {
it('does not return a value in the promise', async function () {
const value = await client.withSession(async () => {});
expect(value).to.equal(undefined);
});
});
});
});
18 changes: 18 additions & 0 deletions test/unit/sessions.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,24 @@ describe('Sessions - unit', function () {
});
});

describe('#withTransaction', function () {
context('when the callback returns a value', function () {
it('returns the value in the promise', async function () {
const value = await session.withTransaction(async () => {
return 'test';
});
expect(value).to.equal('test');
});
});

context('when the callback does not return a value', function () {
it('does not return a value in the promise', async function () {
const value = await session.withTransaction(async () => {});
expect(value).to.equal(undefined);
});
});
});

describe('advanceClusterTime()', () => {
it('should throw an error if the input cluster time is not an object', function () {
const invalidInputs = [undefined, null, 3, 'a'];
Expand Down