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

FileCollection _id's #72

Open edemaine opened 8 years ago

edemaine commented 8 years ago

Is there a reason that an _id returned by e.g. FileCollection.findOne() is an ObjectID wrapping a String, instead of a String like I've seen in every other Meteor collection? Is this a consequence of GridFS?

Whatever the reason, I find it odd that I can't use FileCollection.findOne("1234abcd...") to get the file with the specified string _id. Instead I have to do FileCollection.findOne(new Meteor.Collection.ObjectID("1234abcd...")). Would it make sense to check for a String argument in FileCollection.findOne so that it behaves more like Meteor.Collection.findOne?

vsivsi commented 8 years ago

I implemented file-collection to be exactly compliant with the gridFS data model, which specifies that the _id value is an <ObjectID> and not a string. So that's why it is the way it is... Also, the node.js gridFS streaming libraries I use assume the _id is a Mongo ObjectID, so doing it any other way would have been a lot of work!

edemaine commented 8 years ago

Makes sense -- I figured it was something like that. Do you think it's a good idea for FileCollection.findOne to detect a string argument, or is that hiding the truth?...

vsivsi commented 8 years ago

Feels like "hiding the truth" to me. I'd be for it if Meteor hid Mongo objectIDs from the developer, but it totally doesn't...

0o-de-lally commented 8 years ago

@vsivsi Love this package. I'm running into similar issues. Is this documented somewhere? I'm having several issues with this default behavior.

Take for example uploading data and triggering a method with observeChanges :

    myData.resumable.on('fileAdded', function(file) {
      myData.insert({
       _id: file.uniqueIdentifier,
        filename: file.fileName,
        contentType: file.file.type,
      })
    });

    var query = myData.find( {} )
     var handle = query.observeChanges({
      added: function (id, fields) { //id should return the object id. 

      console.log("New File Uploaded");
      var  filedata = myData.findOne( {_id: id } )  // this returns null. Is it using a string instead of object id? 
      console.log("filedata");
    }
  });

Even if I use observeChanges to return the data, do I need to create a new objectID?

Thanks again

vsivsi commented 8 years ago

Hi, sorry for the confusion... The issue here is that resumable.js doesn't know anything about Mongo.ObjectIDs, so when you get the _id from the fileAdded event, it is a hex string, and that is what resumable uses internally, as well as to communicate with the server (the ObjectID is marshaled as a hex string for resumable's purposes.)

In the code above, the implementation of insert will automatically convert that hex string to an ObjectID for you: https://github.com/vsivsi/meteor-file-collection/blob/master/src/gridFS.coffee#L12

What I think you are saying is that in the added callback, the id value you get is a hex string and not an ObjectID? If so, that seems like a bug, in Meteor. But that seems very unlikely, so I think I must be misunderstanding something here...

0o-de-lally commented 8 years ago

@vsivsi, yes it seems like it's returning the hex string and not the object. Seems like a bug. Or Meteor just assumes you are using meteor generated ids.

So my solution was to create an ID under metadata. I went down a rabbit hole trying to figure out why the _id was not being found.

myData.insert({
      _id: file.uniqueIdentifier, // wish this could be blank, and generated by file-collection
      filename: file.fileName,
      contentType: file.file.type,
      metadata: { resumableId: file.uniqueIdentifier}
}

I'm wondering if this is a more explicit way of using the resumable id. Thoughts?

I do wish that the _id in file-collection worked the same as other meteor collections. I understand it's a tradeoff, but the objectID can be at times tricky to remember how to use, after you are used to the Meteor pattern.

vsivsi commented 8 years ago

The resumable ID is initially generated as a Mongo ObjectID!!

See: https://github.com/vsivsi/meteor-file-collection/blob/master/src/resumable_client.coffee#L34

As for the the whole ObjectID mess, it's really a Meteor vs Mongo/gridFS thing. The gridFS spec specifies an ObjectID, and all of the node.js libraries that deal with gridFS expect an ObjectID. But Meteor has made string IDs the default, and so they are easier to deal with. But there's really not much I can do about that short of reimplementing (or writing wrappers for) all of the node.js code for dealing with gridFS that gets used under the hood here.

BTW, I've tried to replicate the issue I think you are having with no luck...

testObjID = function (id, fields) {
   console.warn("++++++++++++++++++++++++++");
   console.log("File Added")
   console.log("id", typeof id, id, id.toHexString());
   var rec = myData.findOne(id);
   console.log("Found file: ", rec);
   console.warn("++++++++++++++++++++++++++");
}

query = myData.find({});
handle = query.observeChanges({ added: testObjID });

Prints (for example):

W20160404-14:49:23.282(-7)? (STDERR) ++++++++++++++++++++++++++
I20160404-14:49:23.283(-7)? File Added        
I20160404-14:49:23.283(-7)? id object { _str: '0169a2688d63f0756be0da38' } 0169a2688d63f0756be0da38
I20160404-14:49:23.285(-7)? Found file:  { _id: { _str: '0169a2688d63f0756be0da38' },
I20160404-14:49:23.285(-7)?   length: 14601,
I20160404-14:49:23.285(-7)?   md5: '06fd67bc25d55abc65144b93d23b0d04',
I20160404-14:49:23.285(-7)?   uploadDate: Mon Apr 04 2016 14:46:17 GMT-0700 (PDT),
I20160404-14:49:23.285(-7)?   chunkSize: 2096128,
I20160404-14:49:23.285(-7)?   filename: 'moralb_branch01.jpg',
I20160404-14:49:23.285(-7)?   metadata: { _auth: { owner: null } },
I20160404-14:49:23.286(-7)?   aliases: [],
I20160404-14:49:23.286(-7)?   contentType: 'image/jpeg' }
W20160404-14:49:23.286(-7)? (STDERR) ++++++++++++++++++++++++++
vsivsi commented 8 years ago

Just committed the above test to a branch of the file-collection sample app here: https://github.com/vsivsi/meteor-file-sample-app/tree/observe-changes-test

0o-de-lally commented 8 years ago

Thanks @vsivsi for looking at this.

So I guess the part that threw me off is this: ''' var rec = myData.findOne(id); // when I would expect findOne(_id: id) '''

Would be nice to document this. I can help.

Lastly, if the uniqueIdentifier is already created by your package using objectID, why not just have that be the default for the id, instead of having the dev define it?

vsivsi commented 8 years ago

In Meteor:

var rec = myData.findOne(id);
// and
var rec = myData.findOne({ _id: id});

are equivalent. I just tried the {_id: id } form, and it works just as well.

I don't understand what you mean by:

why not just have that be the default for the id, instead of having the dev define it?

Where exactly does the "dev have to define it"?

0o-de-lally commented 8 years ago

In that case I'm not sure where my error is coming from. I'll dig deeper.

When I have tried to omit the _id on the myData.insert() I get an upload error in the console. Is that by design or an error in my code?

Thanks again

Lucas Geiger

vsivsi commented 8 years ago

Oh yes, you need to include the _id from resumable on the insert. It's what connects the resumable upload session to the file in the server-side collection. All of this code is just a convenience to make it easier to use resumable.js with file-collection. They are different projects, and resumable.js doesn't normally come with any server back-end code, so trust me, this is saving you a lot of time and frustration relative to rolling your own upload support! This ObjectID thing is a wrinkle, but relatively minor in comparison to the issues being solved by this overall integration of Meteor, MongoDB/gridFS and resumable.js

0o-de-lally commented 8 years ago

Agreed! Lots of time saved already. I'll post here if I can get something that reproduced my id issue.

Thanks again Vaughn for maintaining such a vital and well written package!