Add/remove labels instead of setting a new set of labels

Setting the full set of labels means that we may race with additions or removals
by the user, which is not the desired behavior. For example, when
autolabeling triggered by files present in a new PR, adding labels would (at
least sometimes) kill labels added by the user, as those are added "after" the
open event is sent, even if immediately specified by the user.

src/github.rs

@@ -3,7 +3,6 @@ use tracing as log;
 
 use async_trait::async_trait;
 use chrono::{DateTime, FixedOffset, Utc};
-use futures::stream::{FuturesUnordered, StreamExt};
 use futures::{future::BoxFuture, FutureExt};
 use hyper::header::HeaderValue;
 use once_cell::sync::OnceCell;
@@ -233,18 +232,6 @@ pub struct Label {
     pub name: String,
 }
 
-impl Label {
-    async fn exists<'a>(&'a self, repo_api_prefix: &'a str, client: &'a GithubClient) -> bool {
-        #[allow(clippy::redundant_pattern_matching)]
-        let url = format!("{}/labels/{}", repo_api_prefix, self.name);
-        match client.send_req(client.get(&url)).await {
-            Ok(_) => true,
-            // XXX: Error handling if the request failed for reasons beyond 'label didn't exist'
-            Err(_) => false,
-        }
-    }
-}
-
 #[derive(Debug, serde::Deserialize)]
 pub struct PullRequestDetails {
     // none for now
@@ -468,13 +455,40 @@ impl Issue {
         Ok(())
     }
 
-    pub async fn set_labels(
+    pub async fn remove_label(&self, client: &GithubClient, label: &str) -> anyhow::Result<()> {
+        log::info!("remove_label from {}: {:?}", self.global_id(), label);
+        // DELETE /repos/:owner/:repo/issues/:number/labels/{name}
+        let url = format!(
+            "{repo_url}/issues/{number}/labels/{name}",
+            repo_url = self.repository().url(),
+            number = self.number,
+            name = label,
+        );
+
+        if !self.labels().iter().any(|l| l.name == label) {
+            log::info!(
+                "remove_label from {}: {:?} already not present, skipping",
+                self.global_id(),
+                label
+            );
+            return Ok(());
+        }
+
+        client
+            ._send_req(client.delete(&url))
+            .await
+            .context("failed to delete label")?;
+
+        Ok(())
+    }
+
+    pub async fn add_labels(
         &self,
         client: &GithubClient,
         labels: Vec<Label>,
     ) -> anyhow::Result<()> {
-        log::info!("set_labels {} to {:?}", self.global_id(), labels);
-        // PUT /repos/:owner/:repo/issues/:number/labels
+        log::info!("add_labels: {} +{:?}", self.global_id(), labels);
+        // POST /repos/:owner/:repo/issues/:number/labels
         // repo_url = https://api.github.com/repos/Codertocat/Hello-World
         let url = format!(
             "{repo_url}/issues/{number}/labels",
@@ -482,13 +496,17 @@ impl Issue {
             number = self.number
         );
 
-        let mut stream = labels
+        // Don't try to add labels already present on this issue.
+        let labels = labels
             .into_iter()
-            .map(|label| async { (label.exists(&self.repository().url(), &client).await, label) })
-            .collect::<FuturesUnordered<_>>();
-        let mut labels = Vec::new();
-        while let Some((true, label)) = stream.next().await {
-            labels.push(label);
+            .filter(|l| !self.labels().contains(&l))
+            .map(|l| l.name)
+            .collect::<Vec<_>>();
+
+        log::info!("add_labels: {} filtered to {:?}", self.global_id(), labels);
+
+        if labels.is_empty() {
+            return Ok(());
         }
 
         #[derive(serde::Serialize)]
@@ -496,11 +514,9 @@ impl Issue {
             labels: Vec<String>,
         }
         client
-            ._send_req(client.put(&url).json(&LabelsReq {
-                labels: labels.iter().map(|l| l.name.clone()).collect(),
-            }))
+            ._send_req(client.post(&url).json(&LabelsReq { labels }))
             .await
-            .context("failed to set labels")?;
+            .context("failed to add labels")?;
 
         Ok(())
     }
@@ -1282,6 +1298,7 @@ impl GithubClient {
         self.client.post(url).configure(self)
     }
 
+    #[allow(unused)]
     fn put(&self, url: &str) -> RequestBuilder {
         log::trace!("put {:?}", url);
         self.client.put(url).configure(self)

src/handlers/autolabel.rs

@@ -3,10 +3,12 @@ use crate::{
     github::{files_changed, IssuesAction, IssuesEvent, Label},
     handlers::Context,
 };
+use anyhow::Context as _;
 use tracing as log;
 
 pub(super) struct AutolabelInput {
-    labels: Vec<Label>,
+    add: Vec<Label>,
+    remove: Vec<Label>,
 }
 
 pub(super) async fn parse_input(
@@ -37,7 +39,10 @@ pub(super) async fn parse_input(
                 }
             }
             if !autolabels.is_empty() {
-                return Ok(Some(AutolabelInput { labels: autolabels }));
+                return Ok(Some(AutolabelInput {
+                    add: autolabels,
+                    remove: vec![],
+                }));
             }
         }
     }
@@ -75,17 +80,21 @@ pub(super) async fn parse_input(
                 });
             }
             if !autolabels.is_empty() {
-                return Ok(Some(AutolabelInput { labels: autolabels }));
+                return Ok(Some(AutolabelInput {
+                    add: autolabels,
+                    remove: vec![],
+                }));
             }
         }
     }
     if event.action == IssuesAction::Closed {
         let labels = event.issue.labels();
-        if let Some(x) = labels.iter().position(|x| x.name == "I-prioritize") {
-            let mut labels_excluded = labels.to_vec();
-            labels_excluded.remove(x);
+        if labels.iter().any(|x| x.name == "I-prioritize") {
             return Ok(Some(AutolabelInput {
-                labels: labels_excluded,
+                add: vec![],
+                remove: vec![Label {
+                    name: "I-prioritize".to_owned(),
+                }],
             }));
         }
     }
@@ -98,13 +107,19 @@ pub(super) async fn handle_input(
     event: &IssuesEvent,
     input: AutolabelInput,
 ) -> anyhow::Result<()> {
-    let mut labels = event.issue.labels().to_owned();
-    for label in input.labels {
-        // Don't add the label if it's already there
-        if !labels.contains(&label) {
-            labels.push(label);
-        }
+    event.issue.add_labels(&ctx.github, input.add).await?;
+    for label in input.remove {
+        event
+            .issue
+            .remove_label(&ctx.github, &label.name)
+            .await
+            .with_context(|| {
+                format!(
+                    "failed to remove {:?} from {:?}",
+                    label,
+                    event.issue.global_id()
+                )
+            })?;
     }
-    event.issue.set_labels(&ctx.github, labels).await?;
     Ok(())
 }

src/handlers/major_change.rs

@@ -217,9 +217,7 @@ async fn handle(
     label_to_add: String,
     new_proposal: bool,
 ) -> anyhow::Result<()> {
-    let mut labels = issue.labels().to_owned();
-    labels.push(Label { name: label_to_add });
-    let github_req = issue.set_labels(&ctx.github, labels);
+    let github_req = issue.add_labels(&ctx.github, vec![Label { name: label_to_add }]);
 
     let partial_issue = issue.to_zulip_github_reference();
     let zulip_topic = zulip_topic_from_issue(&partial_issue);

src/handlers/nominate.rs

@@ -33,7 +33,8 @@ pub(super) async fn handle_command(
         return Ok(());
     }
 
-    let mut issue_labels = event.issue().unwrap().labels().to_owned();
+    let issue_labels = event.issue().unwrap().labels();
+    let mut labels_to_add = vec![];
     if cmd.style == Style::BetaApprove {
         if !issue_labels.iter().any(|l| l.name == "beta-nominated") {
             let cmnt = ErrorComment::new(
@@ -50,11 +51,9 @@ pub(super) async fn handle_command(
 
         // Add the beta-accepted label, but don't attempt to remove beta-nominated or the team
         // label.
-        if !issue_labels.iter().any(|l| l.name == "beta-accepted") {
-            issue_labels.push(github::Label {
-                name: "beta-accepted".into(),
-            });
-        }
+        labels_to_add.push(github::Label {
+            name: "beta-accepted".into(),
+        });
     } else {
         if !config.teams.contains_key(&cmd.team) {
             let cmnt = ErrorComment::new(
@@ -70,29 +69,23 @@ pub(super) async fn handle_command(
         }
 
         let label = config.teams[&cmd.team].clone();
-        if !issue_labels.iter().any(|l| l.name == label) {
-            issue_labels.push(github::Label { name: label });
-        }
+        labels_to_add.push(github::Label { name: label });
 
         let style_label = match cmd.style {
             Style::Decision => "I-nominated",
             Style::Beta => "beta-nominated",
             Style::BetaApprove => unreachable!(),
         };
-        if !issue_labels.iter().any(|l| l.name == style_label) {
-            issue_labels.push(github::Label {
-                name: style_label.into(),
-            });
-        }
+        labels_to_add.push(github::Label {
+            name: style_label.into(),
+        });
     }
 
-    if &issue_labels[..] != event.issue().unwrap().labels() {
-        event
-            .issue()
-            .unwrap()
-            .set_labels(&ctx.github, issue_labels)
-            .await?;
-    }
+    event
+        .issue()
+        .unwrap()
+        .add_labels(&ctx.github, labels_to_add)
+        .await?;
 
     Ok(())
 }

src/handlers/ping.rs

@@ -66,16 +66,11 @@ pub(super) async fn handle_command(
     };
 
     if let Some(label) = config.label.clone() {
-        let issue_labels = event.issue().unwrap().labels();
-        if !issue_labels.iter().any(|l| l.name == label) {
-            let mut issue_labels = issue_labels.to_owned();
-            issue_labels.push(github::Label { name: label });
-            event
-                .issue()
-                .unwrap()
-                .set_labels(&ctx.github, issue_labels)
-                .await?;
-        }
+        event
+            .issue()
+            .unwrap()
+            .add_labels(&ctx.github, vec![github::Label { name: label }])
+            .await?;
     }
 
     let mut users = Vec::new();

src/handlers/prioritize.rs

@@ -11,16 +11,14 @@ pub(super) async fn handle_command(
     event: &Event,
     _: PrioritizeCommand,
 ) -> anyhow::Result<()> {
-    let issue = event.issue().unwrap();
-    let mut labels = issue.labels().to_owned();
-
-    // Don't add the label if it's already there
-    if !labels.iter().any(|l| l.name == config.label) {
-        labels.push(github::Label {
-            name: config.label.to_owned(),
-        });
-    }
-
-    issue.set_labels(&ctx.github, labels).await?;
+    let mut labels = vec![];
+    labels.push(github::Label {
+        name: config.label.to_owned(),
+    });
+    event
+        .issue()
+        .unwrap()
+        .add_labels(&ctx.github, labels)
+        .await?;
     Ok(())
 }

src/handlers/relabel.rs

@@ -22,8 +22,8 @@ pub(super) async fn handle_command(
     event: &Event,
     input: RelabelCommand,
 ) -> anyhow::Result<()> {
-    let mut issue_labels = event.issue().unwrap().labels().to_owned();
-    let mut changed = false;
+    let mut results = vec![];
+    let mut to_add = vec![];
     for delta in &input.0 {
         let name = delta.label().as_str();
         let err = match check_filter(name, config, is_member(&event.user(), &ctx.github).await) {
@@ -46,28 +46,46 @@ pub(super) async fn handle_command(
         }
         match delta {
             LabelDelta::Add(label) => {
-                if !issue_labels.iter().any(|l| l.name == label.as_str()) {
-                    changed = true;
-                    issue_labels.push(github::Label {
-                        name: label.to_string(),
-                    });
-                }
+                to_add.push(github::Label {
+                    name: label.to_string(),
+                });
             }
             LabelDelta::Remove(label) => {
-                if let Some(pos) = issue_labels.iter().position(|l| l.name == label.as_str()) {
-                    changed = true;
-                    issue_labels.remove(pos);
-                }
+                results.push((
+                    label,
+                    event
+                        .issue()
+                        .unwrap()
+                        .remove_label(&ctx.github, &label)
+                        .await,
+                ));
             }
         }
     }
 
-    if changed {
-        event
-            .issue()
-            .unwrap()
-            .set_labels(&ctx.github, issue_labels)
-            .await?;
+    if let Err(e) = event
+        .issue()
+        .unwrap()
+        .add_labels(&ctx.github, to_add.clone())
+        .await
+    {
+        tracing::error!(
+            "failed to add {:?} from issue {}: {:?}",
+            to_add,
+            event.issue().unwrap().global_id(),
+            e
+        );
+    }
+
+    for (label, res) in &results {
+        if let Err(e) = res {
+            tracing::error!(
+                "failed to remove {:?} from issue {}: {:?}",
+                label,
+                event.issue().unwrap().global_id(),
+                e
+            );
+        }
     }
 
     Ok(())

src/handlers/review_submitted.rs

@@ -24,20 +24,19 @@ pub(crate) async fn handle(
         }
 
         if event.issue.assignees.contains(&event.comment.user) {
-            let labels = event
-                .issue
-                .labels()
-                .iter()
-                // Remove review related labels
-                .filter(|label| !config.review_labels.contains(&label.name))
-                .cloned()
-                // Add waiting on author label
-                .chain(std::iter::once(Label {
-                    name: config.reviewed_label.clone(),
-                }));
+            // Remove review labels
+            for label in &config.review_labels {
+                event.issue.remove_label(&ctx.github, &label).await?;
+            }
+            // Add waiting on author
             event
                 .issue
-                .set_labels(&ctx.github, labels.collect())
+                .add_labels(
+                    &ctx.github,
+                    vec![Label {
+                        name: config.reviewed_label.clone(),
+                    }],
+                )
                 .await?;
         }
     }

src/handlers/shortcut.rs

@@ -25,92 +25,26 @@ pub(super) async fn handle_command(
         return Ok(());
     }
 
-    let mut issue_labels = issue.labels().to_owned();
+    let issue_labels = issue.labels().to_owned();
     let waiting_on_review = "S-waiting-on-review";
     let waiting_on_author = "S-waiting-on-author";
 
-    match input {
-        ShortcutCommand::Ready => {
-            if assign_and_remove_label(&mut issue_labels, waiting_on_review, waiting_on_author)
-                .is_some()
-            {
-                return Ok(());
-            }
-            issue.set_labels(&ctx.github, issue_labels).await?;
-        }
-        ShortcutCommand::Author => {
-            if assign_and_remove_label(&mut issue_labels, waiting_on_author, waiting_on_review)
-                .is_some()
-            {
-                return Ok(());
-            }
-            issue.set_labels(&ctx.github, issue_labels).await?;
-        }
-    }
-
-    Ok(())
-}
-
-fn assign_and_remove_label(
-    issue_labels: &mut Vec<Label>,
-    assign: &str,
-    remove: &str,
-) -> Option<()> {
-    if issue_labels.iter().any(|label| label.name == assign) {
-        return Some(());
-    }
-
-    if let Some(index) = issue_labels.iter().position(|label| label.name == remove) {
-        issue_labels.swap_remove(index);
-    }
-
-    issue_labels.push(Label {
-        name: assign.into(),
-    });
-
-    None
-}
-
-#[cfg(test)]
-mod tests {
-
-    use super::{assign_and_remove_label, Label};
-    fn create_labels(names: Vec<&str>) -> Vec<Label> {
-        names
-            .into_iter()
-            .map(|name| Label { name: name.into() })
-            .collect()
-    }
-
-    #[test]
-    fn test_adds_without_labels() {
-        let expected = create_labels(vec!["assign"]);
-        let mut labels = vec![];
-        assert!(assign_and_remove_label(&mut labels, "assign", "remove").is_none());
-        assert_eq!(labels, expected);
-    }
-
-    #[test]
-    fn test_do_nothing_with_label_already_set() {
-        let expected = create_labels(vec!["assign"]);
-        let mut labels = create_labels(vec!["assign"]);
-        assert!(assign_and_remove_label(&mut labels, "assign", "remove").is_some());
-        assert_eq!(labels, expected);
-    }
+    let (add, remove) = match input {
+        ShortcutCommand::Ready => (waiting_on_review, waiting_on_author),
+        ShortcutCommand::Author => (waiting_on_author, waiting_on_review),
+    };
 
-    #[test]
-    fn test_other_labels_untouched() {
-        let expected = create_labels(vec!["bug", "documentation", "assign"]);
-        let mut labels = create_labels(vec!["bug", "documentation"]);
-        assert!(assign_and_remove_label(&mut labels, "assign", "remove").is_none());
-        assert_eq!(labels, expected);
+    if !issue_labels.iter().any(|l| l.name == add) {
+        issue.remove_label(&ctx.github, remove).await?;
+        issue
+            .add_labels(
+                &ctx.github,
+                vec![Label {
+                    name: add.to_owned(),
+                }],
+            )
+            .await?;
     }
 
-    #[test]
-    fn test_correctly_remove_label() {
-        let expected = create_labels(vec!["bug", "documentation", "assign"]);
-        let mut labels = create_labels(vec!["bug", "documentation", "remove"]);
-        assert!(assign_and_remove_label(&mut labels, "assign", "remove").is_none());
-        assert_eq!(labels, expected);
-    }
+    Ok(())
 }