Sortable

[Digital-Thor]

Hi Vaughn. Do you have any thoughts on how to enable manual ordering of files in your MFC sample app? I like the idea of using rubaxa:sortable since it can reorder the actual collection using either drag-and-drop or touch. However, on first perusal, it seems that Sortable replaces the {{ #each dataEntries }} helper approach with {{ #sortable items = mongoCollectionName ... }} and I didn't have much luck as I tried to integrate them.

[vsivsi]

Sortable is pretty neat. file-collections on the client are just Minimongo collections like any other Mongo collection, so code that works for a vanilla Meteor/Mongo collection should also work for a file-collection. Do you have a repo with your attempt to make this work? If so I'd be happy to take a look...

[Digital-Thor]

Thanks. Yes, Sortable does seem very powerful and according to Atmosphere, is the only Meteor ordering package that includes touch. My code is on a private dev path now, but I'll fork the MFC Sample App, should it eventually be of use to others. I hope to have something to look at by the end of the week or so.

[Digital-Thor]

I'm still learning Meteor, Meteor File Collection, and Sortable, but here's my work-in-progress for Adding Sortable to Meteor File Collection. I've included an overview of my integration approach and progress within the readme.md file.

[vsivsi]

Hi, I've taken a look at this and have figured out why it's failing...

The bottom line is that packages that monkey-patch and shadow the Meteor provided global Mongo.Collection are semi-evil at best.

So what is happening here is a good old-fashioned initialization problem.

File-collection extends the Mongo.Collection object as a coffeescript-style subclass. It adds functionality to it without modifying or replacing the original super-class (Mongo.Collection).

Other packages take a different (IMO pretty evil) approach and replace the global Mongo.Collection itself. I noticed from one of your comments that you discovered package load-ordering matters in such cases, because if you load file-collection first, it won't get the soon to be monkey-patched version of Mongo.Collection to extend; and then later, when you try to use it, it detects that it is not an instanceof the current (patched) definition of Mongo.Collection, and it throws an Error to save you from hours of tedious and pointless debugging.

What you've run into with Sortable is yet another initialization dependency issue... Sortable replaces Mongo.Collection indirectly through its use of the dburles:mongo-collection-instances package. That package uses the lai:[email protected] package to install itself, and that package overwrites the global Mongo.Collection object. However, when it does so, it doesn't also update Mongo.Collection.prototype.constructor and this is the value that Coffeescript uses as super when you override the constructor in a new subclass. Still following?

So when you use Sortable (or any other package that shadows Mongo.Collection using the approach of lai:collection-extensions) you end up with Mongo.Collection !== Mongo.Collection.prototype.constructor which breaks any code that properly uses Javascript prototype inheritance, including any Coffeescript subclasses.

So, you can monkey patch the monkey patch by including:

Mongo.Collection.prototype.constructor = Mongo.Collection

before initializing file-collection. Sadly, that just leads to the next issue:

Error: File Collections do not support 'update' on client, use method calls instead

Which I'm afraid is a dead-end. This is a security issue. It is completely unsafe to allow untrusted clients to update gridFS file records (which could cause private data leakage or gridFS database corruption if improperly updated). The only way to do this is using Meteor method calls that implement specific updates on behalf of users. And unfortunately, it doesn't look like Sortable is currently flexible enough to allow you to write your own order update method.

It sucks that this is such a nightmare, I really REALLY dislike that it has become a seemingly widespread convention for packages to overwrite standard Meteor system calls. I do hope that MDG provides a more sane mechanism to add such extensions in the future so we can unwind some of the madness.

[vsivsi]

A bit more poking around lead me to discover that Sortable does use a custom method call to update the collection server-side (because Meteor restricts client update selectors to a single document by _id only, again for security reasons).

However, the client-side stub for this method attempts to do a local update. If you could eliminate the client-side method stub (or at least this line: https://github.com/RubaXa/Sortable/blob/master/meteor/methods-client.js#L14) then it will probably work, but without the stub, you will incur the round trip latency.

[Digital-Thor]

Thanks for looking into this Vaughn and also notifying Richard.

I tried the (monkey patch)^2 approach and of course hit the next error as you did. Recently, I encountered the security issue that you mentioned with my own MFC app and wrote a Meteor Method as the error instructed me, but this time, I guess I would have to fork Rubaxa:Sortable and then have to test/maintain future updates on my fork. Also, who knows when the chain of errors would really end. Probably asking for multiple headaches if I go that route.

Yes, it's a shame that some packages don't know about (or ignore) the standards since part of the whole dream of Open Source (IMO) is to empower creativity through interoperability. Which is especially important to those of us who hope to develop rapidly at a modular level.

Back to the drawing board...

[vsivsi]

I think a lot of this is growing pains for Meteor. There is a lot of work being done right now to hack preexisting solutions (like Sortable) into Meteor. This is seductive because these external code bases are popular and mature, but because Meteor is quite different from what came before, it inevitably requires a fair amount of monkey business in the integration package to get things to work.

I'm hopeful that as time goes on we'll see more Meteor specific packages that deliver great features while taking Meteor's strengths and weaknesses into account in their design rather than hacking and patching to get some existing thing to kind of mostly work. A dev can dream...

[Digital-Thor]

Agreed. I give full credit to all who contribute to Open Source especially since along with the benefits of Meteor, its recent arrival has a lot of us scratching our heads at times regarding best practices. It will be an iterative process to complete the foundation.

[vsivsi]

I have an idea that may make this possible...

Rereading this issue: https://github.com/meteor/meteor/issues/3271

I think it may be possible / desirable to enable a "local-only update" on the client for file-collection. That would enable latency compensation when using a secure Meteor method to update the server-side gridFS collection.

So it would work something like: You can write a method to update the file-collection, and the client-side stub can perform an update (it won't throw like it currently does). However, that client update can never propagate to the server directly, it will only affect the client minimongo cache of the server-side. So you can do whatever you want on the client for UI or whatever, but if you want persistence, then you need to code it into an explicit method call.

I think it could be added as fc.localUpdate() on the client only, and then if you're dealing with a package like sortable, you can alias fc.update = fc.localUpdate

My main concern is in ensuring that it gets properly used and doesn't become a support headache.

Still thinking about that.

[rclai]

Hey guys, can you guys see if this PR will fix this issue (obviously not in the long run)?

[vsivsi]

@Digital-Thor FYI, I just implemented the above idea of fc.localUpdate() on this file-collection branch: https://github.com/vsivsi/meteor-file-collection/tree/local_update

I'm testing now to see if I can use that to make Sortable work.

[vsivsi]

Okay, so I've gotten really close with this. I can now rearrange the file list, and both client and server-side methods are being called with no update errors. The final remaining obstacle is that the Sortable Meteor package doesn't fully support having the order field not be at the first level of the file document.

This is a problem because gridFS file documents have a defined schema and so it's not great to just add top level fields to those file documents. That is what the metadata sub document is for, and file-collection will reject updates that attempt to modify anything other than filename, contentType, aliases and metadata.

The Mongo.update calls in Sortable are actually fine, the problem is actually in the first and last lines here: https://github.com/RubaXa/Sortable/blob/851129a45fbff351d95c23d922436f2660880744/meteor/reactivize.js#L132-L136

If orderField is anything other than a simple fieldname, those lookups (e.g. Blaze.getData(itemEl.nextElementSibling)["metadata.sortable.order"]) fail and return undefined, which ultimately clears the numeric order values (via successful updates!), replacing them with undefined. Bork.

[Digital-Thor]

Wow! Lots of progress. I was concerned about the issue with Sortable's order index using a different level than permitted by GridFS, but was hoping that once the other problems cleared, Sortable would handle a deeper object field. It sounds like you've now confirmed that issue is the show-stopper. You probably noticed that I posted an issue on Rubaxa:Sortable. Hopefully, he'll have a chance to review our dialog.

[vsivsi]

I've forked your repo and added my changes: https://github.com/vsivsi/Adding-Sortable-to-Meteor-File-Collection

To make this work, you need to use the local_update branch of file-collection, which you probably don't know how to do:

# Run from within the root of the project
mkdir packages
cd packages
git clone --recursive https://github.com/vsivsi/meteor-file-collection.git
cd meteor-file-collection
git checkout local_update
cd ../..

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

vsivsi:[email protected]

Running meteor will now use the local checkout in the packages directory because version 1.2.0 doesn't exist on Atmosphere.

[vsivsi]

If you do the above, don't be alarmed when you see this:

###############################################################
## WARNING from vsivsi:defender package:
## Meteor function: Meteor.Collection has been modified!
###############################################################

This whole monkey-patching business has me so steamed that I channeled my frustrations into this new package: https://atmospherejs.com/vsivsi/defender

[vsivsi]

Okay, a bit more work on this. It turns out that adding new fields to the gridFS file documents isn't so bad. MongoDB now even says it's okay:

Documents in the files collection contain some or all of the following fields. Applications may create additional arbitrary fields

So, I tried it out... And hit the next issue: This one will definitely require a change in the Sortable Meteor support here: https://github.com/RubaXa/Sortable/blob/master/meteor/methods-server.js#L24

Sortable is assuming in this check that Mongo _ids are strings. However, while Mongo _ids can be strings, they can also be objects of type Mongo.ObjectID And in fact, gridFS specifies: "_id" : <ObjectId> and so file-collection uses Mongo-style object IDs to conform with this. Meteor explicitly supports both types: http://docs.meteor.com/#/full/mongo_collection

So to handle both cases this line should be:

 check(ids, [ Match.OneOf(String, Mongo.ObjectID) ]);

[vsivsi]

Just created a PR on Sortable to fix the above: https://github.com/RubaXa/Sortable/pull/477

Gawd I hope this is the last thing...

[Digital-Thor]

What a headache! But, this is really coming together.

Looks like Richard is going to chase down his issues with dependent packages and, along with your PR, hopefully a lot of important packages will soon be much better.

I was noticing that I could modify top level GridFS fields, but figured that I was doing something wrong. I've submitted a change suggestion on Mongo's GridFS schema page to include a note within the the metadata description that it's not the only place for arbitrary data. Also, I like your solution to the _id problem. I've encountered that in the past and have always parsed depending on the implementation, but thanks for a more flexible solution!

At the risk of being optimistic that this is wrapping up, your fork (of my fork of the Sample App) should probably stay in your vsivsi repo somewhere; I'm sure a lot more people will benefit if Rubaxa:Sortable becomes compatible with Meteor File Collection. I'll be glad to put a redirect note in my repo's readme.md to wherever you decide to put it.

[vsivsi]

No action today on the above PR. Unfortunately the way the Sortable repo is structured, with the Meteor package implemented in a subdirectory, just using the package in development mode doesn't look totally straightforward. https://github.com/RubaXa/Sortable/blob/master/meteor/publish.sh#L16

So I'm going to hold off working with the fork in the hopes that the devs over there will act on the PR sooner rather than later. If nothing has happened in a few days then I will dive in further.

[Digital-Thor]

Makes sense and thanks for the continued timely support. No major rush from my perspective. Ordering is critical over the long term to my use case, but in the short term, my alpha users can manually number the order index.

Also, I've added a comment to my recent Sortable issue that includes a link to your PR.

[vsivsi]

FYI, I just released v1.2.0 of vsivsi:file-collection on Atmosphere. It merges in the changes that were on the local_update branch. So as soon as Sortable acts on the PR I submitted above and releases a new version to Atmosphere you should be all set.

[majorgilles]

I was having the same issue with another package: http://mongol.meteor.com

[alexeden]

I experienced the described problem and was led here by the link in the error message. I'm entirely naive of the technical details, but the problem was solved by removing the matb33:collection-hooks package. So, go ahead and add that to the list of offending, pretty evil packages.

[vsivsi]

@AlexanderEden91 Yup, there's even an open PR on collection-hooks to fix their bug that is causing this issue... I'm officially finished with packages that monkey patch Meteor. So much so that I wrote a package (with attached manifesto) that will alert you to all such monkey business occurring in any packages you may use: https://github.com/vsivsi/meteor-defender

[alexeden]

Installed and starred on Atmosphere. Thanks for your work!

[MisterQH]

Hi! I tried Mongo.Collection.prototype.constructor = Mongo.Collection, but it didn't fix anything... is there a way to solve this issue ?