Automattic / mongoose

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

Support for Asynchronous Virtual #1894

Closed darthtrevino closed 10 years ago

darthtrevino commented 10 years ago

I'd like to express some virtual properties that perform asynchronous work, such as inspecting other Mongoose models. It would be nice if the virtual getters and setters accepted an optional done function that would be invoked after the async work completed.

aheckmann commented 10 years ago

Use a custom method instead.

lexigren commented 9 years ago

But what to do, if I want to serialize with virtual field, inspecting other document?

vkarpov15 commented 9 years ago

Not possible, because virtuals are just getters/setters, which are assumed to be synchronous. You could work around by making a virtual return a promise.

sibelius commented 8 years ago

@vkarpov15 do you have an example of a virtual returning a promise?

vkarpov15 commented 8 years ago

Promises are objects like any other. Not terribly crazy to have a virtual return a promise or observable.

schema.virtual('myAsyncVirtual').get(() => new Promise((resolve, reject) => {
  setTimeout(() => resolve(1), 1000);
}));
halaprop commented 8 years ago

@vkarpov15 have you tried your suggestion? Consider the case of an api that wants to add virtual properties...

myObjectSchema.virtual('property').get(function() {
    return // an unresolved promise
});

// in a controller
MyObject.findOne({ _id:id }, function(err, instance) {
    res.json(instance);
    console.log(instance.property);  //  not useful --> {"emitter": ....
});

For the asynch virtual to be useful, the find would need to resolve only after the virtuals have. Please prove me wrong about this, because I really need a promise-returning virtual to work.

sibelius commented 8 years ago

@halabe you can use this plugin https://github.com/whitecolor/mongoose-fill for now

vkarpov15 commented 8 years ago

instance.property.then(function(res) { console.log(res); }) works. Or you can use co: console.log(yield instance.property);. In this case, a custom method would probably be better though.

Also, mongoose-fill is a really sweet plugin, you should check it out :+1:

trainerbill commented 8 years ago

Can we get some clarity on what the suggested usage should be and if this will be a feature in the future? I would rather not use mongoose-fill. How would a custom method solve this problem? Are you suggesting a post find hook that executes a custom method? My use case is that I am using node-acl that stores roles. I want my user schema to return the roles every time. I was going to use a virtual getter to return acl.userRoles, which returns a promise that resolves the roles, and am running into this issue.

vkarpov15 commented 8 years ago

@trainerbill which issue is this exactly? Sounds like you got it figured out.

p3x-robot commented 7 years ago

i think, since it is NodeJs, async/await is everywhere, are you guys to add it in? It is nothing.

vkarpov15 commented 7 years ago

Exactly, there's nothing stopping you from having a virtual that returns a promise so you can await on it, no way for mongoose to make that any easier

p3x-robot commented 7 years ago

the problem was with the virtual, when i set , it is not wating until is done, not working with mongoose. So I guess virtual set async function not working (it should wait), I guess i solved by a method. A virtual set, shouldn't be a promsie right? Method is the correct async solution?

vkarpov15 commented 7 years ago

That's not how async/await works, async functions return promises and await keywords must be within async functions. Best mongoose can do is return a promise from a .set() call, a.b = c will never work right if b has an async setter.

joseym commented 6 years ago

Here's an example of how I've used async/await in my virtual definitions, and how it dumps the promise on toObject.

Hopefully this can help someone ...

const mySchema = new mongoose.Schema(...);

// getter is an async function
mySchema.virtual('myProperty').get(async function() {
    return setTimeout(function(){ return 'Had to Wait, didn\'t you?!' }, 1000);
});

// overwrite default `toObject` method
// toObject must now always be called with `await` with this patch
mySchema.methods.toObject = async function(options){
  const obj = this.$toObject(options);
  const virtuals = mySchema.virtuals;
  for(let prop in virtuals) {
    const virtual = virtuals[prop];
    if(virtual.getters[0].constructor.name === 'AsyncFunction') {
      obj[prop] = await this[prop];
    }
  }
};

// Still need to use async/await
console.log(doc.myProperty) // -> Promise { <pending> }
console.log(await doc.myProperty) // -> 'Had to Wait, didn't you?!'

// using toObject()
console.log(doc.toObject()) // -> Promise { <pending> }
console.log(await doc.toObject()) // -> { myProperty: 'Had to Wait, didn't you?!' }
vkarpov15 commented 6 years ago

@joseym have you seen any libs out there that convert objects that contain promises into a promise, sorta like Promise.all() but for deeply nested objects? Been thinking of writing one

p3x-robot commented 6 years ago

@joseym why don't you just use a method that can be async? You can use a method instead of a property. That's how I did it. At least it works what I needed...

joseym commented 6 years ago

@vkarpov15 I haven't, but I've also not spent any time looking. Sounds super useful though. Would it just "pull" all the promises up, resolve them and reposition them to their original location?

joseym commented 6 years ago

@p3x-robot primarily personal preference. I have plenty of instance methods in my model, but - at least for me - sometimes I want to be a bit closer to being forced to resolve something. With an asynchronous virtual its more obvious that I'm missing something (Promise { <pending> } !== { ... }). If I get in a hurry and forget to add await vs forgetting to execute my query and then taking the additional step to call doc.myMethod() its easier to debug (again, for me).

This is more of a problem in larger projects when way down the road when you might forget that something is only retrievable by an additional call when mocking out some endpoints.

halaprop commented 6 years ago

@p3x-robot, the need for me arose in an api project, where I'd like the controllers to be straight-forward and contain little knowledge of the model. In the ideal, I want my controller to form a query and return the result, but instead it must form the query, decide whether this particular query returns virtual properties that depend on promises, resolve those, and then return.

p3x-robot commented 6 years ago

Yeah, I know the issue, would be good to have an async virtual property, for me right now it is a method. I wait for the resolution, too much other issues ....

modemmute commented 6 years ago

@joseym Thanks for your code example. I'm having some problems making that work. I get an error: 'name is not defined'. Is it possible that the line: obj[name] = await this[name]; should instead be... obj[prop] = await this[prop]; Many thanks again.

joseym commented 6 years ago

@modemmute my bad! Yep, it should have been prop rather than name. I've corrected it in my comment as well.

madhums commented 6 years ago

Should this issue be re-opened? It seems like a valid use case for using async logic in virtuals.

I am trying something like this, but the async part doesn't work within .virtual().set(fn)

// models/user.js
UserSchema.virtual('password').set(function(value) {
  // is it a possibility to pass `next` as an argument to the above function? 
  // would that solve this particular async issue?
  getHashAndSalt(value).then(({ hash, salt }) => {
    // unfortunately this part is called only after the `yield user.save()` is called.
    this.hashed_password = hash;
    this.salt = salt;
  });
});

// models/user.test.js
co.wrap(function*() {
  const User = mongoose.model('User');
  const user = new User({
    name: 'John doe',
    email: 'john2@example.com',
    password: 'xyz'
  });
  yield user.save();
});
p3x-robot commented 6 years ago

i can use a method and that's all. you just try to force something that mongoose creators dont want it.... i used a method and it works ....

madhums commented 6 years ago

umm... also found this thread https://github.com/Automattic/mongoose/issues/517#issuecomment-2077096. Only thing being , the api would not be used consistently. Esp for using bcrypt hash and salt generation (as they are blocking and cpu intensive - sync methods are not recommended).

p3x-robot commented 6 years ago

methods i only use async, who said sync?

Patrik

On Nov 29, 2017 15:37, "Madhusudhan Srinivasa" notifications@github.com wrote:

umm... also found this thread #517 (comment) https://github.com/Automattic/mongoose/issues/517#issuecomment-2077096. Only thing being , the api would not be used consistently. Esp for using bcrypt hash and salt generation (as they are blocking and cpu intensive - sync methods are not recommended).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Automattic/mongoose/issues/1894#issuecomment-347879144, or mute the thread https://github.com/notifications/unsubscribe-auth/AbQ4XE81vSHQ4nSVQSHIXKJ60FKC2bg3ks5s7WxBgaJpZM4BcWiT .

BenjaminVanRyseghem commented 4 years ago

I know it's an old issue, but I bumped into this issue today.

I ended up tweaking @joseym 's solution, by overriding toJSON in schema as following:

schema.methods.toJSON = async function(options) {
    const obj = this.$toObject(options, true);
    const virtuals = schema.virtuals;
    for (let prop in virtuals) {
        const virtual = virtuals[prop];
        if (virtual.getters[0].constructor.name === "AsyncFunction") {
            obj[prop] = await this[prop];
        }
    }
    return obj;
};

and in index.js, I tweaked express json method to handle async toJSON:

express.response.json = (function(superJson) {
    return function(object) {
        if (object.toJSON && object.toJSON.constructor.name === "AsyncFunction") {
            return object.toJSON()
                .then((data) => superJson.call(this, data));
        }
        return superJson.call(this, object);
    };
}(express.response.json));

edit: to avoid copy/pasting the schema.methods.toJSON part in every schema, it can be moved in lib/facade.js: Facade>constructor as :

this.Model.prototype.toJSON = async function(options) {
    const obj = this.$toObject(options, true);
    const virtuals = schema.virtuals;
    for (let prop in virtuals) {
        const virtual = virtuals[prop];
        if (virtual.getters[0].constructor.name === "AsyncFunction") {
            obj[prop] = await this[prop];
        }
    }
    return obj;
};
vkarpov15 commented 4 years ago

That solution should work, but you'll have to modify every other lib you choose to use that relies on JSON.stringify() and looks for toJSON(). It's great that it works for Express, but just be aware that you'll have to make a similar change if you choose to use an http client like axios.