Collections operations breaking when adding a collection helper in a FileCollection (and potentially in any Mongo.Collection instance)

[RaniereSouza]

Hello, @dburles !

I don't know if a problem like that was issued before, but I recently found an error (with the help of @vsivsi), which firstly I mistook as a (possible) problem in the package vsivsi:file-collection, but it ended up actually being a problem when assigning a collection helper to a FileCollection instance (and potentially to any Mongo.Collectioninstance).

The problem started with a .remove() operation on the Client side filtered by ObjectID ending up removing ALL files from the FileCollection instance. That's certainly an undesired behavior, and it seemed very strange, as it was just a plain Client side .remove() by ObjectID operation. So @vsivsi tracked this error to be triggered only when assigning a collection helper to the FileCollection instance. He found out that some operations in the collection returned wrong results when a collection helper was assigned to it. And more surprisingly, the same operations returned wrong results when assigning collection helpers to regular Mongo.Collection instances. So, because of that, the Client side .remove() operation of the FileCollection instance was affected.

You can see more detailed information about how this error was tracked at the Meteor FileCollection package Issue #152 page, where it's explained which operations were tested in the meteor mongo and in the meteor shell, and why it affected the Client side .remove() operation in the FileCollection instance. I'm sure this is an undesired behavior in your package, so I'm making you aware of the problem.

[dburles]

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!

[vsivsi]

@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.

[vsivsi]

@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

[dburles]

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

[vsivsi]

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

[vsivsi]

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.

[dburles]

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

[dburles]

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.

[RaniereSouza]

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)

[vsivsi]

@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.

[vsivsi]

@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.

[dburles]

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

[RaniereSouza]

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

[dburles]

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.

[vsivsi]

Sounds familiar: oortcloud/ddp-ejson#2

[dburles]

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

[dburles]

That's interesting! 2014 hey

[vsivsi]

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

[dburles]

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