Skip to content
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.

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

Model static methods: consistently allow session option? #8075

Closed
bathos opened this issue Aug 17, 2019 · 3 comments
Closed

Model static methods: consistently allow session option? #8075

bathos opened this issue Aug 17, 2019 · 3 comments
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@bathos
Copy link

bathos commented Aug 17, 2019

Do you want to request a feature or report a bug?

Feature.

What is the current behavior?

Many of the static methods of Model do not allow specifying session even though they require it to behave correctly if you are inside a transaction. (The model constructor also doesn’t take an arg for this, but you can get around that by calling $session after construction.) For example:

// within withTransaction callback, assume session is ClientSession instance of transaction

const foo = new Foo;
await foo.save({ session });
console.log(await Foo.exists({ _id: foo._id }, { session })); // false — opts arg ignored

If the current behavior is a bug, please provide the steps to reproduce.

What is the expected behavior?

I believe all static methods that execute queries should accept the { session } option.

This isn’t a bug since it’s working as documented — exists above just doesn’t provide any way to specify the session, so it’s running the query outside the current session — but it kind of makes transactions a minefield. Many commonly used static methods do take an options object that supports session, so it’s easy to be surprised by the fact that others, even though they execute commands that take a session, don’t (seemingly at random). The resulting incorrect behavior can be hard to spot, keeping in mind that e.g., exists(query, opts) doesn’t throw, the object is just ignored, and that you might get the right result just through (bad) luck, leaving the bug hidden in waiting.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

Node: 12.7.0
Mongoose: 5.6.9
Mongo: 4.0.11

@vkarpov15 vkarpov15 added this to the 5.6.11 milestone Aug 21, 2019
@vkarpov15 vkarpov15 added the enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature label Aug 21, 2019
@vkarpov15
Copy link
Collaborator

vkarpov15 commented Aug 21, 2019

Thanks, will add that. We support options for most model functions, but we'll audit and make sure we support a session option for all functions where it makes sense to.

As a workaround, you can do Model.exists(query).session(session)

See Query#session() docs

@bathos
Copy link
Author

bathos commented Aug 22, 2019

Thanks!

As a workaround, you can do Model.exists(query).session(session)

I did discover that this worked for countDocuments() & co, but it did not seem to work for exists. It appeared that (consistent w/ the docs actually) exists does not return a query instance, but rather a generic thenable with no query methods. Is this incorrect?

Edit: I suspect because of

return query.then(doc => !!doc);

@vkarpov15
Copy link
Collaborator

Woops you're right, that is a mistake on my part. exists() really should return a query...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

2 participants