Improve Options Handling to Reduce Security Errors

[switchupcb]

Examples are a reflection of the decisions made during the API's creation. Improving the examples allows us to improve the API by identifying issues with assumptions we've made on how the API is used. An additional benefit to better examples is less need for support, and better code throughout the Discord Go scene.

The provided examples assume that the user enters options in the order we define them. Unfortunately, this is not the case which results in multiple errors. This demonstration uses the "options" handler in slash commands example to demonstrate this issue. However, this coding pattern (among others) reoccurs throughout the provided examples.

Fix

There are a few issues we can solve by refactoring the examples:

1. Don't assume the best-case. In this specific example, multiple if len(Options) > n { parse expectedOption } statements are used. This causes an error when the user inevitably includes an option in position 4 (that is supposed to be in position 7).
2. Removing unnecessary panics. Similar to reason 1, we may want to encourage an alternate way to handle issues with a command that doesn't crash the bot.
3. Better ways to handle options. Options are stored as a slice. This works when a single option is required. Less so when it's optional or there is more then one (of the same type). There are ways around this (using len, then looping, etc), but it seems more fitting to use a map[option name][option value] that stores the value. This makes it easy to check when an option exists, still allows use of len(), and provides an easier way to handle them.
4. We can't always assume that Discord provides validation on the client-side.

Explanation

An error occurs because the value at Options[0] is not a string.

Defining Options

{
Name:        "options",
Description: "Command for demonstrating options",
Options: []*discordgo.ApplicationCommandOption{

    {
	    Type:        discordgo.ApplicationCommandOptionString,
	    Name:        "string-option",
	    Description: "String option",
	    Required:    true,
    },
    {
	    Type:        discordgo.ApplicationCommandOptionNumber,
	    Name:        "number-option",
	    Description: "Float option",
	    MaxValue:    10.1,
	    Required:    true,
    },
    {
	    Type:        discordgo.ApplicationCommandOptionBoolean,
	    Name:        "bool-option",
	    Description: "Boolean option",
	    Required:    true,
    },
...

Handling

margs := []interface{}{
    i.ApplicationCommandData().Options[0].StringValue(),
    i.ApplicationCommandData().Options[1].IntValue(),
    i.ApplicationCommandData().Options[2].FloatValue(),
    i.ApplicationCommandData().Options[3].BoolValue(),
}

Input

[switchupcb]

Example of the API using Map for Options:

// Finding the value.
optionMap := i.ApplicationCommandData().Options
if val, ok := optionMap["string-option"]; ok {
    val.StringValue()...
} else {
    // Value doesn't exist.
}

// Checking if the value doesn't exist.
if _, ok := optionMap[""number-option"]; !ok {
    // Value doesn't exist.
}

// Another way to check that the value doesn't exist.
if optionMap["bool-option"] == nil {
    // Value doesn't exist.
}

// Checking the length of the map.
totalOptions := len(optionMap)

// Iteration
for i, v := range optionMap {
    // i = index v = value
}

// Still able to use make() for optimization, etc.

The alternative (which uses slices; current API) requires you to use an excessive amount of switch-cases or conditionals or map the options in order to avoid the error described in #1126 (comment).

[switchupcb]

Code required without API change:

data := i.ApplicationCommandData()
totalOptions := len(data.Options)
optionMap := make(map[string]*discordgo.ApplicationCommandInteractionDataOption, totalOptions)
for _, v := range data.Options {
    optionMap[v.Name] = v
}

[FedorLap2006]

Not sure about everything else, but making options into a map is a good idea.
I've personally done that a few times.
Making options a map would eliminate confusion for beginners. The use case of positional options is very rare.
So feel free to open a PR with these changes.

[switchupcb]

The first step is changing Options field in the ApplicationCommandInteractionData struct (and perhaps the ApplicationCommandInteractionDataOption struct). Running this will result in an unmarshal error: cannot unmarshal array into map. The unmarshal code is rather spread, so I'd recommend for this issue to be assigned to a contributor who's familiar with the unmarshal methods so nothing is broken. However, this may also not be possible due to how the json is structured.

Alternatively, we can add this code once we unmarshal the JSON:

// where unmarshalledOptions is renamed to whatever variable holds the unmarshalled Slice.
v.Options = make(map[string]ApplicationCommandInteractionDataOption, len(v.Options))
for _, option := range unmarshalledOptions {
	v.Options[option.Name] = option
}

However, this may also warrant unforeseen changes (from my perspective; perhaps not others).

Alternatively, we can add a field OptionMap map[string]*ApplicationCommandInteractionDataOption and use the same code above to have it assigned. The only "issue" is now the type ApplicationCommandInteractionData struct will contain 2 fields (Options slice, OptionMap map) duplicate pointers (8/4 bytes * 2 for 64/32 bit machines). The benefit to this option is that is doesn't effect anyone using the existing API.

Would you (@FedorLap2006) or any contributors guide me on the correct solution before I commit to an implementation?

[FedorLap2006]

I think it would be enough just to parse options into a map directly in the example.
We try to keep DiscordGo as close to API as possible, so changing the field to map is out of question. Additionally, it eliminates the ability get options order (the ability which won't be used widely, but I think that we should keep).
While we can add the OptionMap method, I don't think that's a good idea either, we try to keep structures as clean as possible.

[FedorLap2006]

Also, I'm not sure if we should use issues for that, this feels more like a discussion, but we'll keep it as an issue for now.

[switchupcb]

I'm not sure if we should use issues for that, this feels more like a discussion.
@FedorLap2006

This is a discussion about an issue and the best way to solve it. After reading your insight, we should go with the second option (refactoring the field and parsing in the marshal method). You have the final say on whether it gets implemented.

An alternative method is also justified in reasoning, but I advise against it.

Reasoning

Additionally, it eliminates the ability get options order (the ability which won't be used widely, but I think that we should keep).
@FedorLap2006

We can always store the options position in the structure itself to allow retrieving option order. This is good for all but the single case where you want to reference the option by index immediately. However, the issue with this obscure case, is that it is obscure: We are dealing with options that "can't have the same name". In addition, referencing options is not only inefficient, but a security risk.

Keep this information in mind as it's also relevant to the next quote.

We try to keep DiscordGo as close to API as possible, so changing the field to map is out of question.
@FedorLap2006

The Discord API provides options as an array of options. However, this would imply that an option of the same name is possible. We know that it isn't. The correct data structure to use here is a map. There is no real reason to parse options by index, because that index is not guaranteed.

There is a difference between the structure a JSON provides and the domain structure. We can stay as close to the API as possible without constraining ourselves to an inefficient data structure for the use case. The question we need to answer is whether the API models the strict JSON model or the actual use case? The JSON is a universal format; not an implementation of a language.

  While we can add the OptionMap method
@FedorLap2006

We can solve the issue with this approach as well. It would prevent the library from a breaking change; not a justified reason due to tags. The only reason I'd advise against this approach is because it may be out of the libraries scope, or become unnecessary boilerplate (that should be built-in).

[switchupcb]

This PR does what you've requested. Refactoring is still a decision that needs to be made.