Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When the selector is not an id (= server side), does the after.update hook work? #38

Closed
darkship opened this issue Apr 23, 2014 · 23 comments

Comments

@darkship
Copy link

Did someone try it ? Or is it a case where the hooks isn't supposed to work?

When my selector wasn't an Id, my data was updated but my after.update hook wasn't called, so I updated one document after an other to trigger the hook.

Note : before.update is Ok

@mizzao
Copy link
Contributor

mizzao commented May 6, 2014

Are you saying before.update runs for all the documents but after.update does not?

@darkship
Copy link
Author

darkship commented May 6, 2014

Exactly.

Collection.update({foo:"xyz"},{$set:{foo:"abc"}},{multi:true}) does the update and triggers before.update but not after.update

That's why I did:

Collection.find({foo:"xyz"}).forEach(function(col){
      Collecion.update(col._id,{$set:{foo:"abc"}})
})

so that both, before and after, are triggered

@gabrielhpugliese
Copy link

Same issue here. What I'm doing is this:

test.after.update(function (userId, doc) {
    var x = 1;
    test.update({ _id: doc._id }, { $set: { x: x } });
});

@mizzao
Copy link
Contributor

mizzao commented May 8, 2014

@gabrielhpugliese if after.update was working as advertised, your code would result in an infinite loop.

@gabrielhpugliese
Copy link

Lol yeah, true story. I think the buffer just exploded. Anyway, how can we
handle the issue now? I really need the after.update

node: ../src/node_object_wrap.h:60: static T*
node::ObjectWrap::Unwrap(v8::Handlev8::Object) [with T = node::Buffer]:
Assertion `!handle.IsEmpty()' failed

Gabriel Pugliese
CodersTV.com
@CodersTV

On Wed, May 7, 2014 at 9:32 PM, Andrew Mao notifications@github.com wrote:

@gabrielhpugliese https://github.com/gabrielhpugliese if after.updatewas working as advertised, your code would result in an infinite loop.


Reply to this email directly or view it on GitHubhttps://github.com//issues/38#issuecomment-42501125
.

@gabrielhpugliese
Copy link

I think I need a rest :(

@mizzao
Copy link
Contributor

mizzao commented May 8, 2014

I am extremely confused by your penultimate post.

@gabrielhpugliese
Copy link

After you saying about the loop, I've checked my Meteor console and it was raising this error (probably because of the loop):
node: ../src/node_object_wrap.h:60: static T* node::ObjectWrap::Unwrap(v8::Handle<v8::Object>) [with T = node::Buffer]: Assertion!handle.IsEmpty()' failed`

But, disconsider that, please. Sorry for that.
I'm just facing the same problem as @darkship - after.update is not working as I expected. I'm trying to update doc.x = x; and it's not saving to db.

@matb33
Copy link
Collaborator

matb33 commented May 8, 2014

I'm unable to reproduce this. I even added multi: true and still no luck reproducing -- test passes. Perhaps you can modify the test until it fails? It's in the master branch:

https://github.com/matb33/meteor-collection-hooks/blob/master/tests/update_without_id.js

@matb33
Copy link
Collaborator

matb33 commented May 8, 2014

Can you show us more of your code? I suspect you are trying to modify the doc in the after update by either setting a property on doc (which won't work) or running an update again (which may go into an infinite loop without the right guards in place)

@darkship
Copy link
Author

darkship commented May 8, 2014

I tried out and reproduced it. https://github.com/darkship/collection_hooks_bug_after_update.

I noticed that it didn't happened in every case.
coll.update({random:i},{$inc:{random:1}},{multi:true}) -> fails to trigger after.update
coll.update({random:{$gt:i}},{$inc:{random:1}},{multi:true}) --> triggers after.update

@matb33
Copy link
Collaborator

matb33 commented May 8, 2014

@darkship I cloned your repo and got the example running. I am able to click on the +1 button of any and they increment accordingly, eventually all falling into step and incrementing together. Looks like it's working perfectly.

I noticed some remnants of Collection2 in there. Do you have it enabled when it fails for you?

@darkship
Copy link
Author

darkship commented May 8, 2014

I tried with both, it had the same effect.

When you say it's working, did you see any logs? When you click on "+1" the update is done but I never see the "after" in the server console, only the before.

coll.before.update(function(userId, doc, fieldNames, modifier, options){
    console.log("before",doc.count)
})
coll.after.update(function(userId, doc, fieldNames, modifier, options){
    console.log("after",doc.count)
})

@matb33
Copy link
Collaborator

matb33 commented May 8, 2014

OK I understand what's going on. The problem is that the after hook has no idea which documents you're referring to anymore because your original selector no longer matches any documents after the update occurred.

For example, let's say your selector used to determine which docs to update is {random: 76}. Let's say that matches 50 documents, and proceeds to run your update that increments the value of random by 1. Now all those 50 documents have the value of 77 for the field random. After the update has finished, meteor gives me back the number of affected documents (50), nothing else -- no ids. The "after" code tries to find these documents and call the after hook for each of them. It tries to use the original selector {random: 76} but of course this won't match anything -- all the values for random are now 77.

I'm not sure how to proceed. Any ideas?

@mizzao
Copy link
Contributor

mizzao commented May 8, 2014

Don't we have the _ids from the find operation that was run?

@matb33
Copy link
Collaborator

matb33 commented May 8, 2014

I think we do! Good call, gonna try

@mizzao
Copy link
Contributor

mizzao commented May 8, 2014

While you're thinking about that, do you think it would be possible to short circuit the per-document update/remove if no hooks are installed for a collection? It just seems awfully inefficient to use the find and per _id update if it doesn't need to be done.

One can also use the direct operations if they realize this, of course.

@matb33 matb33 closed this as completed in 0ae1f0f May 8, 2014
@matb33
Copy link
Collaborator

matb33 commented May 8, 2014

@mizzao sure thing. I'm assuming I did what you meant: f1d2ac6

@mizzao
Copy link
Contributor

mizzao commented May 8, 2014

That looks right as long as it works :)

@darkship
Copy link
Author

darkship commented May 8, 2014

thanks!

@gabrielhpugliese
Copy link

Can you please (please!) can take a quick look on my reproduction? You can clone and just mrt then click on button to create/update the only one doc.
https://github.com/gabrielhpugliese/after-hook-test/blob/master/after-hook-test.js

If I change the after to before it works ok.

@matb33
Copy link
Collaborator

matb33 commented May 8, 2014

@gabrielhpugliese you can't modify the document after the update has already occurred. You'll need to do that in a before hook. If you really want to run an update after an update, you could use the new collection.direct.update

@gabrielhpugliese
Copy link

Hmmmm, I see. Gonna do that on before hook, then. Many thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants