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

Javascript reserved words / transformed collections bug #8329

Closed
dburles opened this Issue Feb 8, 2017 · 8 comments

Comments

@dburles
Contributor

dburles commented Feb 8, 2017

This issue was originally brought to my attention via the dburles:collection-helpers package by @RaniereSouza and @vsivsi.

In spending some time investigating it further it appears that using a transformed document containing a numeric field named length as a query selector seemingly mangles the resulting query.

Simple reproduction (transform example from the Meteor documentation):

Animal = function (doc) {
  _.extend(this, doc);
};

_.extend(Animal.prototype, {
  makeNoise: function () {
    console.log(this.sound);
  },
});

export const Test = new Mongo.Collection('test', {
  transform: function (doc) { return new Animal(doc); }
});

const docs = [
  { length: 285113 },
  { length: 14203 },
  { length: 133052 },
];

docs.forEach(doc => Test.insert(doc));

const doc = Test.findOne();

test.equal(Test.find(doc).count(), 1);

In this example the test fails as the resulting query returns 3 documents instead of 1.

Full reproduction app

Note that this also affects the remove query.

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 8, 2017

Thanks for the excellent reproduction @dburles - I'm seeing the same thing.

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 8, 2017

Interesting - the problem happens here:

_.each(selector, function (value, key) {

When key contains length, bad things happen within the _.each call. Turns out this is a known underscore issue: jashkenas/underscore#1590

I haven't digested that entire thread yet (not enough coffee!), but I'll take a closer look shortly. Replacing the _.each call with something else should fix this issue. I'll try to get a PR submitted later today (if no-one else gets to it before me). Thanks!

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 8, 2017

Ha - I just noticed this in the underscore each docs:

Note: Collection functions work on arrays, objects, and array-like objects such as arguments, NodeList and similar. But it works by duck-typing, so avoid passing objects with a numeric length property.

🙂

@hwillson hwillson self-assigned this Feb 8, 2017

@vsivsi

This comment has been minimized.

vsivsi commented Feb 8, 2017

Just some history on this Underscore each problem in Meteor (it goes way back...)

https://github.com/meteor/meteor/blob/devel/packages/underscore/underscore.js#L88

My understanding has been that Meteor "fixed" this issue in Underscore by forking and patching its own private copy.

I'd be very interested to know how this bug evaded that "fix".

@dburles

This comment has been minimized.

Contributor

dburles commented Feb 9, 2017

Yeah, perhaps rather Object.keys(...).forEach(...) would be more robust...

Edit: I just tried this and the tests pass as expected. I'm just not sure on whether this would have any side effects.

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 16, 2017

Hi guys - sorry for the delay on this. I've looked more closely at why this is failing. The problem happens here:

var looksLikeArray = function (obj) {
  return (obj.length === +obj.length
          // _.isArguments not yet necessarily defined here
          && (_isArguments(obj) || obj.constructor !== Object));
};

This returns true for the Animal object returned from the test's transform function, which means the following block of code is run:

for (var i = 0, length = obj.length; i < length; i++) {
  if (iterator.call(context, obj[i], i, obj) === breaker) return;
}

This is clearly not what should be happening, since the incoming object is being treated like an array. So we can either fix this check in Meteor's custom underscore package, or replace the _.each call. Since Meteor will be eventually moving away from underscore completely, replacing the Mongo.Collection._rewriteSelector functions _.each call with a Object.keys(selector).forEach call sounds like a plan to me. I've submitted PR #8380 to help with this. Thanks!

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 22, 2017

PR #8380 was merged, so the fix will be coming soon. Thanks for reporting this!

@hwillson hwillson closed this Feb 22, 2017

@abernix

This comment has been minimized.

Member

abernix commented Mar 9, 2017

This should be fixed in Meteor 1.4.3.2. You can try the latest 1.4.3.2 beta and help confirm by running:

meteor update --release 1.4.3.2-beta.0

Please report back if you encounter any problems, and thanks for taking reporting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment