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

Inserting field named 'length' not allowed #594

Closed
raix opened this Issue Jan 8, 2013 · 29 comments

Comments

Projects
None yet
8 participants
@raix
Contributor

raix commented Jan 8, 2013

Hi when trying a simple collection.insert({ owner: Meteor.userId(), length:3 }); in Meteor it's executed by client but revoked by server (or in process, same for collection.update) At the moment I renamed the field length to 'len' and voila it works... But now I'm not in compliance with the GridFS spec saying: 'fs.files.length' (the project: collectionfs.meteor.com)

raix added a commit to raix/meteor that referenced this issue Jan 8, 2013

meteor#594 - Insert field named 'length' not allowed
replaced _.each with for in
in functions set, unset, complete, methods
@raix

This comment has been minimized.

Contributor

raix commented Jan 10, 2013

Tried replacing _.each in livedata_server - It works, but the use of .lenght breaks the live update - guess the event que cant handle errors?

raix added a commit to raix/meteor that referenced this issue Jan 11, 2013

Issue meteor#594
Seems to work now, .length field accepted at insert and update, and is
updated live. Replaced some _.each, Check if 'for in' used on array
@n1mmy

This comment has been minimized.

Member

n1mmy commented Jan 12, 2013

Yipes! This is a serious issue. Meteor uses _.each to iterate over objects with user defined keys all over the place. We need to either fix up _.each (possibly using Lo-Dash), or replace all uses of _.each that iterate over objects with user-provided keys.

Good catch, and thanks for the report!

@raix

This comment has been minimized.

Contributor

raix commented Jan 12, 2013

I would go for replacing it, making a fixed each would save the time of rewrite, but imo:

  • The code looks clearer without each function
  • Each handles something allready handled nicely in js
  • Just extra text to upload eg. 'Function(){}' cannot be minimized, though handled by gzip
  • Each makes debugging harder? (Finding Line 301 in mongo_driver.js was slow)
  • It's slow compared to pure js, eg:

When having a push array the
While (a.length) foo(a.pop());
Is much faster than a normal: (since it dont spend time on undefined indexes)
For (var i;....)
This is again faster than:
For (var foo in bar) since for in loops through all items with string index
Again, in one place i saw a:
_.each(foo, function(dummy, id){ bar(id); }
Instead of simply eg.:
For (var id in foo) bar(id);
My opinion migth seem old school, i know

@glasser

This comment has been minimized.

Member

glasser commented Jan 12, 2013

The advantage to using some sort of function for loops is that (a) for objects, we don't have to check hasOwnProperty ourselves and (b) we can use the loop value or key in a closure and it actually keeps the value from that iteration of the loop. The problem is just that Underscore's functions don't work well for these arbitrary user supplied objects.

We should switch to less polymorphic functions.

@raix

This comment has been minimized.

Contributor

raix commented Jan 12, 2013

hasOwnProperty is only a problem when using 'for in..'? But agree, dont get (b) are you talking scope?

@awatson1978

This comment has been minimized.

Contributor

awatson1978 commented Jan 12, 2013

Agreed. One of the strengths of meteor is that it sacrifices a bit of performance to provide a very friendly high-level environment, where objects are generally automatically pulled from the database well-constructed and well-behaved. Compared to developing on SQL and Oracle systems, I've definitely noticed the lack of need to use coding constructs such as hasOwnProperty, and it's well appreciated!

Please correct me if I've followed these emails incorrectly, but as I understand things, the original reason that inspired this discussion of .len and .length, is that Morten is trying to get FSCollection working, so that the minimongo spec matches the GridFS spec, so we can use keep images in the mongo database and not resort to filepicker.io type shenanigans.

If that's the case, it would be painful to have to horse-trade being able to pull well-constructed objects from the database for inclusion of images in the database.

::sent from my tablet::

On Jan 12, 2013, at 8:48 AM, David Glasser notifications@github.com wrote:

The advantage to using some sort of function for loops is that (a) for objects, we don't have to check hasOwnProperty ourselves and (b) we can use the loop value or key in a closure and it actually keeps the value from that iteration of the loop. The problem is just that Underscore's functions don't work well for these arbitrary user supplied objects.

We should switch to less polymorphic functions.


Reply to this email directly or view it on GitHub.

@raix

This comment has been minimized.

Contributor

raix commented Jan 12, 2013

Well, I've decided to workaround by convert to string: .length = ''+file.size(); for now (underscore checks if length is a number and then assumes an array like object, by design),

Its still not compliant the gridFS spec defines length as a number...

Hope you'll go for the lodash, and replace the each with a forOwn?:

  • livedata_server set, unset
  • mongo_livedata ln301 change

Deleted, altered comments quit a bit, every time I had a:'can' or 'cant' moment, do you guys get new email every time? If so, sorry - kinda new to github

@n1mmy

This comment has been minimized.

Member

n1mmy commented Jan 12, 2013

Glad you have a workaround that works for you. We definitely still want to solve this, it is a nasty surprise for users when using a number length field suddenly breaks things. It's a natural thing to want to do, and we should support it.

@raix

This comment has been minimized.

Contributor

raix commented Jan 12, 2013

Great, thanks

@n1mmy

This comment has been minimized.

Member

n1mmy commented Jan 12, 2013

Also, FYI, people do not get an email each edit. Only one email on the initial comment. So no need to worry about spamming people =)

@raix

This comment has been minimized.

Contributor

raix commented Jan 12, 2013

:)

raix added a commit to raix/meteor that referenced this issue Jan 18, 2013

raix added a commit to raix/meteor that referenced this issue Jan 19, 2013

Created tests for Issue meteor#594
Tests insert and update, (not allow/deny) At the moment insert and
update fails when on clientside.
@glasser

This comment has been minimized.

Member

glasser commented Mar 26, 2013

We should check the reproduction example https://github.com/jtblin/meteor-issue-876 of a particularly egregious error message when we think we've fixed this.

@raix

This comment has been minimized.

Contributor

raix commented Mar 26, 2013

  • wouldn't it be good practice to add tests for it in Meteor?
  • Are you going for lodash foreach or extending _. with your own foreach?
@jdalton

This comment has been minimized.

jdalton commented Apr 18, 2013

Just a heads up, Lo-Dash provides _.forOwn and _.forIn for iterating objects and won't short circuit when objects have length properties and smooths over iteration bugs in various browsers.

@mschloesser-r7

This comment has been minimized.

mschloesser-r7 commented Jun 25, 2013

any updates on this? just ran into the same issue trying to access gridfs...

@raix

This comment has been minimized.

Contributor

raix commented Jun 27, 2013

@n1mmy, this bug is getting old
Are you going for lodash foreach or extending _. with your own foreach? Or something else, if you want contributions gotta have some guidelines

@raix

This comment has been minimized.

Contributor

raix commented Jul 17, 2013

I'm closing this - still an issue, but the lack of response made it +6 months which moves status far away from critical bug in my world.

It can be workedaround by converting to string e.g. .length = '' + myLength;

Not sure why @mongodb would use a near keyword length instead of a more telling size in the first place.
The gridFS support in Meteor is not installed 'in' bundle, last time I looked - not sure what future the native build of gridFS is going to be if any.

I'm sure @meteor will pick this up at some time, since removing _.dependencies is in their docs.

@raix raix closed this Jul 17, 2013

@awwx

This comment has been minimized.

Contributor

awwx commented Jul 17, 2013

@raix I think you should leave this open because it's still a bug.

@raix

This comment has been minimized.

Contributor

raix commented Jul 17, 2013

@awwx okay

@trentlarson

This comment has been minimized.

trentlarson commented Dec 23, 2013

I just got bit by this behavior. http://stackoverflow.com/questions/20738226/why-would-one-meteor-client-collection-behave-different-from-what-is-shown-in-mo

As long as there's a problem, it would be helpful to detect and throw an error on this kind of input, at least on the client side. (I wonder if there are other fields that cause problems.)

@glasser

This comment has been minimized.

Member

glasser commented Jan 8, 2014

Fixed in bab936e (GitHub seems to be glitchy and didn't notice).

@glasser glasser closed this Jan 8, 2014

@jdalton

This comment has been minimized.

jdalton commented Jan 8, 2014

Not sure if this matters but some older versions of jQuery (< v1.5) and the current version of Zepto: $('div').constructor === Object is true which means they will stop working with those Underscore methods.

Manually patching Underscore seems like the wrong thing to do here.

@glasser

This comment has been minimized.

Member

glasser commented Jan 8, 2014

So what is a better answer?

Switching to lodash is presumably helpful for a lot of reasons, but I don't understand how it will help here.

It would give the opportunity to use forOwned instead of each, but (a) this now requires auditing the entire Meteor codebase to carefully change many but not all eaches into forOwned and (b) it doesn't fix all of the other collection functions like, say, _.size which will happily tell you that _.size({length: 5}) is 5, or _.all, or ...

The expectation of most Meteor users has been "when I decide that an object (that I'm putting in a database, eg) has a field called length that's a number, that shouldn't magically make it an array", and as far as I can tell lodash does nothing to change that.

I know that this check isn't perfect. Thanks for pointing out the jQuery/zepto issue, though doesn't jQuery have its own methods that perform much of the functionality of underscore? http://api.jquery.com/category/traversing/

It's much more important to me that in a Meteor app, you be able to use properly use underscore methods with the objects that are the lifeblood of a Meteor app (documents read from the database, published over DDP, and stored in the local cache) than that you be able to use them with jQuery objects which have their own array-style methods...

glasser added a commit that referenced this issue Jan 8, 2014

Patch _.each to not treat {length: 5} as an array
Specifically, in all Underscore "collection" functions which treat their
arguments polymorphically as either "object-like" or "array-like", don't
treat arguments with `x.constructor === Object` as arrays (except for
the 'arguments' object).

Fixes #594. Fixes #1737.
@jdalton

This comment has been minimized.

jdalton commented Jan 8, 2014

It would give the opportunity to use _.forOwn instead of each, but (a) this now requires auditing the entire Meteor codebase to carefully change many but not all eaches into forOwned

Methods like _.forOwn, _.forIn, _.transform, _.pick, _.omit, & _.mapValues allow working with objects, so it at least gives devs an option for handling these kinds of cases.

and (b) it doesn't fix all of the other collection functions like, say, _.size which will happily tell you that _.size({length: 5}) is 5, or _.all, or ...

Right, as those are "Collections" methods. Lo-Dash does provide "Objects" methods like _.forOwn and friends to cater to object centric iteration. If you wanted the number of keys in an object and wanted to avoid array-like issues of the "Collections" method _.size you'd use the "Objects" method _.isEmpty or _.keys like _.keys(obj).length.

The expectation of most Meteor users has been "when I decide that an object (that I'm putting in a database, eg) has a field called length that's a number, that shouldn't magically make it an array", and as far as I can tell lodash does nothing to change that.

Looks like a case of not reading the docs. Both Underscore and Lo-Dash separate methods into categories like "Collections", "Arrays", "Objects", and so on.

I know that this check isn't perfect. Thanks for pointing out the jQuery/zepto issue, though doesn't jQuery have its own methods that perform much of the functionality of underscore? http://api.jquery.com/category/traversing/

Even if jQuery & Underscore have similar methods usually one wins out. I know some prefer using Underscore methods for consistency even if there are working with jQuery collections.

It's much more important to me that in a Meteor app, you be able to use properly use underscore methods with the objects that are the lifeblood of a Meteor app

At the expense of breaking established behaviors of a common lib. If you want to ensure Meteor handles things a certain way then why not expose specific apis to do that.

@glasser

This comment has been minimized.

Member

glasser commented Jan 8, 2014

In the context of Meteor, it's much more important to be able to handle plain old object with a numeric length field consistently with MongoDB, JSON REST APIs, etc, than to handle zepto return values correctly.

It was disappointing that Underscore decided to make a backwards-incompatible change in 1.4.0 that basically breaks the ability to use Underscore collection functions with plain old JSON objects.

Had we realized that Underscore was likely to make a backwards-incompatible change to treat some plain JSON objects as arrays, perhaps Meteor would have not chosen to use Underscore so pervasively, and perhaps we will find time later to completely rewrite Meteor to remove all internal uses of Underscore. But that's not going to happen before Meteor 1.0.

I mean, I understand that Underscore made the choice in 1.4.0 to redefine what a "collection" is. That doesn't mean it was a good choice, or one that doesn't have all sorts of random negative impacts on systems that chose to use Underscore (or an Underscore-compatible library like lodash).

@jdalton

This comment has been minimized.

jdalton commented Jan 8, 2014

In the context of Meteor, it's much more important to be able to handle plain old object with a numeric length field consistently with MongoDB, JSON REST APIs, etc, than to handle zepto return values correctly.

Libs like Lo-Dash give you more than enough Objects methods to handle plain old objects with numeric lengths.

It was disappointing that Underscore decided to make a backwards-incompatible change in 1.4.0 that basically breaks the ability to use Underscore collection functions with plain old JSON objects.

Not sure what you mean, Underscore has had a length check of some kind since it was started.

@glasser

This comment has been minimized.

Member

glasser commented Jan 9, 2014

Oops, I'm not sure how I came to the conclusion that this used to work before 1.4.0. Sorry about that. (I know that they considered changing the behavior before 1.4.0 and reverted it.)

In any case, I think it does more of a service to Meteor users to have iteration constructs (and again, it's not just each... there's all, any, find...) that work without surprises on any structure that you get by parsing JSON (and as extra sugar on top, also work with DOM NodeLists, etc), than to have iteration constructs that work with zepto return values. Underscore is welcome to make up whatever conventions it would like about duck typing, but this particular choice is user-unfriendly. I hope that after 1.0 we will have time to do a full audit of every iteration in our codebase and make more precise choices (though I have no idea how you're expected to be able to do _.all over an object that might have a numeric length field without just giving up on Underscore).

@glasser

This comment has been minimized.

Member

glasser commented Jan 9, 2014

Since it's definitely been the case that several times dealing with this issue I've misunderstood the state of underscore/lodash: It is still the case that there are no variants even in lodash of all, any, contains, find, findLast, max, min, reject, etc which interpret plain JS objects with an numeric length field as objects, right?

Maybe the answer is to provide another build of lodash (like the existing underscore/strict/mobile/etc builds) for users who value consistent behavior with plain JS objects over the ability to use lodash with duck-typed arrays?

@jdalton

This comment has been minimized.

jdalton commented Jan 9, 2014

It is still the case that there are no variants even in lodash of all, any, contains, find, findLast, max, min, reject, etc which interpret plain JS objects with an numeric length field as objects, right?

// collections → objects
_.map_.mapValues
_.reject_.omit
_.filter_.pick
_.reduce_.transform

// other object methods
_.cloneDeep
_.create
_.findKey
_.findLastKey
_.forIn
_.forInRight
_.forOwn
_.forOwnRight
_.isEmpty
_.isPlainObject
_.keys
_.merge

With the iteration methods _.forOwn and _.transform most other "Collections" methods can be implemented as "Objects" methods

Maybe the answer is to provide another build of lodash (like the existing underscore/strict/mobile/etc builds) for users who value consistent behavior with plain JS objects over the ability to use lodash with duck-typed arrays?

Lo-Dash has added "Objects" methods when they've been popular requests.

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