-
Notifications
You must be signed in to change notification settings - Fork 21
The collections argument must be a Mongo.Collection instance or an array of them #8
Comments
Oh wait, so turns out that the issue was that my app had Then I added |
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? |
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 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 |
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. |
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 ( |
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. |
@rclai the util package sounds like a great idea |
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. |
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. |
Here's my package. Feedback is welcome. |
Great work @rclai how did the tests go? |
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. |
Hope to have a a look later tonight |
@rclai is the original issue here fixed now? At least from the standpoint of this package? |
Yes it is. |
When I do:
I get:
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:This works just fine.
The text was updated successfully, but these errors were encountered: