Collections operations breaking when adding a collection helper in a FileCollection (and potentially in any Mongo.Collection instance) about meteor-collection-helpers HOT 19 CLOSED

Hey @RaniereSouza seems the underlying problem is that FileCollection doesn't play nicely with collection-helpers as it applies the transform function after the collection is instantiated. I'm not sure there's any changes that should be made here to avoid the problem other than checking for an instance of FileCollection perhaps and then throwing up a warning. Though it would seem more correct for FileCollection to account for collection-helpers if possible!

Edit: Sorry, the tone of my original comment came across as unintentionally negative!

from meteor-collection-helpers.

@dburles Hi, the bug appears to happen even when using a vanilla Mongo.Collection with no transform defined. I don't understand how FileCollection can "account" for that. FileCollection is built entirely on top of Mongo.Collection and does not modify the underlying Meteor Mongo implementation in any way. As such it would be impossible for it to effectively "account" for other Atmosphere packages that patch/extend/replace core Meteor APIs in unknown ways.

The heart of this issue is that a .find() operation using a selector that contains a valid unique _id value is returning more than one result (and is actually returning every document in the collection.) I can reproduce that using a completely vanilla Mongo.Collection when a helper function is added to it using meteor-collection-helpers. As near as I can tell, this issue has nothing to do with FileCollection other than that the bug was first encountered in that context.

from meteor-collection-helpers.

@dburles

You can find a complete reproduction of this bug without any connection to the FileCollection package here:

https://github.com/vsivsi/helper-bug-repro

from meteor-collection-helpers.

@vsivsi very interesting, looks like it's an issue with Meteor core. See here: vsivsi/helper-bug-repro@master...dburles:master

from meteor-collection-helpers.

Interesting... I've definitely done this type of thing before using Coffeescript classes (pre ES6) without any problems, but maybe I never hit on this specific combination of factors. See e.g.

https://github.com/vsivsi/meteor-file-job-sample-app/blob/master/sample.coffee#L21

from meteor-collection-helpers.

I assume you are planning on filing an issue with MDG for this. I'm happy to chime in over there if you get any pushback, but this seems pretty airtight.

from meteor-collection-helpers.

Is it common to pass an entire document (including ObjectId's) as a selector? It's not something I've ever found a need for. I.e changing: testColl.find(testColl.findOne()).count() != 1 to testColl.find(testColl.findOne()._id).count() != 1 works as expected

from meteor-collection-helpers.

Either way if the behaviour changes when the collection is transformed in this way it's still a bug. Happy for you to open a bug report since you have some more context around the issue.

from meteor-collection-helpers.

Should I close this Issue? since it looks like a problem within the core Mongo.Collection when it's applied a transform operation to an instance, and in the end it's neither dburles:collection-helpers nor vsivsi:file-collection packages malfunctioning...

EDIT: @vsivsi I've seen in the Meteor Issue #6416 page that you treated the "monkey-patch/replacing" problem as an "anti-pattern", a bad practice within Meteor's community. Does it means that packages like dburles:collection-helpers should adapt their behavior to avoid this kind of action? (in this case, the problem actually IS really related to dburles:collection-helpers itself, and this Issue here should not be closed)

from meteor-collection-helpers.

@RaniereSouza The decision on whether the use of packages like dburles:collection-helpers in your project is an "anti-pattern" is yours to make. I personally avoid using them because in my experience they make the Meteor core APIs difficult to reason about when problems are encountered (this issue being a case in point). In fairness, @dburles appears to be correct that in this instance the bug is in Meteor itself, but in my experience that is not typical in these sorts of cases (and I've dealt with more than a few).

@dburles In this case I use the entire document as the selector because this is part of a remote file/document removal process, and I want to ensure that the permission metadata hasn't changed since it was validated, avoiding a race condition. I.e. If the file metadata has changed since the file remove was remotely requested, then fail gracefully (and allow the remote client to retry) rather than adding and maintaining a bunch of logic to try to determine if the change might modify the outcome of the permission checks. There are other ways this could be accomplished, but as you point out, this is a completely valid approach that should work.

from meteor-collection-helpers.

@dburles @RaniereSouza On who should report this bug to Meteor, I'm frankly not interested in owning it. I've spent too much time on this issue as it is, and I'm a couple of levels removed from the direct impact of it, so I feel like I've done my part. As I said above, if you get pushback on it, I'm happy to rally in support, but otherwise I'm going to need to step away from this issue.

from meteor-collection-helpers.

Fair enough, I'll throw together a super simple reproduction and post a bug report

from meteor-collection-helpers.

I'll close this Issue then. Thanks @dburles and @vsivsi for taking your time to look into this problem.

from meteor-collection-helpers.

Digging into it a little bit more, I've narrowed down the cause to the length field each document contains, it seems like somewhere in Meteor internals a transformed document length field is being interpreted as the native Javascript length method rather than a regular field.

from meteor-collection-helpers.

Sounds familiar: oortcloud/ddp-ejson#2

from meteor-collection-helpers.

Here's a repro if you're interested. The second test fails.

https://github.com/dburles/transform-bug/blob/master/packages/bug/bug-tests.js

from meteor-collection-helpers.

That's interesting! 2014 hey

from meteor-collection-helpers.

Repro looks good, thanks for accepting the baton on this.

from meteor-collection-helpers.

No problem. Seems like it's most directly affecting this package after all! Here's the bug report meteor/meteor#8329

from meteor-collection-helpers.