Meteor-Community-Packages / meteor-roles

Authorization package for Meteor, compatible with built-in accounts packages
http://meteor-community-packages.github.io/meteor-roles/
MIT License
920 stars 164 forks source link

Pass in userIds to narrow down result #305

Closed CyberCyclone closed 4 years ago

CyberCyclone commented 4 years ago

I've got an issue where there might be a million+ users for a particular role / scope and obviously it's not practical to pull the complete list of users into an array to then filter the result.

Specially, I want to search for a user who's firstName is "Amy", and from the results remove all the users who aren't part of that role.

The current way "meteor-roles" works as far as I can tell is that you can't do any of your own queries or narrow the result down. Currently it's pulling all the users that are a part of that role of which I'd then need to pluck the ids, and then pass that into the query to find the persons who's name matches.

This could pull back millions of users when using "getUsersInRole" based on a particular role.

The easiest way to make this work is to do the query first on the "users" collection to get the ids matching the "firstName", and then pass that array into "getUsersInRole" to then see if the supplied userIds belong to that role.

My thinking is to add this option into "_getUsersInRoleCursor":

if(options.userIds){
      selector['user._id'] = {$in: options.userIds}
}

For my scenario this works well and only pulls the docs that are actually needed / used.

SimonSimCity commented 4 years ago

To be quite honest ... in this case I would also think about extendability. How far would this solution scale? Would it help others or just something that is rather quite unique to my case.

I had a similar case, where I was wondering about if my use-case could be something that would help others as well. In the end I decided that it's quite unique:

(https://github.com/Meteor-Community-Packages/meteor-roles/issues/270#issue-390578779)

Search for the same role on multiple resources. Think of it as if the write-permission on a folder would grant you access to its files. The files are too many and tend to change rapidly. Some users now get permission on a per-file-basis, some for the folder, some for the full hard-drive.

What I ended up with is a small wrapper-plugin around this package which has a fixed major-version dependency. In this way you can be assured that you will get notified if something in the database structure changes, and you'll not be able to update until you allow it in the dependencies.

Unless I get someone else to vote for this feature, I wouldn't take it in, but rather ask you to take the approach of a wrapper-package, where you can write the query in the most performant way for your use-case.

Leaving it open for feedback and upvotes.

CyberCyclone commented 4 years ago

Thanks for the feedback / suggestion. I guess the main case for adding it to this package would be, do you see users / Meteor being something that would attract thousands / millions of users.

In that scenario, using "_getUsersInRoleCursor" would not scale well and make this package not really usable for high volume scenarios.

For example, if there was a million users, and you returned the millions of users with "_getUsersInRoleCursor", what if all those million users used the app at the same time? That would be 1 trillion records you've just pulled down when it could be easily avoided.

SimonSimCity commented 4 years ago

Well, this option starts with an underscore, which makes it kind-of internal by convention, I guess - but referring to the function getUserAssignmentsForRole(), you should use it when you need it.

The documentation of the function clearly states:

Retrieve all assignments of a user which are for the target role.

If you have too much data, you can use the query-option parameter to limit the amount, but you then clearly want all the assignments users have on a specific role ... otherwise you wouldn't use this function, would you ..?

So depending on your data-set you either want or don't want the list. Or am I taking this from the wrong perspective?

CyberCyclone commented 4 years ago

The initial function is getUsersInRole.

I guess where I'm coming from is, the package only allows you to pull data for one user, or pull data for all users instead. Ideally there should be the option to only pull data for the users that you need it for.

Having to verify 1 million+ users are in a role when you only need to verify 50+ doesn't seem logical, even if you limit the data in the query options.

SimonSimCity commented 4 years ago

The package is currently only meant for listing all roles of a user or all users of a role, correct.

As said, you always have the option to extend this package by a wrapper package. I also have a lot of custom queries in my project, which I set up as a wrapper to this package. This way, I can write queries as most effective for my project while still retain the version requirement.

Your example looks like you want to do what SQL calls a join - in which case it might be even better to call this per user. This will help you in reducing the load - provided you're using redis-oplog and custom channels - and I guess you're using it, in regards to the amount of data you're talking about.

Closing this issue. If you think this is definitively a need, please provide a use-case where it wouldn't be used as a join.

CyberCyclone commented 4 years ago

Yep, all good. I wasn't sure if it was in scope for the package, but for us it's much easier forking it and adding the 1 line of code.

Just for context in our use case, we're using it inside a Meteor Method for searching for users, not a subscription.

E.g, we need to find a user by their firstName or lastName (plus whatever other data). Using an Aggregate would be my go-to method, however that would mean hardcoding the logic into the app and would be outside this plugin, which is bad from a plugin update perspective.

Since the top level function Roles.getUsersInRole already does what I want, I just want it to search on less users.

The only practical way using this plugin without tampering with it as suggested above, would be to pull all 50 million user documents using Roles.getUsersInRole and with that result, loop through and check the ones that match the criteria. An extremely unoptimised way to do it.

So for us, while the package is fantastic and helps a lot, for our case we can't use it at all without making this change, which works perfectly and it's the cleanest way I can think of to do it (since it's 1 line of code).

SimonSimCity commented 4 years ago

Well, instead of forking the package, you can also write an extension to it. Just create a small package, which only lives in your code. Just need to create a file package.js within packages/my-package of your project, have a dependency on alanning:roles@3.0.0, add a file and extend the Roles object. You could even overwrite existing methods, even though I only would advise it if you extend the code - not replace it.

This link might help you to kick it off: https://guide.meteor.com/writing-atmosphere-packages.html#local-packages

If I find some time (I think this will first be after the 4.x) I'll write a tutorial of how to extend this package.