Skip to content
This repository has been archived by the owner on Dec 24, 2021. It is now read-only.

hasPermission() & inhibitor to allow asynchronous code to be run inside #34

Closed
mindaugaskasp opened this issue May 3, 2017 · 4 comments

Comments

@mindaugaskasp
Copy link

mindaugaskasp commented May 3, 2017

It would be really great if inhibitor function and/or hasPermission() function would be able to deal with asynchronous code inside. It seems from the source point of view rewritting hasPermission() to deal with async would take a bit of work and might be prone to bugs (hell, if no one wants to do it, I can) so I propose an alternative function HasPermissionAsync() or whatever you guys like more. It would require just additional, perhaps, redundant if check in discord.js-commando/src/commands/message.js #134

if(!this.command.hasPermission(this)) {
	this.client.emit('commandBlocked', this, 'permission');
	return this.reply(`You do not have permission to use the \`${this.command.name}\` command.`);
}

There could also be an additional check, let's say..

if(! await this.command.hasPermissionAsync(this)) {
	this.client.emit('commandBlocked', this, 'permission');
	return this.reply(`You do not have permission to use the \`${this.command.name}\` command.`);
}

This doesn't seem to cause any issues since run() method is async itself. It might be redundant, but gives flexibility for users who want async code to be run in hasPermission() and those who don't. The same goes for inhibitor functions, to be honest, I can't get the sense why they are not async to begin with.

Now, pardon my walls of text, the reason I am proposing this is that I am developing myself a commando based bot and my application is quite restrictive in terms of who can do what and when. Meaning there has to be a permission / role check for every command user requests. All that translates to various services calling to database and retrieving relevant information and forming a response whether that user can, at this point of time, use desired command. Back to the point, all those services that connect to database are asynchronous, because database calls by nature are like that in Node and that makes things complicated when commando does not handle async code. This issue I am having could've been solved either by inhibitor and/or hasPermission method call if they handled async code.

I could, of course, just do redundant and nightmare to maintain checks in every command I develop, but honestly, the thought alone, makes me regret to porting my code to commando. My code previously was mostly plain discord.js with npm clapp package integration. Good parts, it was very flexible, but became a nightmare to maintain in the long run due to lack of solid structure. Commando has that, but lacks flexibility part. I am hoping for a decent middle ground between the two.

@Gawdl3y
Copy link
Member

Gawdl3y commented Oct 21, 2017

hasPermission not supporting async code is completely by design, to discourage behaviour that would have terrible performance. If you were to do database queries or anything similar in there, you'd be regretting it. A simple use of the help command runs hasPermission for every single command that is registered, for example. Anything permission-related should ideally be cached in-memory.

@mindaugaskasp
Copy link
Author

mindaugaskasp commented Oct 26, 2017

That's a good point, I haven't been aware of this method being called somewhere else. However, I am sure you are aware that permission checks are usually more complex requiring various query calls etc. So the only way is to either cache that data somewhere and process it in hasPermissions() method or just make completely alternative method, wrapper class - to do your own thing there. Still, would be a nice addition to have some sort of async permission layer out of the box set up that wouldn't go against the commando conventions.

@vintprox
Copy link

I have use case where I have to asynchronously get data in Inhibitor.

More specifically, it's Keyv storage operation: await store.get(channel.id, false). I use it to determine whether the bot was assigned to current channel. Do I have to cache channels list separately from Keyv storage then?

@vintprox
Copy link

vintprox commented Aug 30, 2019

OK, I temporarily solve my case by introducing guards module that provides function for returning Message when channel is not whitelisted.

I import this guard function in each command module tightly restricted to whitelisted channels and check whether to return that warning Message or to continue.

Gist

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants