new feature: virtual async

[fundon]

New Feature virtual async, plz support!

const User = new Schema(
  {
    username: {
      type: String,
      index: true,
      unique: true
    },
    encryptedPassword: {
      type: String,
      required: true,
      minlength: 64,
      maxlength: 64
    },
    passwordSalt: {
      type: String,
      required: true,
      minlength: 32,
      maxlength: 32
    }
})

User.virtual('password').set(async function generate(v) {
  this.passwordSalt = await encryptor.salt()
  this.encryptedPassword = await encryptor.hash(v, this.passwordSalt)
})

  const admin = new User({
    username: 'admin',
    password: 'admin'
  })
  admin.save()

[fundon]

Current, I use a dirty way:

User.virtual('password').set(function(v) {
  this.encryptedPassword = v
})

User.pre('validate', function preValidate(next) {
  return this.encryptPassword().then(next)
})

User.method('encryptPassword', async function encryptPassword() {
  this.passwordSalt = await encryptor.salt()
  this.encryptedPassword = await encryptor.hash(
    this.encryptedPassword,
    this.passwordSalt
  )
})

[gcanu]

+1

[neyasov]

+1

[vkarpov15]

Some very good ideas in your code, we've seen this issue a few times but haven't quite had time to investigate it. I like this idea though, will consider for an upcoming release

[heisian]

the problem is.. what does the usage syntax look like?

await (user.password = 'some-secure-password');

This doesn't work.

According to ECMA262 12.15.4, the return value of user.password = 'some-secure-password' should be rval, which is in this case 'some-secure-password'.

You are proposing to have the return value of someVar = object be a Promise, and according to this thread, and the above linked ES262 specification, it is a "deep violation of ES semantics."

Additionally, attempting to implement such a semantic-violating issue for the mere purpose of having a convenience function is a pretty bad idea, especially since it could potentially mean all kinds of bad things for the mongoose codebase as a whole.

Why don't you just do:

const hashPassword = require('./lib/hashPassword');

const password = await hashPassword('some-secure-password');
User.password = password; // This is completely normal.

There is literally no need to try to make an async setter, which shouldn't be done in the first place, for such a simple one-liner as this.

[heisian]

You could also just do this:

User.methods.setPassword = async function (password) {
  const hashedPassword = await hashPassword(password);
  this.password = hashedPassword;
  await this.save();
  return this;
};

const myUser = new User();
await myUser.setPassword('mypassword...');

I have no idea why you would go through all the trouble of doing virtuals, pre-save hooks, etc...

[wlingke]

I agree with @heisian. This feels like feature/api bloat to me IMHO. I don't see how the alternative of using an instance method is inconvenient here. But adding a pretty major syntax support for this definitely feels like bloat.

[gcanu]

We should have a very simple feature like this :

User.virtual('password').set((value, done) => {
  encryptValueWithAsyncFunction
    .then(response => done(null, value))
    .catch(reason => done(reason))
  ;
})

[heisian]

@gcanu you're completely ignoring what I posted, what you are proposing returns a Promise from an assignment call and that completely breaks the Javascript/ECMA262 specification. For your code snippet to work, your setter function needs to be a Promise, which by definition is not allowed per specification, and wouldn't work anyways.

What's wrong with just doing:

await User.setPassword('password');

???

In case you didn't see before, this won't work:

await (User.password = 'password');

[heisian]

@vkarpov15 This is not a mongoose-specific issue but rather questioning the validity of the current ECMAScript spec. This "feature request" should be closed...

[fundon]

Below code is very bad idea! Why set password includes save operation?

User.methods.setPassword = async function (password) {
  const hashedPassword = await hashPassword(password);
  this.password = hashedPassword;
  await this.save();
  return this;
};

const myUser = new User();
await myUser.setPassword('mypassword...');

Mongoose needs more modern, more elegant.

[gcanu]

@heisian Ok my mistake, I didn't take care of setter use...

[fundon]

@heisian Plz see https://github.com/Automattic/mongoose/blob/master/lib/virtualtype.js.

Currently, In Mongoose IMPL, getter or setter just register a function then calling, it's not https://tc39.github.io/ecma262/#sec-assignment-operators-runtime-semantics-evaluation and tc39/proposal-async-await#82. That's different.

So plz open this request.

[heisian]

@fundon , Tell me this: How exactly will you call your virtual setter? Please show the usage. If you're using async it must be handled by a promise. Your original example does not show await anywhere in the setter/assignment call.

[heisian]

My example code is just an example... you can also do this so easily:

User.methods.setPassword = async function (password) {
  const hashedPassword = await hashPassword(password);
  this.password = hashedPassword;
  return this;
};

