RFC: use builders for HTTP client methods

[baptiste0928]

Actually, methods of the http client are very similar to builders, and some conflicts with existing builders (e.g. #1165). I think it would be better to transform all these methods to independent builders. This will allow more flexibility, such as sending built types over network or storing them in database.

Suggested implementation

Two new traits are introduced. The Validate trait will be implemented by models to perform validation of their data (e.g. checking command description length). The Builder trait will be implemented by builders. Builders must validate type using their Validate implementation before building them.

(I am not sure if the syntax of the following code is correct, it is an example)

pub trait Validate {
   type ValidationError: Error;   

   fn validate(&self) -> Result<(), Self::ValidationError>;
}

pub trait Builder<T: Validate> {
   type BuildError: Error;   

   fn build(self) -> Result<T, Self::BuildError>;
}

A default implementation of Builder will be provided for each type that implement Validate (the implementation will only validate the type and return it).

Example usage

Therefore, most existing http methods could take an impl Builder<T> parameter :

let message = MessageBuilder::new(); // Build a message (no call to .build() here)

http
   .create_message(message)?  // Will build and validate the message
   .exec()
   .await?;

It will not be mandatory to use a builder, you can also construct the type manually, and it will be validated using its Validate impl.

What do you think about that ?

[laralove143]

why not validate it when building and use the same built struct for every api call instead? this way it won't have to be validated every time there's an api request

[baptiste0928]

In this case, no validation would be performed when using types created manually, only when a builder is used.

[itohatweb]

How about http methods taking a TryFrom<T> instead?
After bit of thinking Builder<T> seems to be better.

[itohatweb]

Mabye make the builder error to require From<T::ValidationError>?

[7596ff]

We already had a discussion about builders that went a while, with the result being here: #1056 (comment)

The short-term plan, however, does not consider validation. There is still discussion to be had about what to do regarding validation. Our HTTP client will always perform validation to ensure it causes the least amount of errors on Discord's side. However, the validation methods could be exposed for other uses, if one is making their own HTTP requests without our client, but still wishes to use our models.

My thinking is that builders should not do validation at all, and users may validate built structs if they choose. Again, our HTTP client will always validate. But it's important to have one source of truth regarding validation, one correct set of constants and functions that validate structs.

[7596ff]

To respond to some more discussion on Discord:

Methods like create_global_command (#1165); The request body to create a command requires a majority of the fields that the Command struct has. It's the same with Components. These structs should have builders because developers are directly sending the same models that users are seeing. However, with Message, as was mentioned, the fields that we send are a fraction of what is received. In a message's case, it would be better to keep what we already have, which are HTTP request builders for the fields that we need to send.

My vision for a unified validation API is something like this:

pub struct MessageValidationError {
    kind: MessageValidationErrorType,
    source: Option<Box<dyn Error + Send + Sync>>,
}

pub enum MessageValidtionErrorType {
    ContentInvalid,
    EmbedsInvalid,
    ComponentsInvalid,
    // ...
}

impl MessageValidationError {
    // our usual error methods...

    pub fn content(content: &str) -> Self {
        // work to validate the content of a message
    }

    pub fn embeds(embeds: &[Embed]) -> Self {
        EmbedValidationError::embeds(embeds).map_err(|source| MessageValidationError {
            kind: MessageValidationErrorType::EmbedsInvalid,
            source: Some(source),
        })
    }
}

And on each of the create-message-like HTTP request builders, we keep our content(&str) -> Result<Self, MessageValidationError> {} that return this validation error.

This way users can use the EmbedBuilder and other buidlers without having to deal with validation, and hopefully in a constant context, if they wish to make embeds and store them elsewhere, or when responding to an interaction (such as with our example client: https://github.com/twilight-rs/twilight/tree/main/examples/model-webhook-slash), they can create it, validate it, and send it back to Discord without using our HTTP client.

[itohatweb]

Although this is inconsistent design?

[7596ff]

To sum up more discussion on Discord, and to respond directly here as well, yes, having some methods take a built struct and some having their own internal request builders is inconsistent. Sending messages is probably the most important feature of a bot user, so additionally, this needs careful consideration on API design to ensure flexibility.

Proposal

To extend off of Baptiste's design above, I am proposing that create and update flows for certain resources have a payload method, that accepts a built and validated version of the resource. The flows will still contain all their previous fns for creating and updating single aspects of resources, but in the common use case of having canned responses to common tasks a bot may perform, the user might choose to use this payload method.

Valdiation

Validation will either be performed by the already-existing fns in these flows, or by a Builder for the resource itself. As a library, it is our responsibility to ensure we send validated data to Discord. However, users might not want to perform these checks if they know their data is already validated, so we should provide a flow where users can create structs that are validated once and sent many times.

However, these builders should also be able to be used in invalidated contexts. Users might want to send invalid data. With the upcoming http-ratelimiter crate that exists in next, and the next major release, users are able to properly ratelimit requests without using our Client, so they still have this choice.

Messages

There are many different ways to send messages. Listed:

• Creating messages
  • application::interaction::create_followup_message
  • application::interaction::interaction_callback
  • channel::message::create_message
  • channel::webhook::execute_webhook
• Updating messages
  • application::interaction::update_followup_message
  • application::interaction::update_original_response
  • channel::message::update_message
  • channel::webhook::update_webhook_message

These should have a unified and consistent API. Obviously, they can't, since they have different use cases and some have unique fields, etc., but we should ensure that they behave similarly when they can. Part of how I want to ensure this is by having a single error type across all of these methods, that have functions for validating their specific parts, such as components or the message length. This error type can also be used in a MessagePayloadBuilder so that everything is validated by the same constants and there is only one place to change them if they must be changed. I have a stash for this in the works, that I'll be PR'ing sometime in December.

The only remaining consideration is payload_json, which some endpoints have, and which is also used internally in order to faciliate uploading files. I'm omitting a plan for this in order to get this post out at all.

Analysis

After analyzing all of our API requests, separating them into concerns, and comparing how much the JSON body structure is shared in the actual Discord resource itself, I have made a list of them. Primarily, this will be based on creating a Discord resource.

Role: 7/11 (63%) shared fields.

struct Role {
    // Shared fields.
    color: u32,
    hoist: bool,
    icon: Option<String>,
    mentionable: bool,
    name: String,
    permissions: Permissions,
    unicode_emoji: Option<String>,

    // Only present in Discord resource.
    id: RoleId,
    managed: bool,
    position: i64,
    tags: Option<RoleTags>,
}

Channel: 10/16 (62%) shared fields.

// Combined fields from Category, Text, and Voice.
struct TextChannel {
    // Shared fields.
    bitrate: u64,
    kind: ChannelType,
    name: String,
    nsfw: bool,
    parent_id: Option<ChannelId>,
    permission_overwrites: Vec<PermissionOverwrite>,
    position: i64,
    rate_limit_per_user: Option<u64>,
    topic: Option<String>,
    user_limit: Option<u64>,

    // Only present in Discord resource.
    guild_id: Option<GuildId>,
    id: ChannelId,
    last_message_id: Option<MessageId>,
    last_pin_timestamp: Option<Timestamp>,
    rtc_region: Option<String>,
    video_quality_mode: Option<VideoQualityMode>,
}

Command: 5/9 (55%) shared fields.

struct Command {
    // Shared fields.
    default_permission: Option<bool>,
    description: String,
    kind: CommandType,
    name: String,
    options: Vec<CommandOption>,

    // Only present in Discord resource.
    application_id: Option<ApplicationId>,
    guild_id: Option<GuildId>,
    id: Option<CommandId>,
    version: CommandVersionId,
}

StageInstance: 3/6 (50%) shared fields.

struct StageInstance {
    // Shared fields.
    channel_id: ChannelId,
    privacy_level: PrivacyLevel,
    topic: String,

    // Only present in Discord resource.
    discoverable_disabled: bool,
    guild_id: GuildId,
    id: StageId,
}

Ban: 1/3 (33%) shared fields.

struct Ban {
    // Shared fields.
    reason: Option<String>,

    // Only present in Discord resource.
    pub user: User,

    // Only present in `Create{}Fields`.
    delete_message_days: Option<u64>,
}

Sticker: 3/12 (25%) shared fields.

struct Sticker {
    // Shared fields.
    description: Option<String>,
    name: String,
    tags: String,

    // Only present in Discord resource.
    available: bool,
    format_type: StickerFormatType,
    guild_id: Option<GuildId>,
    id: StickerId,
    kind: StickerType,
    pack_id: Option<StickerPackId>,
    sort_value: Option<u64>,
    user: Option<User>,
    
    // Only present in `Create{}Fields`.
    tags: &'a str,
}
</details>

<details>
<summary>Emoji: 2/9 (22%) shared fields.</summary>

```rust
struct Emoji {
    // Shared fields.
    name: String,
    roles: Vec<RoleId>,

    // Only present in Discord resource.
    animated: bool,
    available: bool,
    id: EmojiId,
    managed: bool,
    require_colons: bool,
    user: Option<User>,

    // Only present in `Create{}Fields`.
    image: &'a str,
}

Template: 2/11 (18%) shared fields.

pub struct Template {
    // Shared fields.
    description: Option<String>,
    name: String,

    // Only present in Discord resource.
    code: String,
    created_at: Timestamp,
    creator: User,
    creator_id: UserId,
    is_dirty: Option<bool>,
    serialized_source_guild: TemplateGuild,
    source_guild_id: GuildId,
    updated_at: Timestamp,
    usage_count: u64,
}

Invite: 3/18 (16%) shared fields.

struct Invite {
    // Shared fields.
    pub max_age: Option<u64>,
    pub max_uses: Option<u64>,
    pub temporary: Option<bool>,

    // Only present in Discord resource.
    approximate_member_count: Option<u64>,
    approximate_presence_count: Option<u64>,
    channel: InviteChannel,
    code: String,
    created_at: Option<Timestamp>,
    expires_at: Option<Timestamp>,
    guild: Option<InviteGuild>,
    inviter: Option<User>,
    stage_instance: Option<InviteStageInstance>,
    target_type: Option<TargetType>,
    target_user: Option<User>,
    uses: Option<u64>,

    // Only present in `Create{}Fields`.
    target_application_id: Option<UserId>,
    target_user_id: Option<UserId>,
    unique: Option<bool>,
}

Webhook: 2/12 (16%) shared fields.

struct Webhook {
    // Shared fields.
    avatar: Option<String>,
    name: Option<String>,

    // Only present in Discord resource.
    application_id: Option<ApplicationId>,
    channel_id: ChannelId,
    guild_id: Option<GuildId>,
    id: WebhookId,
    kind: WebhookType,
    source_channel: Option<WebhookChannel>,
    source_guild: Option<WebhookGuild>,
    token: Option<String>,
    url: Option<String>,
    user: Option<User>,
}

Thread: 2/14 (14%) shared fields.

// For comparison's sake, separate channels and threads as concerns.
struct Thread {
    // Shared fields.
    auto_archive_duration: Option<AutoArchiveDuration>,
    name: String,

    // Only present in Discord resource.
    default_auto_archive_duration: Option<AutoArchiveDuration>,
    guild_id: Option<GuildId>,
    kind: ChannelType,
    last_message_id: Option<MessageId>,
    member: Option<ThreadMember>,
    member_count: u8,
    message_count: u8,
    name: String,
    owner_id: Option<UserId>,
    parent_id: Option<ChannelId>,
    rate_limit_per_user: Option<u64>,
    thread_metadata: ThreadMetadata,
}

Special Cases

Emojis

I can definitely see cases for creating canned emojis in a server that the bot uses for interactivity, so despite them sharing only 22% of the fields with its Discord resource, this API could do with a payload method.

Guilds

We already have a complicated builder situation for our CreateGuild API. It does validation, but it is fully present in http, so it does not fit into our current builder plans. It deserves separate consideration as well, since it is an cold route except in special use cases.

Reactions

We already have quite a few different forms that reactions can take. When creating a reaction, the user passes a RequestReactionType. When receiving a message, the reactions take the form of a MessageReaction, which itself has a ReactionType. These structs hold a unicode emoji or enough identifiable information about a custom emoji. The custom variant is different from an Emoji, which contains many more fields.