Merge pull request #1540 from Mark-Simulacrum/label-changes

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

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(())
     }
@@ -659,6 +675,25 @@ impl Issue {
             .context("failed to close issue")?;
         Ok(())
     }
+
+    /// Returns the diff in this event, for Open and Synchronize events for now.
+    pub async fn diff(&self, client: &GithubClient) -> anyhow::Result<Option<String>> {
+        let (before, after) = if let (Some(base), Some(head)) = (&self.base, &self.head) {
+            (base.sha.clone(), head.sha.clone())
+        } else {
+            return Ok(None);
+        };
+
+        let mut req = client.get(&format!(
+            "{}/compare/{}...{}",
+            self.repository().url(),
+            before,
+            after
+        ));
+        req = req.header("Accept", "application/vnd.github.v3.diff");
+        let diff = client.send_req(req).await?;
+        Ok(Some(String::from(String::from_utf8_lossy(&diff))))
+    }
 }
 
 #[derive(serde::Serialize)]
@@ -762,13 +797,6 @@ pub struct IssuesEvent {
     pub repository: Repository,
     /// Some if action is IssuesAction::Labeled, for example
     pub label: Option<Label>,
-
-    // These fields are the sha fields before/after a synchronize operation,
-    // used to compute the diff between these two commits.
-    #[serde(default)]
-    before: Option<String>,
-    #[serde(default)]
-    after: Option<String>,
 }
 
 #[derive(Debug, serde::Deserialize)]
@@ -794,37 +822,7 @@ pub fn files_changed(diff: &str) -> Vec<&str> {
     files
 }
 
-impl IssuesEvent {
-    /// Returns the diff in this event, for Open and Synchronize events for now.
-    pub async fn diff_between(&self, client: &GithubClient) -> anyhow::Result<Option<String>> {
-        let (before, after) = if self.action == IssuesAction::Synchronize {
-            (
-                self.before
-                    .clone()
-                    .expect("synchronize has before populated"),
-                self.after.clone().expect("synchronize has after populated"),
-            )
-        } else if self.action == IssuesAction::Opened {
-            if let (Some(base), Some(head)) = (&self.issue.base, &self.issue.head) {
-                (base.sha.clone(), head.sha.clone())
-            } else {
-                return Ok(None);
-            }
-        } else {
-            return Ok(None);
-        };
-
-        let mut req = client.get(&format!(
-            "{}/compare/{}...{}",
-            self.issue.repository().url(),
-            before,
-            after
-        ));
-        req = req.header("Accept", "application/vnd.github.v3.diff");
-        let diff = client.send_req(req).await?;
-        Ok(Some(String::from(String::from_utf8_lossy(&diff))))
-    }
-}
+impl IssuesEvent {}
 
 #[derive(Debug, serde::Deserialize)]
 pub struct IssueSearchResult {
@@ -1282,6 +1280,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(
@@ -14,30 +16,42 @@ pub(super) async fn parse_input(
     event: &IssuesEvent,
     config: Option<&AutolabelConfig>,
 ) -> Result<Option<AutolabelInput>, String> {
+    // On opening a new PR or sync'ing the branch, look at the diff and try to
+    // add any appropriate labels.
+    //
+    // FIXME: This will re-apply labels after a push that the user had tried to
+    // remove. Not much can be done about that currently; the before/after on
+    // synchronize may be straddling a rebase, which will break diff generation.
     if let Some(config) = config {
-        if let Some(diff) = event
-            .diff_between(&ctx.github)
-            .await
-            .map_err(|e| {
-                log::error!("failed to fetch diff: {:?}", e);
-            })
-            .unwrap_or_default()
-        {
-            let files = files_changed(&diff);
-            let mut autolabels = Vec::new();
-            for (label, cfg) in config.labels.iter() {
-                if cfg
-                    .trigger_files
-                    .iter()
-                    .any(|f| files.iter().any(|diff_file| diff_file.starts_with(f)))
-                {
-                    autolabels.push(Label {
-                        name: label.to_owned(),
-                    });
+        if event.action == IssuesAction::Opened || event.action == IssuesAction::Synchronize {
+            if let Some(diff) = event
+                .issue
+                .diff(&ctx.github)
+                .await
+                .map_err(|e| {
+                    log::error!("failed to fetch diff: {:?}", e);
+                })
+                .unwrap_or_default()
+            {
+                let files = files_changed(&diff);
+                let mut autolabels = Vec::new();
+                for (label, cfg) in config.labels.iter() {
+                    if cfg
+                        .trigger_files
+                        .iter()
+                        .any(|f| files.iter().any(|diff_file| diff_file.starts_with(f)))
+                    {
+                        autolabels.push(Label {
+                            name: label.to_owned(),
+                        });
+                    }
+                }
+                if !autolabels.is_empty() {
+                    return Ok(Some(AutolabelInput {
+                        add: autolabels,
+                        remove: vec![],
+                    }));
                 }
-            }
-            if !autolabels.is_empty() {
-                return Ok(Some(AutolabelInput { labels: autolabels }));
             }
         }
     }
@@ -75,17 +89,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 +116,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(())
 }