Add CreateAttachment.description and EditAttachment (serenity-rs#2575)

* Add attachments to payloads, support description

* Add separate struct for new message attachments

* Make everything cleaner (in my judgment, that is)

CreateAttachment is now Serialize and can be used for the `attachments` field directly, eliminating the extra `files` fields.

Attachment handling is now centralized in EditAttachments. This removes redundancy in serenity code and makes it more maintainable. Also, by modifying attachments through EditAttachments instead of ad-hoc methods on the builder itself, the side effects of adding attachments can now be documented much more clearly, as well as mirroring those side effects in the builder syntax (also see serenity-rs#2570 (comment)). The old ad-hoc methods are kept for backwards-compatibility, but marked deprecated

* new_keep_all -> keep_all as per mkrasnitski/polarbits' suggestion

* Update src/builder/create_attachment.rs

Co-authored-by: Michael Krasnitski <[email protected]>

* Update src/builder/create_attachment.rs

Co-authored-by: Michael Krasnitski <[email protected]>

* Rename dont_keep to remove

As per the principle of "do what you can be blamed the least for", even though I still don't like this rename

* Add MultipartUpload enum

* Undeprecate methods

* Apply mkrasnitski/polarbits' suggestions

* Update src/builder/edit_interaction_response.rs

Co-authored-by: Michael Krasnitski <[email protected]>

* Update src/builder/edit_interaction_response.rs

Co-authored-by: Michael Krasnitski <[email protected]>

* Apply suggestions from code review

Co-authored-by: Michael Krasnitski <[email protected]>

* Update src/builder/create_attachment.rs

Co-authored-by: Michael Krasnitski <[email protected]>

* Update src/builder/create_attachment.rs

Co-authored-by: Michael Krasnitski <[email protected]>

* Fix CI

---------

Co-authored-by: Kneemund <[email protected]>
Co-authored-by: Michael Krasnitski <[email protected]>
Co-authored-by: Michael Krasnitski <[email protected]>

examples/testing/src/main.rs

@@ -59,7 +59,7 @@ async fn message(ctx: &Context, msg: Message) -> Result<(), serenity::Error> {
             )
             .await?;
         // Pre-PR, this falsely triggered a MODEL_TYPE_CONVERT Discord error
-        msg.edit(&ctx, EditMessage::new().add_existing_attachment(msg.attachments[0].id)).await?;
+        msg.edit(&ctx, EditMessage::new().attachments(EditAttachments::keep_all(&msg))).await?;
     } else if msg.content == "unifiedattachments" {
         let mut msg = channel_id.send_message(ctx, CreateMessage::new().content("works")).await?;
         msg.edit(ctx, EditMessage::new().content("works still")).await?;
@@ -72,9 +72,9 @@ async fn message(ctx: &Context, msg: Message) -> Result<(), serenity::Error> {
             .await?;
         msg.edit(
             ctx,
-            EditMessage::new()
-                .attachment(CreateAttachment::url(ctx, IMAGE_URL_2).await?)
-                .add_existing_attachment(msg.attachments[0].id),
+            EditMessage::new().attachments(
+                EditAttachments::keep_all(&msg).add(CreateAttachment::url(ctx, IMAGE_URL_2).await?),
+            ),
         )
         .await?;
     } else if msg.content == "ranking" {
@@ -240,9 +240,10 @@ async fn interaction(
         let msg = interaction
             .edit_response(
                 &ctx,
-                EditInteractionResponse::new()
-                    .new_attachment(CreateAttachment::url(ctx, IMAGE_URL_2).await?)
-                    .keep_existing_attachment(msg.attachments[0].id),
+                EditInteractionResponse::new().attachments(
+                    EditAttachments::keep_all(&msg)
+                        .add(CreateAttachment::url(ctx, IMAGE_URL_2).await?),
+                ),
             )
             .await?;
 
@@ -253,8 +254,7 @@ async fn interaction(
             .edit_response(
                 &ctx,
                 EditInteractionResponse::new()
-                    .clear_existing_attachments()
-                    .keep_existing_attachment(msg.attachments[1].id),
+                    .attachments(EditAttachments::new().keep(msg.attachments[1].id)),
             )
             .await?;
     } else if interaction.data.name == "unifiedattachments1" {

src/builder/create_attachment.rs

@@ -5,33 +5,29 @@ use tokio::io::AsyncReadExt;
 #[cfg(feature = "http")]
 use url::Url;
 
+use crate::all::Message;
 #[cfg(feature = "http")]
 use crate::error::Error;
 use crate::error::Result;
 #[cfg(feature = "http")]
 use crate::http::Http;
 use crate::model::id::AttachmentId;
 
-/// [Discord docs] with the caveat at the top "For the attachments array in Message Create/Edit
-/// requests, only the id is required."
-///
-/// [Discord docs]: https://discord.com/developers/docs/resources/channel#attachment-object-attachment-structure
-#[derive(Clone, Debug, Serialize)]
-pub(crate) struct ExistingAttachment {
-    pub id: AttachmentId,
-    // TODO: add the other non-required attachment fields? Like content_type, description,
-    // ephemeral (ephemeral in particular seems pretty interesting)
-}
-
 /// Enum that allows a user to pass a [`Path`] or a [`File`] type to [`send_files`]
 ///
+/// [Discord docs](https://discord.com/developers/docs/resources/channel#attachment-object-attachment-structure).
+///
 /// [`send_files`]: crate::model::id::ChannelId::send_files
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Serialize)]
 #[non_exhaustive]
 #[must_use]
 pub struct CreateAttachment {
-    pub data: Vec<u8>,
+    pub(crate) id: u64, // Placeholder ID will be filled in when sending the request
     pub filename: String,
+    pub description: Option<String>,
+
+    #[serde(skip)]
+    pub data: Vec<u8>,
 }
 
 impl CreateAttachment {
@@ -40,6 +36,8 @@ impl CreateAttachment {
         CreateAttachment {
             data: data.into(),
             filename: filename.into(),
+            description: None,
+            id: 0,
         }
     }
 
@@ -60,10 +58,7 @@ impl CreateAttachment {
             )
         })?;
 
-        Ok(CreateAttachment {
-            data,
-            filename: filename.to_string_lossy().to_string(),
-        })
+        Ok(CreateAttachment::bytes(data, filename.to_string_lossy().to_string()))
     }
 
     /// Builds an [`CreateAttachment`] by reading from a file handler.
@@ -75,10 +70,7 @@ impl CreateAttachment {
         let mut data = Vec::new();
         file.try_clone().await?.read_to_end(&mut data).await?;
 
-        Ok(CreateAttachment {
-            data,
-            filename: filename.into(),
-        })
+        Ok(CreateAttachment::bytes(data, filename))
     }
 
     /// Builds an [`CreateAttachment`] by downloading attachment data from a URL.
@@ -98,10 +90,7 @@ impl CreateAttachment {
             .and_then(Iterator::last)
             .ok_or_else(|| Error::Url(url.to_string()))?;
 
-        Ok(CreateAttachment {
-            data,
-            filename: filename.to_string(),
-        })
+        Ok(CreateAttachment::bytes(data, filename))
     }
 
     /// Converts the stored data to the base64 representation.
@@ -117,4 +106,179 @@ impl CreateAttachment {
         encoded.insert_str(0, "data:image/png;base64,");
         encoded
     }
+
+    /// Sets a description for the file (max 1024 characters).
+    pub fn description(mut self, description: impl Into<String>) -> Self {
+        self.description = Some(description.into());
+        self
+    }
+}
+
+#[derive(Debug, Clone, serde::Serialize)]
+struct ExistingAttachment {
+    id: AttachmentId,
+}
+
+#[derive(Debug, Clone, serde::Serialize)]
+#[serde(untagged)]
+enum NewOrExisting {
+    New(CreateAttachment),
+    Existing(ExistingAttachment),
+}
+
+/// You can add new attachments and edit existing ones using this builder.
+///
+/// When this builder is _not_ supplied in a message edit, Discord keeps the attachments intact.
+/// However, as soon as a builder is supplied, Discord removes all attachments from the message. If
+/// you want to keep old attachments, you must specify this either using [`Self::keep_all`], or
+/// individually for each attachment using [`Self::keep`].
+///
+/// # Examples
+///
+/// ## Removing all attachments
+///
+/// ```rust,no_run
+/// # use serenity::all::*;
+/// # async fn _foo(ctx: Http, mut msg: Message) -> Result<(), Error> {
+/// msg.edit(ctx, EditMessage::new().attachments(EditAttachments::new())).await?;
+/// # Ok(()) }
+/// ```
+///
+/// ## Adding a new attachment without deleting existing attachments
+///
+/// ```rust,no_run
+/// # use serenity::all::*;
+/// # async fn _foo(ctx: Http, mut msg: Message, my_attachment: CreateAttachment) -> Result<(), Error> {
+/// msg.edit(ctx, EditMessage::new().attachments(
+///     EditAttachments::keep_all(&msg).add(my_attachment)
+/// )).await?;
+/// # Ok(()) }
+/// ```
+///
+/// ## Delete all but the first attachment
+///
+/// ```rust,no_run
+/// # use serenity::all::*;
+/// # async fn _foo(ctx: Http, mut msg: Message, my_attachment: CreateAttachment) -> Result<(), Error> {
+/// msg.edit(ctx, EditMessage::new().attachments(
+///     EditAttachments::new().keep(msg.attachments[0].id)
+/// )).await?;
+/// # Ok(()) }
+/// ```
+///
+/// ## Delete only the first attachment
+///
+/// ```rust,no_run
+/// # use serenity::all::*;
+/// # async fn _foo(ctx: Http, mut msg: Message, my_attachment: CreateAttachment) -> Result<(), Error> {
+/// msg.edit(ctx, EditMessage::new().attachments(
+///     EditAttachments::keep_all(&msg).remove(msg.attachments[0].id)
+/// )).await?;
+/// # Ok(()) }
+/// ```
+///
+/// # Notes
+///
+/// Internally, this type is used not just for message editing endpoints, but also for message
+/// creation endpoints.
+#[derive(Default, Debug, Clone, serde::Serialize)]
+#[serde(transparent)]
+#[must_use]
+pub struct EditAttachments {
+    new_and_existing_attachments: Vec<NewOrExisting>,
+}
+
+impl EditAttachments {
+    /// An empty attachments builder.
+    ///
+    /// Existing attachments are not kept by default, either. See [`Self::keep_all()`] or
+    /// [`Self::keep()`].
+    pub fn new() -> Self {
+        Self::default()
+    }
+
+    /// Creates a new attachments builder that keeps all existing attachments.
+    ///
+    /// Shorthand for [`Self::new()`] and calling [`Self::keep()`] for every [`AttachmentId`] in
+    /// [`Message::attachments`].
+    ///
+    /// If you only want to keep a subset of attachments from the message, either implement this
+    /// method manually, or use [`Self::remove()`].
+    ///
+    /// **Note: this EditAttachments must be run on the same message as is supplied here, or else
+    /// Discord will throw an error!**
+    pub fn keep_all(msg: &Message) -> Self {
+        Self {
+            new_and_existing_attachments: msg
+                .attachments
+                .iter()
+                .map(|a| {
+                    NewOrExisting::Existing(ExistingAttachment {
+                        id: a.id,
+                    })
+                })
+                .collect(),
+        }
+    }
+
+    /// This method adds an existing attachment to the list of attachments that are kept after
+    /// editing.
+    ///
+    /// Opposite of [`Self::remove`].
+    pub fn keep(mut self, id: AttachmentId) -> Self {
+        self.new_and_existing_attachments.push(NewOrExisting::Existing(ExistingAttachment {
+            id,
+        }));
+        self
+    }
+
+    /// This method removes an existing attachment from the list of attachments that are kept after
+    /// editing.
+    ///
+    /// Opposite of [`Self::keep`].
+    pub fn remove(mut self, id: AttachmentId) -> Self {
+        #[allow(clippy::match_like_matches_macro)] // `matches!` is less clear here
+        self.new_and_existing_attachments.retain(|a| match a {
+            NewOrExisting::Existing(a) if a.id == id => false,
+            _ => true,
+        });
+        self
+    }
+
+    /// Adds a new attachment to the attachment list.
+    #[allow(clippy::should_implement_trait)] // Clippy thinks add == std::ops::Add::add
+    pub fn add(mut self, attachment: CreateAttachment) -> Self {
+        self.new_and_existing_attachments.push(NewOrExisting::New(attachment));
+        self
+    }
+
+    /// Clones all new attachments into a new Vec, keeping only data and filename, because those
+    /// are needed for the multipart form data. The data is taken out of `self` in the process, so
+    /// this method can only be called once.
+    pub(crate) fn take_files(&mut self) -> Vec<CreateAttachment> {
+        let mut id_placeholder = 0;
+
+        let mut files = Vec::new();
+        for attachment in &mut self.new_and_existing_attachments {
+            if let NewOrExisting::New(attachment) = attachment {
+                let mut cloned_attachment = CreateAttachment::bytes(
+                    std::mem::take(&mut attachment.data),
+                    attachment.filename.clone(),
+                );
+
+                // Assign placeholder IDs so Discord can match metadata to file contents
+                attachment.id = id_placeholder;
+                cloned_attachment.id = id_placeholder;
+                files.push(cloned_attachment);
+
+                id_placeholder += 1;
+            }
+        }
+        files
+    }
+
+    #[cfg(feature = "cache")]
+    pub(crate) fn is_empty(&self) -> bool {
+        self.new_and_existing_attachments.is_empty()
+    }
 }

