The collections argument must be a Mongo.Collection instance or an array of them

[rclai]

When I do:

MyCollection = new Mongo.Collection('myCollection');
...
MyCollection.permit('insert').ifLoggedIn().apply();

I get:

Error: The collections argument must be a Mongo.Collection instance or an array of them
W20150218-11:27:35.768(-5)? (STDERR)     at SecurityRuleConstructor.Security.Rule.collections (packages/ongoworks:security/security-api.js:45:1)
W20150218-11:27:35.769(-5)? (STDERR)     at [object Object].Mongo.Collection.permit (packages/ongoworks:security/security-api.js:101:1)
W20150218-11:27:35.769(-5)? (STDERR)     at app/server/security/global.js:7:16
W20150218-11:27:35.769(-5)? (STDERR)     at /root/database/.meteor/local/build/programs/server/boot.js:212:5

This collection is instantiated inside a local package, with ongoworks:security as a dependency. Oddly enough when I do this following code in a server startup code:

Meteor.startup(function () {
  var test = new Mongo.Collection('wtfisgoingon');
  test.attachSchema(new SimpleSchema({
    wtf: {
      type: String
    }
  }));
  test.helpers({
    yo: function () {
      return 'waddup'
    }
  });
   test.permit('insert').ifLoggedIn().apply();
});

This works just fine.

[rclai]

Oh wait, so turns out that the issue was that my app had dburles:mongo-collection-instances installed directly via meteor add, so I meteor remove'd it and that fixed it.

Then I added dburles:mongo-collection-instances as a dependency in my package, and now everything is fine. Sheesh, packages management is such a nightmare.

[rclai]

Whoops, spoke too soon.

So this code:

// dburles:collection-instances api call.. I'm trying attach this security rule to all collections
_.each(Mongo.Collection.getAll(), function (collection) {
   collection.instance.permit(['insert', 'update', 'remove']).ifLoggedIn().apply();
});

Still gives me the same error.

What's really interesting is that in the package dependencies if I put ongoworks security before collection instances, I get that error, but if I put ongoworks security after collection instances I get an error saying that the permit method is not defined.

How should this be reconciled? Should I get @dburles in here?

[rclai]

So I cloned your repo and included it locally and debugged it.

https://github.com/ongoworks/meteor-security/blob/master/security-api.js#L42-L46

The collection instance does go through (I console.log'd what collections was) but for some reason the instanceof check returns false for some reason. Does that have something to do with what collection-instances does to the Mongo.Collection object? I also tried doing

if (collections instanceof Mongo.Collection || collections instanceof Meteor.Collection) {

to see if would work but that didn't work.

https://github.com/dburles/mongo-collection-instances/blob/master/mongo-instances.js

I'm not too JS ninja enough to know. What is it that caused the collection instance to "lose" being an instanceof Mongo.Collection?

[aldeed]

Yeah, there are a few slightly different ways to override constructor/prototype, and some of them result in it no longer passing the instanceof check. I consider this to be an issue with the packages that are doing the overriding.

When I used to override the prototype in collection2 package, I used code like this:

var constructor = Meteor.Collection;
Meteor.Collection = function {}; // etc...
Meteor.Collection.prototype = constructor.prototype;

for (var prop in constructor) {
  if (constructor.hasOwnProperty(prop)) {
    Meteor.Collection[prop] = constructor[prop];
  }
}

// backwards compatibility
if (typeof Mongo === "undefined") {
  Mongo = {};
  Mongo.Collection = Meteor.Collection;
}

Maybe something like that would work.

Should add a test to mongo-collection-instances tests that verifies the instanceof check passes.

[rclai]

I don't know if this is a good idea, but do you think that there should be some kind of "official", super-compatible Mongo.Collection monkey-patching package, where you can do:

Mongo.wrapCollection = function (customFunction) {
    var constructor = Meteor.Collection;
    Meteor.Collection = function () {
             var ret  = constructor.apply(this, arguments);
             customFunction.apply(this, arguments);
             return ret;
        };
    Meteor.Collection.prototype = constructor.prototype;

    for (var prop in constructor) {
      if (constructor.hasOwnProperty(prop)) {
        Meteor.Collection[prop] = constructor[prop];
      }
    }

    // backwards compatibility
    if (typeof Mongo === "undefined") {
      Mongo = {};
      Mongo.Collection = Meteor.Collection;
    }
}

That way a bunch of these major packages (collection-hooks, collection-instances, collectionFS, etc) that monkey-patch Mongo.Collection can use it to inject their functionality and play nice with each other once and for all?

[aldeed]

Might be useful if you or someone wants to create it and wrangle the various package authors to use it. I doubt MDG would add that to core anytime soon.

I'm not positive that's even the best way to code it, but it worked for me for awhile. The Meteor/Mongo backwards compatibility probably complicates it, too. I don't think any of my packages override any constructors anymore.

[dburles]

@rclai the util package sounds like a great idea

[rclai]

Okay I'll see what I can do. I'll fork the major packages and the refactor them to use the utility function, run their tests and see what happens when used all together.

[rclai]

Just an update. This is what I've worked on so far. It's not complete yet. Right now the package has beef with some other packages right now, so don't use it, just wanted some early feedback as to the API. Here's a Hackpad for it.

We can take this discussion over at the Hackpad or in my repo so that it doesn't make constant noise here.

[rclai]

Here's the pull request.

Here's my package.

Feedback is welcome.

[dburles]

Great work @rclai how did the tests go?

[rclai]

They were green for everything. The test that should cover this issue also got fixed as a result.

Also, that pull request is for you @dburles, did you test it?

Also, I'm still writing more tests to be 200% sure.

[dburles]

Hope to have a a look later tonight

[aldeed]

@rclai is the original issue here fixed now? At least from the standpoint of this package?

[rclai]

Yes it is.