vsivsi / meteor-file-collection

Extends Meteor Collections to handle file data using MongoDB gridFS.
http://atmospherejs.com/vsivsi/file-collection
Other
159 stars 37 forks source link

Malformed ObjectID values on inserted files #80

Closed DrDanRyan closed 8 years ago

DrDanRyan commented 8 years ago

When I use upsertStream to insert a new file, the returned document in the callback has a strange format for the _id field (which I cannot figure out to even convert to standard ObjectID format). It is an object like:

{ _bsontype: 'ObjectID',
  id: '³£‚Ó°¦ gŠs\u0018' }

If I then call find on the collection and retrieve this document, the ObjectID is a normal instanceof Mongo.ObjectID.

Can you please help me understand what is going on here? I am using Meteor 1.2.1.

vsivsi commented 8 years ago

Hi, this looks like a bug. What you are seeing is the BSON representation of the file object (BSON is the MongoDB equivalent of EJSON). Unfortunately, BSON and EJSON don't represent ObjectIDs the same, so file-collection constantly has to convert back and forth. It looks like a I missed a case here...

You should be able to work around this temporarily by using:


correctId = new Mongo.ObjectID(file._id.toString());

I'll fix this quickly and push out a release, probably today.

DrDanRyan commented 8 years ago

I appreciate the quick response. The returned _id value just turns into '[object Object]' with toString(). I will just put in a call to findOne after the insert (using other uniquely identifying info) as a workaround for now.

Let me know if I can provide you with any further details you may find helpful.

vsivsi commented 8 years ago

That's strange... Are you sure you're invoking .toString() on the _id value, and not the whole file object?

vsivsi commented 8 years ago

I just committed a fix on the dev branch.

vsivsi commented 8 years ago

How about .toHexString()?

DrDanRyan commented 8 years ago

nope, and no helpful .valueOf() it really just seems like a simple {a: 1, b: 2} type of vanilla JS object. Maybe it got stringified and reparsed or something? The id property is clearly some strange string encoding, not sure which...

vsivsi commented 8 years ago

Hmmm. Okay.

Can you try out the package on the dev branch and let me know if it solves this issue for you?

DrDanRyan commented 8 years ago

Ok, how do I add the dev branch version to my Meteor project?

DrDanRyan commented 8 years ago

I think the problem is with the constructor call to writeStream

writeStream = Meteor.wrapAsync(@gfs.createWriteStream.bind(@gfs))
            root: @root
            _id: mongodb.ObjectID("#{file._id}")
            mode: 'w'
            timeOut: @lockOptions.timeOut
            lockExpiration: @lockOptions.lockExpiration
            pollingInterval: @lockOptions.pollingInterval

At this point, file._id is already constructed with Meteor.Collection.ObjectID, so maybe the _id line should be:

_id: mongodb.ObjectID("#{file._id.toHexString()}")

Is conversion through hex string necessary because Meteor's Mongo.ObjectID and mongodb native driver's ObjectID are not the same? If conversion is not necessary, it could just be

_id: file._id
DrDanRyan commented 8 years ago

After reading the constructor chain up to the gridfs-stream project, it looks like you can actually just pass the hex string in and it will turn it into the ObjectId for you... so

_id: file._id.toHexString()

should work too

vsivsi commented 8 years ago

I'm trying to figure out why you and I are getting different results from this.

Can you please try the following:

git clone --recursive https://github.com/vsivsi/meteor-file-collection.git fcTest
cd fcTest
git checkout dev
meteor test-packages ./

Now go to a browser and open localhost:3000

There should be 56 passing tests.

vsivsi commented 8 years ago

If those unit tests pass, then you can try the dev branch in your app by doing this...

# Go to your app root directory
git clone --recursive https://github.com/vsivsi/meteor-file-collection.git packages/fcTest
cd packages/fcTest
git checkout dev
cd ../..

Now edit your .meteor/versions file and change the vsivsi:file-collection line to:

vsivsi:file-collection@1.3.0

Then start your app and test.

vsivsi commented 8 years ago

Note the version above should be 1.3.0

DrDanRyan commented 8 years ago

Ok, will do in the morning, thanks for your help. Will let you know as soon as I have results.

DrDanRyan commented 8 years ago

Dev branch works. Thanks a lot!

Still seems really strange to me though that I could not call toHexString() on the object returned in version 1.2.2... If I ever figure out what that was about, I will let you know.

vsivsi commented 8 years ago

Great! I'm going to reopen this until I publish the fix to Atmosphere with the rest of the 1.3.0 changes. Probably today or tomorrow. I'll close this again when that is done.

vsivsi commented 8 years ago

Fixed in 1.3.0 on Atmosphere now.