const myUser = new User();
await myUser.setPassword('mypassword...');
await myUser.save();

Obviously..

[fundon]

Your example is not a good way for me.

I want

await new User({ password }).save()

Hash the password in the mode that more simple, more elegant.

[heisian]

why? so you can save a few lines of code? the reason isn't enough to justify all the extra work and possibly breaking changes to the codebase.

[heisian]

You also have to realize at the end of the day no matter how you phrase it, what's going on internally within Mongoose is a setter, which can't be async/await.

[fundon]

I don't agree with @heisian. Mongoose has too many old things. Mongoose needs refactor!
Mongoose needs modern.

If this issue is closed. I will fork Mongoose, refactor it! Bye!

[heisian]

Great! That is the point of open source. Please go ahead and create fork with a trimmed down version, it would be good for us all.

[vkarpov15]

There's really no concern about needing await (User.password = 'password'); . The only real downside is that user.password = 'password'; will then mean that there's some async operation that's happening, so user.passwordSalt will not be set. How that relates to hooks is also an interesting question: what happens if you have a pre('validate') or pre('save') hook, should those wait until the user.password async op is done?

I'm not inclined to dismiss this issue out of hand. There's a lot of value to be had in consolidating async behavior behind .save(), .find(), etc. just need to make sure it fits neatly with the rest of the API.

[gcanu]

Today, async getters and setters are very important to me. I need to send HTTP requests from getters and setters to decrypt/encrypt fields with proprietary encryption methods, and currently, there is no way to do that. Do you have an idea how to achieve that ?

[vkarpov15]

@gcanu I'd just implement those as methods

[heisian]

For my reasons mentioned and the fact that there's methods to easily handle any async operations you need, I don't see any utility behind consolidating async behaviour.. again, await (User.password = 'password') breaks ECMAScript convention and I guarantee it's going to be difficult and not worth it to implement gracefully...

[vkarpov15]

You're right, that pattern isn't something we will implement. The idea of waiting for async virtual to resolve before saving is interesting.

[gabzim]

I would love it for a toJSON({virtuals: true}) implementation. Some of the virtual fields I obtain by running other queries to the db, that I only want to run once you serialize.

[vkarpov15]

@gabzim that would be pretty messy because JSON.stringify does not support promises. So res.json() will never be able to handle async virtuals unless you add extra helpers to express.

[gabzim]

Ah yeah, makes sense, thanks @vkarpov15

[JulianSoto]

Would it be a good practice to make a query inside get callback?
I think this would be useful in some cases.

Let's say I want to get the full path of a web page (or document), where documents can be nested, something like Github URL paths.

const Doc = require('./Doc.js');
//...
subDocSchema.virtual('fullpath').get(async function(){
    const doc = await Doc.findById(this.doc); //doc is a Doc ref of type _id
    return `/${ doc.path }/${ this.path }`
})

Here we have to use async/await as query operations are asynchronous.

[vkarpov15]

@JulianSoto in this case, I recommend you use a method rather than a virtual. The primary reason to use virtuals is because you can make Mongoose include virtuals in toJSON() and toObject() output. But toJSON() and toObject() are synchronous, so they wouldn't handle the async virtual for you.

[claytongulick]

I've run into a use case for this as well, and I'm trying to think through a good solution, very open to ideas, and agree with not breaking setter semantics.

I wrote a utility to automatically apply JSON Patch sets to mongoose models. It supports auto population with deep paths: https://github.com/claytongulick/mongoose-json-patch

The idea, is that in combination with some rules: https://github.com/claytongulick/json-patch-rules you can come pretty close to having an 'automatic' api with JSON Patch.

My plan has been that for cases where simple assignment won't work, to use virtuals. When the patch is applied, a virtual will pick up anything you want - this would allow your interface object to be different than the actual mongoose model / db object.

For example, I have a User object that supports an 'add' operation on 'payment_methods'. Adding a payment method isn't a straight add to an array - it's a call out to the processor with a payment token, getting a payment method token back, storing that in a different way in the model, etc...

But I'd like the interface model, the conceptual model, to be able to be patched with a JSON patch 'add' op.

Without async setters, this won't work. I guess the only option is to have mongoose-json-patch accept as an option some sort of mapping between paths, ops, and mongoose methods, unless there are better ideas?

[vkarpov15]

@claytongulick why do you need an async setter rather than await on an async operation and then synchronously set?

[isamert]

