Customized slash command registration

[lieut-data]

A common pattern seen in plugins is to hook into ExecuteCommand, split args.Command by fields, and check the first field to match against the desired slash command. There’s a lot of power in the command abstraction, but some of it is overkill for the typical slash command interaction.

Let’s extend the plugin helper API (not the RPC API) by adding a RegisterSlashCommand helper method to the github.com/mattermost/mattermost-server/plugin package (see Helpers and HelpersImpl) that accepts the usual model.Command along with a callback:

func (p *HelpersImpl) RegisterSlashCommand(command *model.Command, callback func(slashCommandArgs *plugin.SlashCommandArgs, originalArgs *model.CommandArgs)) error

The plugin would use RegisterSlashCommand in OnActivate (or anywhere else) and manually proxy the ExecuteCommand to the helpers:

func (p *Plugin) ExecuteCommand(c *Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) {
    return p.Helpers.ExecuteCommand(c, args)
}

The registered callback function should be executed whenever the helpers determined that the trigger in question matched. The originalArgs would expose all the original data, but the callback is likely to just use the new slashCommandArgs that the helpers fill in automatically:

type SlashCommandArgs {
    Trigger string
    Args    []string
}

This is a modest improvement, but for bonus points, it would be incredible to integrate https://github.com/spf13/cobra into this equation. The plugin would register commands via cobra as usual, and the helpers would proxy the data through cobra’s SetArgs and Execute APIs to trigger the command. Perhaps a RegisterCobraSlashCommand for this purpose. Challenges here is Cobra’s single-threaded assumption and the potential race between a call to SetArgs and the Execute, plus the need to proxy context and potentially return a CommandResponse.

Example of the current pattern: https://github.com/mattermost/mattermost-plugin-github/blob/092af14724e60178ebf16393bf177d9f7dc9ad82/server/command.go#L56 that this helper seeks to improve.

JIRA Ticket: https://mattermost.atlassian.net/browse/MM-16801

---

If you're interested in this ticket, please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

[ethervoid]

Hi, I'd like to grab this one :)

[ethervoid]

Hi @lieut-data! , I'm currently working on this issue and I've been reading about plugins and so on to know how is it done, etc. I've also read your proposal and I have a couple of questions about the ticket:

• I've seen there is a RegisterCommand method and the proposal adds a RegisterSlashCommand to the Helper. I thought a command was always a slash command. What is the difference between a command and a slash command?
• What is the purpose to redirect the ExecuteCommand call to a new method in the Helpers called ExecuteCommand?
• From your proposal, I'm inferring that the improvement is to automatically parse the arguments of the command call instead of the plugin creators have to do it manually by themselves. Am I right?

I'm trying to understand the whole picture and the issue before I start coding something. Thank you very much

[lieut-data]

Thanks for tackling the effort, @ethervoid! To answer your questions in order:

• You're right, this method could just be called RegisterCommand. The term Command never quite spoke to me as being equivalent to a SlashCommand, but it's probably better to be consistent.
• The server will invoke the plugin's implementation of ExecuteCommand via the RPC API. In order for the helpers to be able to help, they need to handle this ExecuteCommand for themselves. If you can think of another way to do this, let me know!
• Yes, this is exactly the problem. For example, take a look at https://github.com/mattermost/mattermost-plugin-github/blob/2b6d6f2ad3d503bc729a726f5f91eb494690b9f4/server/command.go#L56 to see how the GitHub plugin does it. Pretty much every implementation of ExecuteCommand has all of this boilerplate.

[ethervoid]

Thank you @lieut-data for your answers :) I have more questions:

I've been thinking about the implementation so the idea is the following:

1. The plugin' creator uses p.Helper.RegisterCommand(command, func(..))
2. The plugin' user types a command and the p.Helper.ExecuteCommand is called
3. That method extracts the args and parses it into the SlashCommandArgs
4. After that, the callback is called inside the p.Helper.ExecuteCommand method

I have two questions about this workflow if it's correct what I'm putting above:

First question

I've been thinking about how to store the callbacks functions.

I've seen there is a KV object for plugins but it stores byte slices so I can't use it to store the callbacks. I've seen how the commands are stored in a PluginCommands slice in the server but this is different.

Is there any other data structure that stores plugin objects?

Second question

My second question is about the callback execution.

I've added to the p.Helper.ExecuteCommand method the *Plugin pointer as an argument because the plugin' creator would want to call methods inside his/her code, for example this one, and is going to need the plugin object to do it.

But even passing the plugin object I think is not possible because the non-exported methods (lowercase ones if I'm not mistaken) are going to be out of the package scope and the helper is not going to be able to execute them.

Am I right? Are you thinking on other approach that I'm not seeing?

Thank you very much :)

[lieut-data]

Hey @ethervoid!

With regards to your first question, it’s not possible to store a callback in the KV store — nor is it necessary. The helper code is part of the compiled binary, and just can just allocate, say, a map, to keep track of all the callbacks. It won’t conflict with other plugins. Consider using sync.Map to ensure safe access to the underlying structure if a plugin registers commands concurrently.

With regards to your second question, if you just invoke the callback given by the plugin, it will already contain the receive variable that is the *Plugin. That is, unlike other languages, there’s no need to “restore the context” — calling the callback is equivalent to calling the method on the original instance itself.

Hope that helps!