src/builder/create_interaction_response.rs

@@ -1,6 +1,12 @@
 #[cfg(feature = "http")]
 use super::{check_overflow, Builder};
-use super::{CreateActionRow, CreateAllowedMentions, CreateAttachment, CreateEmbed};
+use super::{
+    CreateActionRow,
+    CreateAllowedMentions,
+    CreateAttachment,
+    CreateEmbed,
+    EditAttachments,
+};
 #[cfg(feature = "http")]
 use crate::constants;
 #[cfg(feature = "http")]
@@ -132,7 +138,7 @@ impl Builder for CreateInteractionResponse {
         let files = match &mut self {
             CreateInteractionResponse::Message(msg)
             | CreateInteractionResponse::Defer(msg)
-            | CreateInteractionResponse::UpdateMessage(msg) => std::mem::take(&mut msg.files),
+            | CreateInteractionResponse::UpdateMessage(msg) => msg.attachments.take_files(),
             _ => Vec::new(),
         };
 
@@ -156,9 +162,7 @@ pub struct CreateInteractionResponseMessage {
     flags: Option<MessageFlags>,
     #[serde(skip_serializing_if = "Option::is_none")]
     components: Option<Vec<CreateActionRow>>,
-
-    #[serde(skip)]
-    files: Vec<CreateAttachment>,
+    attachments: EditAttachments,
 }
 
 impl CreateInteractionResponseMessage {
@@ -179,13 +183,15 @@ impl CreateInteractionResponseMessage {
 
     /// Appends a file to the message.
     pub fn add_file(mut self, file: CreateAttachment) -> Self {
-        self.files.push(file);
+        self.attachments = self.attachments.add(file);
         self
     }
 
     /// Appends a list of files to the message.
     pub fn add_files(mut self, files: impl IntoIterator<Item = CreateAttachment>) -> Self {
-        self.files.extend(files);
+        for file in files {
+            self.attachments = self.attachments.add(file);
+        }
         self
     }
 
@@ -194,8 +200,8 @@ impl CreateInteractionResponseMessage {
     /// Calling this multiple times will overwrite the file list. To append files, call
     /// [`Self::add_file`] or [`Self::add_files`] instead.
     pub fn files(mut self, files: impl IntoIterator<Item = CreateAttachment>) -> Self {
-        self.files = files.into_iter().collect();
-        self
+        self.attachments = EditAttachments::new();
+        self.add_files(files)
     }
 
     /// Set the content of the message.