@vkarpov15 What about simply making toObject() and toJSON() async by default and introduce toObjectSync() and toJSONSync() functions? Sync variants should simply skip async virtuals. (I remember this pattern is used in mongoose somewhere, so it wouldn't be too weird to have.)

My use case is something like this: I have a schema that has a virtual which does a find() on an another model (a little bit more complex than simply populating an id). Of course I can denormalize the stuff that I want into my main model using save/delete hooks but that comes with a lot of maintainability costs (and I really don't need the performance benefits in this particular case). So it feels natural to have a virtual to do that for me.

[vkarpov15]

JSON.stringify() doesn't support async toJSON(), so unfortunately the toJSONSync() idea won't work.

I know you said your find() is pretty complex, but you might want to take a look at populate virtuals just in case. You could also try query middleware.

Also, does your async virtual have a setter or only a getter @isamert ?

[silto]

A solution for those having this issue:

In the case where only the setter is asynchronous, I have found a solution. It's a bit dirty but it seems to work fine.
The idea is to pass to the virtual setter an object containing a promise resolver as a callback prop and the virtual property to set. when the setter is done, it calls the callback, which means to the outside that the object can be saved.

To use a basic example inspired from the first question:

const User = new Schema(
  {
    username: {
      type: String,
      index: true,
      unique: true
    },
    encryptedPassword: {
      type: String,
      required: true,
      minlength: 64,
      maxlength: 64
    }
})

User.virtual('password').set(function generate(inputWithCb, virtual, doc) {
  let cb = inputWithCb.cb;
  let password = inputWithCb.password;
  encryptor.hash(password)
  .then((hash) => {
    doc.set("encryptedPassword", hash);
    cb && cb();
  });
})

// create the document
const admin = new User({
  username: 'admin'
});
// setup the promise for setting the async virtuals
const pwdProm = new Promise((resolve) => {
  admin.set("password", {cb: resolve, password: "admin"});
})

//run the promise and save only when the virtual setters have finished executing
pwdProm
.then(() => {
  admin.save();
});

This might have unwanted consequences so use at your own risk.

[vkarpov15]

@silto why don't you just use a schema method that returns a promise?

[silto]

@vkarpov15 I would normally do that, but in the project where I did this I have schemas, virtuals and graphQL endpoints generated automatically from a json "plan", so I prefer having a uniform virtual interface instead of a method for a particular case.

[vkarpov15]

@silto can you provide any code samples? I'd love to see what this looks like

[chumager]

In the case of setter, you may want to save it or just look for the document data, if you save, that's a promise, so you can check for the fields that are promises, resolve them and then save.

If you want to look for the data, you can define by schema options that this Model will be a Promise type or when you create the model check the schema and check if there is a setter, getter or virtual that is a promise and then turn it into a promise.

Or you can simply use an exec like function that you already have (execPopulate).

in resume, if you want to observe the data that has a setter, getter or virtual you can build a function for that matters, if you want to save the data it is already a promise so you can use the same function to transform the data before save it.

I use to use virtuals with promises but as I use express-promise, almost all the time I don't care about the promises but in some cases I use Doc..then(), as I never have used setters with promises I don't have that problem...

Either way it'd be nice to have some kind of wrapper to get all the data already resolved and don't have to use the "then" on every promisified virtual and getter, of after defining a promisified setter.

I you want I can help you with this approach.

Best Regards.

PS: a typical example of use of promisifed virtuals, in my case I use 2 paths to know if my Documents can be deleted or updates according external data, so I usually need to query other models to know if this one can be deleted or modified. As I say before, express-promise resolve this problem for me, but if I want to check internally if those tasks can be done I had to resolve this promises before.

[vkarpov15]

@chumager can you please provide some code samples?

[chumager]

Hi, for example, according to my comment below, I use 2 virtuals _update and _delete, and a plugin who defines those virtuals in case it's not defined in the schema, returning true.

I have a Simulation model to define a credit, and a Project mode to publish the simulation with mkt data.
The simulation can't be deleted in case there is a project associated with the simulation, and can't be updated if the project is published for investments.

The resolution of the virtual _update in the simulation is by finding a project with the simulation referenced and the status is "En Financiamiento", if this query is true then the simulation can;t be updated... obviously the "find" is a promise, so the virtual it's also...

As normally I use this virtual in the frontend, the data is parse by a module who resolves the object (co or express-promise depending is one or an array of result).

In the case I wanted to see the document, I'll find that my virtuals are promises, so I've to use co module to resolve, but I already had to use result as promise... maybe just adding co to the result will do the magic, or with a plugin that use co after find... but it seems more naturally the result set already have done the job.

I use lots of endpoints to get data from mongoose, si I'll have to use that function everywhere or use a post hook for find.

same thing with getters, with setters the hook should be pre validation, but it's important not to touch other props from the document, as it has circular references and other props like the constructor.

Regards...

PS: If you really need example codo please let me know.

[vkarpov15]

@chumager big wall of prose !== code sample. I would really prefer a code sample.