Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.92k stars 3.84k forks source link

Bug: Setters on virtuals are not called on findOneAndUpdate #8804

Open TimKochDev opened 4 years ago

TimKochDev commented 4 years ago

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

What is the current behavior? On findOneAndUpdate the setters of normal fields get called as expected, but the setters for virtuals don't.

If the current behavior is a bug, please provide the steps to reproduce. I've built a minimal repo to reproduce the bug: https://github.com/TiKo98/mongoose-bug To start, run npm install and npm start.

Here is the important code:

const mongoose = require('mongoose');
const { Schema } = mongoose;

const testSchema = new Schema({
  field: { type: String, set: v => { console.log('set field'); return v }}
})

testSchema.virtual('virtual').set(v => {
  console.log('set virtual');
  return v;
});

const Test = mongoose.model('test', testSchema);

const worksWell = new Test({
  field: "Hello World",
  virtual: "It's me."
})
worksWell.save(); // -> "set field", "set virtual"

const server = http.createServer( async (req, res) => {
  await Test.findOneAndUpdate({field: "Hello World"}, {field: "Invokes setter", virtual: "Won't invoke setter"}); // -> "set field", "set field"

  res.statusCode = 200;
  res.setHeader('Content-Type', 'text/plain');
  res.end('Hello World');
});

What is the expected behavior? I would expect the setter of the virtual to be called as well.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version. "mongoose": "^5.9.9", "node": "^13.13.0"

More information You see that I didn't connect the example code to any MongoDB. But that doesn't change the behavior of this bug.

A little research revealed that mongoose 4.10 introduced the runSettersOnQuery option. Then issue #5340 stated, that this should be enabled by default and that the option would be removed by mongoose v5. This seems to have worked for normal fields but not for virtuals.

This is my first contribution to open-source projects, so I hope to have provided useful information ;)

vkarpov15 commented 4 years ago

Thanks for the repro script! This issue looks like a duplicate of #5763, which is a feature we've considered adding for a while.

I'm curious: what are you planning on using the virtual for? Mongoose doesn't store virtuals in the database, so putting a virtual path in an update wouldn't do anything unless the virtual setter has side effects.

TimKochDev commented 4 years ago

Hi! I planned to add more languages to my MongoDB. The first thing, that a google search suggested was mongoose-intl (https://www.npmjs.com/package/mongoose-intl).

Under the hood it stores a field of type string as a field of type object { en: <english value>, de: <german value>, ...}. Setting and getting those values is possible via virtuals.

This did work well when saving a new document to a collection. But on findOneAndUpdate it just doesn't do anything. Our system relies heavily on findOneAndUpdate with {upsert: true} so nothing worked as expected. Then I tried to rebuild mongoose-intl myself (it is not that big) and found out, that the problem is mongoose itself because it doesn't call virtual setters on update.


While writing these lines I'm wondering why to use virtuals at all. Shouldn't it be possible to achieve the same functionality with a field of type mixed? Then the field itself would have a setter and a getter... I'll give it a try today!

vkarpov15 commented 4 years ago

That's an interesting point. We'll look into mongoose-intl in the future and see what we can do to better support this use case.

TimKochDev commented 4 years ago

While writing these lines I'm wondering why to use virtuals at all. Shouldn't it be possible to achieve the same functionality with a field of type mixed? Then the field itself would have a setter and a getter... I'll give it a try today!

Sorry for taking so long. Indeed, I was able to build the same functionality without using virtuals. As said before, the goal was to offer a solution for multilangual fields with mongoose. I had found mongoose-intl which didn't work for us because of the bug (?) this issue is about.

So here is my workaround, which appears to suit our needs much better the the mongoose-intl plugin ever could. https://github.com/TiKo98/mongoose-bug/blob/master/IntlPlugin.js The first lines of my plugin are heavily inspired by mongoose-intl but then it works completely different. For a Schema

new Schema({
    multilangualString: { type: {}, intl: true }
})

It registers a pre-hook (in our case findOneAndUpdate because all our insertions use that command with {upsert: true}). This hook checks if the object to be updated already is an intlObject like {en: "english", de: "deutsch"}. If not, it converts for example "english" -> { en: { "english"} }. The plugin also add getters to those fields which simply return the value in the specified language. So the multilanguality is completely hidden.

I've added two more features


To conclude: Setters of virtuals still aren't called on update methods. But for our use case, there are better ways to go.

parasgulati commented 2 years ago

Hey Guys, When can we accept fix for this bug ?

vkarpov15 commented 2 years ago

We'll consider this for 6.6.