search

Meteor-Community-Packages / meteor-collection-hooks

Meteor Collection Hooks
https://atmospherejs.com/matb33/collection-hooks
MIT License
657 stars 92 forks source link

Collection hooks mutates selector breaking the '$' array selector #75

Closed cwohlman closed 9 years ago

cwohlman commented 9 years ago

If you define a collection hook for some operation, then using array manipulation doesn't work, eg:

Docs.update({
  'authors.name': 'joe'
}, {
  'authors.$.rating': '5 starts'
}); // Throws: The positional operator did not find the match needed from the query
mizzao commented 9 years ago

Can you make a reproduction repo for this? I feel like it has definitely worked for me before.

Are you sure your database record has an authors array and that there is a name field in one of the objects in the array?

cwohlman commented 9 years ago

I can confirm that using .direct.update works when .update does not.

I'll work on a reproduction.

On Tue, Dec 2, 2014 at 5:23 PM, Andrew Mao notifications@github.com wrote:

Can you make a reproduction repo for this? I feel like it has definitely worked for me before.

Are you sure your database record has an authors array and that there is a name field in one of the objects in the array?

— Reply to this email directly or view it on GitHub https://github.com/matb33/meteor-collection-hooks/issues/75#issuecomment-65324897 .

cwohlman commented 9 years ago

So here's the issue, in one of my before hooks I've got this line:

otherCollection.update(docId, {
   $set: {
    'name': 'joe'
  }
});

Apparently collection hooks has some kind of internal state which is being overridden by this second call to update.

mizzao commented 9 years ago

I think collection hooks fetches the set of documents to be updated before running the before hooks, which in turn happen before the update itself. So modifications done in the before hooks will never change the update set. That might be what you are experiencing.

cwohlman commented 9 years ago

It's not changing the update set (it's a completely different collection, I'm inserting some logging info)

On Tue, Dec 2, 2014 at 6:36 PM, Andrew Mao notifications@github.com wrote:

I think collection hooks fetches the set of documents to be updated before running the before hooks, which in turn happen before the update itself. So modifications done in the before hooks will never change the update set. That might be what you are experiencing.

— Reply to this email directly or view it on GitHub https://github.com/matb33/meteor-collection-hooks/issues/75#issuecomment-65332842 .

cwohlman commented 9 years ago

Let me rephrase that: The update to otherCollection is working correctly, but the update to Docs is not. It's two different collections, the error I'm getting seems to indicate that the selector for the first update operation (Docs.update) is being rewritten to include only the _id of the document being modified.

mizzao commented 9 years ago

You sure you're not missing a {multi: true} as the third argument to update? See https://docs.meteor.com/#/full/update.

cwohlman commented 9 years ago

It's only supposed to modify one document.

On Tue, Dec 2, 2014 at 6:42 PM, Andrew Mao notifications@github.com wrote:

You sure you're not missing a {multi: true} as the third argument to update? See https://docs.meteor.com/#/full/update.

— Reply to this email directly or view it on GitHub https://github.com/matb33/meteor-collection-hooks/issues/75#issuecomment-65333456 .

mizzao commented 9 years ago

Are you using any other packages that also do monkey patching of the Collection APIs (like collection2)?

cwohlman commented 9 years ago

I'll upload the reproduction so you can see what's going on.

On Tue, Dec 2, 2014 at 6:45 PM, Andrew Mao notifications@github.com wrote:

Are you using any other packages that also do monkey patching of the Collection APIs (like collection2)?

— Reply to this email directly or view it on GitHub https://github.com/matb33/meteor-collection-hooks/issues/75#issuecomment-65333760 .

cwohlman commented 9 years ago

Ok, so the error I was getting wasn't actually from the failed updated to Docs but from a failed update that was being performed as part of the before hook. The stack trace didn't include any code from userland so I just assumed that the Docs.update operation was failing.

Thanks for your help! My apologies for opening an issue about bugs in my code.

cwohlman commented 9 years ago

Do you know why the failed updated doesn't include a stack trace into my code, even though everything's being run synchronously? I'd like to make a pull request fixing that if it's possible.

mizzao commented 9 years ago

I think @aramk was trying to fix this issue in #68, but he didn't seem to get it fully working and abandoned it. That might be a good place to start.

cwohlman commented 9 years ago

Thanks!

On Tue, Dec 2, 2014 at 7:39 PM, Andrew Mao notifications@github.com wrote:

Closed #75 https://github.com/matb33/meteor-collection-hooks/issues/75.

— Reply to this email directly or view it on GitHub https://github.com/matb33/meteor-collection-hooks/issues/75#event-201673640 .

aramk commented 9 years ago

I did get the exception stuff working in this branch. I didn't want to merge that PR because of a bug, but I found it shortly afterwards, so I could make another if people find it useful. It will basically call the collection callback with the error when it catches one in the hook callback.

cwohlman commented 9 years ago

I'll take a look. I don't imagine what I was doing was a really common use case, but if your code improves the error reporting I'm sure I won't be the only one to find it useful.

On Tue, Dec 2, 2014 at 10:29 PM, Aram Kocharyan notifications@github.com wrote:

I did get the exception stuff working in this branch https://github.com/aramk/meteor-collection-hooks/tree/feature/exceptions. I didn't want to merge that PR because of a bug, but I found it shortly afterwards, so I could make another if people find it useful. It will basically call the collection callback with the error when it catches one in the hook callback.

— Reply to this email directly or view it on GitHub https://github.com/matb33/meteor-collection-hooks/issues/75#issuecomment-65353969 .

aramk commented 9 years ago

Actually I was attempting to implement a lightweight method of validation where I throw an exception in the hook and this cancels the underlying operation, but other than that it does provide a reason for errors in the hook.

On Thursday, Dec 4, 2014 at 9:34 am, Joshua Ohlman notifications@github.com, wrote:

I'll take a look. I don't imagine what I was doing was a really common use

case, but if your code improves the error reporting I'm sure I won't be the

only one to find it useful.

On Tue, Dec 2, 2014 at 10:29 PM, Aram Kocharyan notifications@github.com

wrote:

I did get the exception stuff working in this branch

https://github.com/aramk/meteor-collection-hooks/tree/feature/exceptions.

I didn't want to merge that PR because of a bug, but I found it shortly

afterwards, so I could make another if people find it useful. It will

basically call the collection callback with the error when it catches one

in the hook callback.

—

Reply to this email directly or view it on GitHub

https://github.com/matb33/meteor-collection-hooks/issues/75#issuecomment-65353969

.

— Reply to this email directly or view it on GitHub.