[lieut-data]

@RajatVaryani has kindly agreed to tackle this issue. Can't assign him in GitHub right now for some reason.

[RajatVaryani]

@lieut-data Now you can.

[RajatVaryani]

@lieut-data Even before staring working on this issue I would like to understand the overall vision for slash commands w.r.t plugins. In the PR raised there were comments raised wanting a full interface like cobra and cobra has lot of features which might or might not be useful. I want to look at the problem from a macroscopic point of view and understand the usecase we are trying to cater to. The reason I am asking is because using cobra for a usecase like ours is going to come with its own set of caveats like proxying data like plugin context and returning CommandResponse which is not straightforward.

[lieut-data]

@RajatVaryani, the primary goal here is to make it much easier for plugin authors to define and implement slash commands. Today, that requires manually parsing the incoming string and rebuilding a lot of functionality that already comes for free with Cobra, e.g. subcommands, positional arguments, and named arguments. You can see how things are done today in other plugins, but ideally we'd set a pattern that would simplify for everyone. I think it's fine to restrict the scope if there are cobra-specific features that wouldn't make sense for a plugin.

[RajatVaryani]

@lieut-data Thank you. Here is an initial design. Please take note of the flaws in current design.

• Not being able to pass CommandResponse depending on the subcommand. I could not find any workaround to this. Can plugin authors do the same thing using API which can be done using CommandResponse?
• Keeping model.CommandArgs and plugin.Context in the Plugin global context. We will have to use concurrency primitives to make them thread safe.

Here is a code snippet: https://gist.github.com/RajatVaryani/a8121661a112b6fc41d5ea13975e724f

Thoughts?

[lieut-data]

Thanks for sharing the code snipper and design, @RajatVaryani! I'll take a closer look and get you feedback.

[RajatVaryani]

@lieut-data A gentle reminder.

[lieut-data]

Thanks, @RajatVaryani!

A few thoughts:

• ExecuteCommand currently sets p.Args and p.Context before calling p.Helpers.ExecuteCommand. Despite the lock acquired inside ExecuteCommand, this still isn't thread safe, since a concurrent slash command execution might overwrite p.Args and p.Context before the lock can be acquired. Should we just pass these as parameters instead?
• If we assume that serializing all slash commands is still a viable approach (given the lock above), then I think we can still build a mechanism to reply with a CommandResponse. Could we store the command response on the plugin object and refer to it in the ExecuteCommand helper?
• There's an interesting pull request over at Add support for context.Context spf13/cobra#893 that purports to add context to the cobra tooling that might simplify all/some of this.
• The discussion at Execute same command concurrently spf13/cobra#908 (comment) is interesting as well, since it suggests that cobra may be fundamentally unthreadsafe beyond even the obvious issues. The idea of building a separate binary is intriguing, but I don't know if it would help us or not?

[RajatVaryani]

@lieut-data Thank you for the nudge. I re-looked my approach. I came to the conclusion If we are able to proxy data (arguments) in the cobra command we would find an effective solution. In order to do that I have embedded a commandContext struct in HelpersImpl. Using this we will be able to proxy data in and out of cobra commands hiding complexity from the plugin authors.

Along with it I also had to expose additional helpers methods to get and set the data required in cobra commands. Please note this code snippet only supports a single root command (which will be changed while implementing it) and is thread safe.

Code snippet: link

[RajatVaryani]

@lieut-data A gentle reminder.

[lieut-data]

@RajatVaryani, thanks again for meeting the entire Toolkit team today and presenting your proposal. Key takeaways from that discussion:

• The forced serialization of command handlers is likely to be showstopper. There are two reasons to do this, one has to do with access to the context/return parameters, and the other is a potential limitation in the cobra stack. Could you experiment with the proposal to isolate the context/return parameters in sync.Map, keyed by a unique id that is passed through the cobra arguments somehow and automatically extracted "on the other side"? Presumably, we can then unit test the behaviour of cobra under multiple, concurrent slash command processings.
• One idea was to eliminate CommandResponse() by just requiring the command handlers to return this value directly, but I think that's a limitation of cobra's Run expectations, so I think it's fine to leave as-is for this proof-of-concept.
• Would it be possible to expand on the proof-of-concept to illustrate how a plugin could extract positional and named arguments?

Thanks for driving this improvement!

[RajatVaryani]

@lieut-data Thank you for the opportunity. 😄 I second the team's opinion about serialization being a showstopper. Let me try to find a workaround using your idea. I will also add extraction of positional and named arguments in the proof of concept.

[RajatVaryani]

@lieut-data Just an update. I did not get chance to work on this. I will be working in this week.

[RajatVaryani]

Hi @lieut-data ,

I have made the changes. To avoid serialization I am making use of sync.Map. The key in this map is the request-id from the Context with the assumption that it is safe to reuse. The request-id is passed as a command flag instead of command argument. Passing id as argument is prone to error as it will change the number of arguments of the original command. I also added another subcommand unsubscribe which extracts the argument and also has check for number of arguments. The design still has flaws like we cannot use utility method as cobra.MinimumNArgs() from cobra but write our own functions to handle them.

Code snippet: link

Also I doubt using sync.Map over normal Map as our usecase does not satisfy. 0/5

Thank you.

[lieut-data]

Thanks for exploring this space, @RajatVaryani! I'll definitely take a look :)