wclr / mongoose-fill

Virtual async fileds for mongoose.js
61 stars 8 forks source link

Support fill propagation to field models #2

Closed sibelius closed 8 years ago

sibelius commented 8 years ago

Use case: a Post that has Comment that has a Like

I want to use mongoose-fill like this:

post.find().fill('comment.like', user)

Right now, I need to define this propagation step on my schemas

Post.fill('comment.like', function(user, cb) {
  this.comment.fill('like', user, cb);
})

Comment.fill('like', function(user, cb) {
  Like.find({owner: this._id, user: user}).exec(function (err, like) {
    cb(err, like.length > 0);
  });
})
wclr commented 8 years ago

And what should be done with:

post.find().fill('comment.like.anotherFillPropertyOnLikeSchema', user) 

? Imo such "propagation" is too implicit and error prone.

sibelius commented 8 years ago

it is the same approach of the deepPopulate plugin, the same syntax, we just need to throw an error when a property doesn't exist

wclr commented 8 years ago

And what would you do with multiple documents?

post.find().fill('comments.like', user)

It is not difficult to add. But I still think that it is really unnesessary thing it will not change you app's logic code and will get rid of some few lines of config code, but they make app more clear and explicit. What you are aguments for this?

sibelius commented 8 years ago

you probably gonna need a map fill for multiple documents.

it is just more sugar syntax, just let this issue open and see whether more people need this as well

wclr commented 8 years ago

@sibeliusseraphini try to have fun with 1.1.0

sibelius commented 8 years ago

that's great, thanks

sibelius commented 8 years ago

there is only one missing case that you forgot to handle, when the comment is null.

if comment is null, for instance here: post.find().fill('comment.like', user)

this will great the post.find()

wclr commented 8 years ago

do you mean it has to fill comment first or what?

sibelius commented 8 years ago

this problem happens when the comment of product is null

product.comment is null, so it should not call the fill for comment of this product

the current code is call product.comment.fill(...) even if comment is null

wclr commented 8 years ago

Does it throw?

rturk commented 8 years ago

At least for me the latest version works if I use up to 1 level deep:

I have the following model structure Products can contain Comments (we use fill for this). A newsfeed can contain Products.

If I use mongoose-fill with 1 level deep Product --> Comments works as expected

But if I try to use 2 levels deep: Newsfeed --> Product --> Comments it fails.

wclr commented 8 years ago

can you supply a test case or something? There is a test for double nested prop: https://github.com/whitecolor/mongoose-fill/blob/master/test.js#L182

wclr commented 8 years ago

at least your code snippet and error output is needed

sibelius commented 8 years ago

@whitecolor I will provide the test case and maybe a PR with the solution as well, but only on next Monday or Tuesday, thanks for all your help

sibelius commented 8 years ago

are you willing to use ES6/ES7 - async/await - Promises on tests? or even on this plugin itself?

sibelius commented 8 years ago

I've just figure it out why the error is happening.

In this case post.find().fill('comment.like', user), in the case that comment is a field of post not a fill property, Model.__fill (https://github.com/whitecolor/mongoose-fill/blob/master/index.js#L85) will be undefined, receiving the following error: TypeError: Cannot read property 'comment' of undefined

This happen because I don't want to fill comment, because it is not a fill property but a real field, I want to fill only like of comment

wclr commented 8 years ago

Ok, I think I fixed in in 1.1.3 and added couple of tests (new ES7 shiny ones)

rturk commented 8 years ago

With 1.1.3 I don't get any errors (server don't crash) however I don't get any data either.. at least for me any filled field will not show in the final result

wclr commented 8 years ago

hm look at last test

sibelius commented 8 years ago

@whitecolor it is fixed, thanks.

ES7 really shiny

rturk commented 8 years ago

I've Found the problem and a solution for my use case

Mongoose-fill is not compatible with mongoose-deepPopulate if the fill function requires data from the deepPopulate module, even if called prior to the fill function

This is how my NewsFeed Model looks like:

const NewsFeedSchema = new mongoose.Schema({
  date:     { type: Date, required: true, default: Date.now},
  product:  { type: ObjectId, required: false, ref: 'Product'},
  post:     { type: ObjectId, required: false, ref: 'Post'},
}

So in order to populate product comments (product.comments) for a given user we first need to load product. Previously I was using deepPopulate, (as described bellow) but I was able to refactor my code do solely use Mongoose Populate. Note: Mongoose populate now supports deepPopulate (example below)

Previous code using Mongoose-DeepPopulate (incompatible with mongoose-fill)

  ctx.body = await HomeNewsFeed
    .find(find)
    .sort(sort)
    .deepPopulate(['product', 'product.brand', 'product.attributes', 'post', 'post.author'])
    .fill('product.liked', user)
    .fill('product.commentsn', user)
    .fill('post.liked', user)
    .fill('post.commentsn', user)
    .limit(limit);

New code using solely Mongoose populate (fully compatible with mongoose-fill)

  ctx.body = await HomeNewsFeed
    .find(find)
    .sort(sort)
    .populate([{ path: 'product',
                 populate: [ { path: 'attributes' }, // Deep Populate attributes, brand from Product
                             { path: 'brand' } ]
              },
              { path: 'post',
                populate: { path: 'author' } // Deep Populate author from Post
              }])
    .fill('product.liked product.commentsn', user)
    .fill('post.liked post.commentsn', user)
    .limit(limit);
rturk commented 8 years ago

@sibeliusseraphini @whitecolor thank you for all the help!

wclr commented 8 years ago

great :+1: