Closed RaniereSouza closed 7 years ago
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!
@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.
@dburles
You can find a complete reproduction of this bug without any connection to the FileCollection package here:
@vsivsi very interesting, looks like it's an issue with Meteor core. See here: https://github.com/vsivsi/helper-bug-repro/compare/master...dburles:master
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
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.
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
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.
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)
@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.
@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.
Fair enough, I'll throw together a super simple reproduction and post a bug report
I'll close this Issue then. Thanks @dburles and @vsivsi for taking your time to look into this problem.
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.
Sounds familiar: https://github.com/oortcloud/ddp-ejson/issues/2
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
That's interesting! 2014 hey
Repro looks good, thanks for accepting the baton on this.
No problem. Seems like it's most directly affecting this package after all! Here's the bug report https://github.com/meteor/meteor/issues/8329
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 aFileCollection
instance (and potentially to anyMongo.Collection
instance).The problem started with a
.remove()
operation on the Client side filtered byObjectID
ending up removing ALL files from theFileCollection
instance. That's certainly an undesired behavior, and it seemed very strange, as it was just a plain Client side.remove()
byObjectID
operation. So @vsivsi tracked this error to be triggered only when assigning a collection helper to theFileCollection
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 regularMongo.Collection
instances. So, because of that, the Client side.remove()
operation of theFileCollection
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 themeteor shell
, and why it affected the Client side.remove()
operation in theFileCollection
instance. I'm sure this is an undesired behavior in your package, so I'm making you aware of the problem.