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

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

Author: Mark-Simulacrum

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,39 +455,68 @@ 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",
             repo_url = self.repository().url(),
             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)]
         struct LabelsReq {
             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,41 +3,55 @@ 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(
     ctx: &Context,
     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);