feat(NODE-2014)!: return executor result from withSession and withTransaction #3783
Conversation
src/sessions.ts
Outdated
| } | ||
|
|
||
| return attemptTransactionCommit(session, startTime, fn, options); | ||
| return attemptTransactionCommit(session, startTime, fn, options).then(() => result); |
There was a problem hiding this comment.
What happens if we need to retry the transaction? I think we'd get incorrect behavior, because the first call to attemptTransaction would always return the result of the first call to fn - but I'd assume that we want to return the result associated with the successful attemptTransaction.
There was a problem hiding this comment.
Perhaps this is okay and expected behavior? We document that Client.withSession may call its callback more than once - we should here too at the very least. But I think since we may call the callback twice, it's an acceptable limitation that the callback should be idempotent.
There was a problem hiding this comment.
edit: it is noted for this method
There was a problem hiding this comment.
pending slack convo
There was a problem hiding this comment.
Updated to return the last result and added a test
| @@ -591,18 +589,18 @@ function attemptTransaction<TSchema>( | |||
|
|
|||
| if (!isPromiseLike(promise)) { | |||
| session.abortTransaction().catch(() => null); | |||
| throw new MongoInvalidArgumentError( | |||
| 'Function provided to `withTransaction` must return a Promise' | |||
| return Promise.reject( | |||
There was a problem hiding this comment.
Why'd you revert the throw?
There was a problem hiding this comment.
I guess it ends up being equivalent only because the caller is now an async function, but throwing here is incorrect since it changes the expectation that this function is always promise returning. I don't feel strongly about keeping this, esp with the ticket to refactor this properly coming up.
There was a problem hiding this comment.
I was just curious, this change is fine. It'll be moot once Malik gets to this code anyways
| }) | ||
| .finally(async () => await session.endSession()); | ||
|
|
||
| expect(withTransactionResult).to.equal(counter); |
There was a problem hiding this comment.
| expect(withTransactionResult).to.equal(counter); | |
| expect(withTransactionResult).to.equal(3); |
either we do this, or we somehow assert that the transaction was retried. this test would pass as-is if the failpoint were never triggered
There was a problem hiding this comment.
fixed this and the manual abort test
| @@ -591,18 +589,18 @@ function attemptTransaction<TSchema>( | |||
|
|
|||
| if (!isPromiseLike(promise)) { | |||
| session.abortTransaction().catch(() => null); | |||
| throw new MongoInvalidArgumentError( | |||
| 'Function provided to `withTransaction` must return a Promise' | |||
| return Promise.reject( | |||
There was a problem hiding this comment.
I was just curious, this change is fine. It'll be moot once Malik gets to this code anyways
baileympearson
added
the
Primary Review
baileympearson
added
Team Review
Description
What is changing?
withSession&withTransactionreturn the value returned by the provided functionsIs there new documentation needed for these changes?
Yes, API docs have been updated. Look for docs changes needed on the ticket
What is the motivation for this change?
See highlight.
Release Highlight
withSessionandwithTransactionreturn the value returned by the provided functionThe
await client.withSession(async session => {})now returns the value that the provided function returns. Previously, this function returnedvoidthis is a feature to align with the following breaking change.The
await session.withTransaction(async () => {})method now returns the value that the provided function returns. Previously, this function returned the server command response which is subject to change depending on the server version or type the driver is connected to. The return value got in the way of writing robust, reliable, consistent code no matter the backing database supporting the application.withTransactionbreaking changeImportant
When upgrading to this version of the driver audit the usages of
withTransactionforifstatements or other conditional checks on the return value ofwithTransaction. Previously, the return value was the command response if the transaction was committed, andundefinedif it had been manually aborted. It would only throw if an operation or the author of the function threw an error. Since thus far there has not been the ability to get the result of the function passed towithTransactionwe suspect most existing usages to returnvoid, makingwithTransactionavoidreturning function in this major release. Take care to ensure that the return values of your function match the expectation of the code that follows the completion ofwithTransaction.Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript