Skip to content
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

Got exception while polling query: TypeError: Object 0 has no method 'substr' #2095

Closed
ghost opened this issue Apr 30, 2014 · 7 comments
Closed

Comments

@ghost
Copy link

@ghost ghost commented Apr 30, 2014

After upgrading Meteor from 0.8.0.1 to 0.8.1 I repeatly get this log message Got exception while polling query: TypeError: Object 0 has no method 'substr'.
It is cause by

var unmakeMongoLegal = function (name) { return name.substr(5); };
.

Here is the Stacktrace:
bildschirmfoto 2014-04-30 um 22 06 54

thing is a Buffer:
bildschirmfoto 2014-04-30 um 22 08 58

The buffer comes from binId:
bildschirmfoto 2014-04-30 um 22 11 23

FS.File is from the collectionFS package.

@avital
Copy link
Contributor

@avital avital commented Apr 30, 2014

Can you please share a small repro? The repro should not use the collectionFS package.

@ghost
Copy link
Author

@ghost ghost commented May 1, 2014

Here is the minimal reproduction code: https://github.com/Sanjo/ejson-repro
Just running it will cause the error.

@estark37 estark37 closed this in 63b3119 May 1, 2014
@estark37
Copy link
Contributor

@estark37 estark37 commented May 1, 2014

Hi @sanjo. Thanks for the clear repro. Inserting an EJSON user-defined type isn't really supported (the same way that you can't insert new Date() or 3 as a document), but there was an easy change that is probably correct regardless that should also fix your issue. We just released it in 0.8.1.1.

@ghost
Copy link
Author

@ghost ghost commented May 1, 2014

Thanks for the quick fixing. It's ok for me that it isn't really supported.
The documents were inserted directly as EJSON because I activated cfs-ejson-file (relevant for CollectionFS/Meteor-cfs-ejson-file#1)

@glasser
Copy link
Member

@glasser glasser commented May 1, 2014

Emily's change fixes the crash, but we talked about it a little more and have decided that the real fix (for the next release) involves preventing the accidental attempt to insert EJSON custom types as top-level Mongo documents.

The encoding of an EJSON custom type is a 2-element document with keys $type and $value. This is what EJSON's own code has always checked:

return _.has(obj, '$type') && _.has(obj, '$value') && _.size(obj) === 2;

Unfortunately, since this encoding is not compatible with MongoDB, we use the keys EJSON$type and EJSON$value instead. Due to an error, this code forgot to check that the document has only two elements, like the corresponding code in ejson.js. So it accidentally allowed you to insert EJSON custom types as top-level Mongo documents. But just because the encoding of EJSON custom types for storage happens to be as "object" doesn't mean it's legitimate to insert it as a document, just like for binary blobs: there's no place to put the _id!

In the next version of Meteor, we'll throw an error if you try to insert non-documents into MongoDB. CollectionFS should be updated to store the custom type in a field on the document.

glasser added a commit that referenced this issue May 1, 2014
Follow-up to 63b3119; further addresses #2095.

There were a few problems here:

- We didn't check that the argument to insert was a document.  (EJSON
  custom types don't count as documents, because they don't have _ids!)

- The check to see if something coming from the database was an EJSON
  custom type didn't match the check in ejson.js (specifically, it was
  missing size===2). This made it sort of look like you could use EJSON
  custom types as top-level documents, until a change in the MongoDB
  driver made made that coincidental almost-working code stop working.

- The replaceNames function wasn't documented as only taking pure JSON,
  so it wasn't obvious that "it throws when there's a Buffer" was a bug
  in the caller rather than a bug in replaceNames.

This should all be resolved now.  Use cases like CollectionFS which were
mislead by these bugs into believing that an EJSON custom type could be
a document should move their custom type into a field.
@aldeed
Copy link
Contributor

@aldeed aldeed commented May 11, 2014

Regarding CFS:

CollectionFS should be updated to store the custom type in a field on the document.

I'm pretty sure we (CFS team) don't have any code that attempts to store the custom type directly into a collection, and we provide the custom type pkg (cfs-ejson-file) only so that developers can easily store a file reference within a related document in another collection, or to easily pass them around over DDP. So I don't think CFS changes are necessary since we don't do anything like what's being done in the repro, but if I'm wrong, someone can submit a CFS issue about this.

@raix
Copy link
Contributor

@raix raix commented May 11, 2014

Just a tiny note:
toJSONValue returns an object representation of a FS.File e.g. { collectionName: '', _id: ''}
fromJSONValue returns an instance of FS.File from the value in json

Problem
This works just great until inserting into the native mongo db, since it would call fromJSONValue - right?

So Meteor would try to insert the FS.File instance into the mongodb. This works fine with Date since the Date is supported in the bson format - but it would not make sense in the FS.File case since we could be carrying binary data.

@glasser not sure what you mean, in general it should be ok to insert a custom ejson type anywhere - but when on top level it should be an object and may currently not contain _id since it would collide when storing into native mongodb since its converted via fromJSONValue, correct?

Typical use:

  var image = images.insert(file); // Track and upload data
  users.insert({
    name: '',
    image: image // Create reference
  });

Is it expected that we actually want to store the fromJSONValue into native mongodb? What about quering, could I do users.find({ image: new FS.File({ _id, collectionName}) }); and get the result I actually wanted?

What about the overhead using custom ejson types, ejson should in so many ways be trimmed for performance and size - maybe deprecate the ejson custom types api?

Just my 2c, I take blame on the cfs team, we actually wanted to deprecate the file-ejson package due to these problems - but a lot of people found it useful so we kept it - now it seems to get us in to trouble.

Kind regards Morten

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.