Automattic
new feature: virtual async
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()
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
)
})
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
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.
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...
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.
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))
;
})
@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');
@vkarpov15 This is not a mongoose-specific issue but rather questioning the validity of the current ECMAScript spec. This "feature request" should be closed...
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.
@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 https://github.com/tc39/ecmascript-asyncawait/issues/82. That's different.
So plz open this request.
@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.
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..
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.
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.
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.
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!
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.
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.
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 ?
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...
You're right, that pattern isn't something we will implement. The idea of waiting for async virtual to resolve before saving is interesting.
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.
@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.
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.
@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.