From d852d3a6f562f6d6d8d3ee29c9accdb1d0ead270 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 12 Oct 2024 00:47:08 +0200 Subject: [PATCH] Use a manual parser for `rust-timer queue` Instead of a regex. This allows the user to pass the arguments in an arbitrary order. --- Cargo.lock | 19 ++++ site/Cargo.toml | 3 + site/src/request_handlers/github.rs | 156 ++++++++++++++++++++++++---- 3 files changed, 160 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 55d0441f5..9507460c4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1257,6 +1257,18 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "insta" +version = "1.40.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6593a41c7a73841868772495db7dc1e8ecab43bb5c0b6da2059246c4b506ab60" +dependencies = [ + "console", + "lazy_static", + "linked-hash-map", + "similar", +] + [[package]] name = "instant" version = "0.1.12" @@ -1400,6 +1412,12 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "linked-hash-map" +version = "0.5.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0717cef1bc8b636c6e1c1bbdefc09e6322da8a9321966e8928ef80d20f7f770f" + [[package]] name = "linux-raw-sys" version = "0.1.4" @@ -2430,6 +2448,7 @@ dependencies = [ "humansize", "hyper", "inferno", + "insta", "itertools", "jemalloc-ctl", "jemallocator", diff --git a/site/Cargo.toml b/site/Cargo.toml index 30b2994de..a11f90246 100644 --- a/site/Cargo.toml +++ b/site/Cargo.toml @@ -57,3 +57,6 @@ jemalloc-ctl = "0.5" serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } toml = "0.7" + +[dev-dependencies] +insta = "1.40.0" diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index fd7e97a7d..dd1a5d1c0 100644 --- a/site/src/request_handlers/github.rs +++ b/site/src/request_handlers/github.rs @@ -5,15 +5,13 @@ use crate::github::{ }; use crate::load::SiteCtxt; -use std::sync::Arc; - +use hashbrown::HashMap; use regex::Regex; +use std::sync::Arc; lazy_static::lazy_static! { static ref BODY_TIMER_BUILD: Regex = Regex::new(r"(?:\W|^)@rust-timer\s+build\s+(\w+)(?:\W|$)(?:include=(\S+))?\s*(?:exclude=(\S+))?\s*(?:runs=(\d+))?").unwrap(); - static ref BODY_TIMER_QUEUE: Regex = - Regex::new(r"(?:\W|^)@rust-timer\s+queue(?:\W|$)(?:include=(\S+))?\s*(?:exclude=(\S+))?\s*(?:runs=(\d+))?").unwrap(); } pub async fn handle_github( @@ -118,26 +116,25 @@ async fn handle_rust_timer( return Ok(github::Response); } - if let Some(captures) = BODY_TIMER_QUEUE.captures(&comment.body) { - let include = captures.get(1).map(|v| v.as_str()); - let exclude = captures.get(2).map(|v| v.as_str()); - let runs = captures.get(3).and_then(|v| v.as_str().parse::().ok()); - { - let conn = ctxt.conn().await; - conn.queue_pr(issue.number, include, exclude, runs).await; - } - main_client - .post_comment( - issue.number, + if let Some(queue) = parse_queue_command(&comment.body) { + let msg = match queue { + Ok(cmd) => { + let conn = ctxt.conn().await; + conn.queue_pr(issue.number, cmd.include, cmd.exclude, cmd.runs) + .await; format!( "Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf {COMMENT_MARK_TEMPORARY}" - ), - ) - .await; + ) + } + Err(error) => { + format!("Error occurred while parsing comment: {error}") + } + }; + main_client.post_comment(issue.number, msg).await; return Ok(github::Response); } @@ -163,6 +160,64 @@ async fn handle_rust_timer( Ok(github::Response) } +/// Parses the first occurrence of a `@rust-timer queue <...>` command +/// in the input string. +fn parse_queue_command(body: &str) -> Option> { + let bot_line = body + .lines() + .find_map(|line| line.trim().strip_prefix("@rust-timer")) + .map(|l| l.trim())?; + + let args = bot_line.strip_prefix("queue").map(|l| l.trim())?; + let mut args = match parse_command_arguments(args) { + Ok(args) => args, + Err(error) => return Some(Err(error)), + }; + let mut cmd = QueueCommand::default(); + cmd.include = args.remove("include"); + cmd.exclude = args.remove("exclude"); + if let Some(runs) = args.remove("runs") { + let Ok(runs) = runs.parse::() else { + return Some(Err(format!("Cannot parse runs {runs} as a number"))); + }; + cmd.runs = Some(runs as i32); + } + + if let Some((key, _)) = args.into_iter().next() { + return Some(Err(format!("Unknown command argument {key}"))); + } + + Some(Ok(cmd)) +} + +/// Parses command arguments from a single line of text. +/// Expects that arguments are separated by whitespace, and each argument +/// has the format `=`. +fn parse_command_arguments(args: &str) -> Result, String> { + let mut argmap = HashMap::new(); + for arg in args.split_whitespace() { + let Some((key, value)) = arg.split_once("=") else { + return Err(format!( + "Invalid command argument `{arg}` (there may be no spaces around the `=` character)" + )); + }; + let key = key.trim(); + let value = value.trim(); + if argmap.insert(key, value).is_some() { + return Err(format!("Duplicate command argument `{key}`")); + } + } + + Ok(argmap) +} + +#[derive(Default, Debug)] +struct QueueCommand<'a> { + include: Option<&'a str>, + exclude: Option<&'a str>, + runs: Option, +} + /// Run the `@rust-timer build` regex over the comment message extracting the commit and the other captures fn build_captures(comment_body: &str) -> impl Iterator { BODY_TIMER_BUILD @@ -196,6 +251,7 @@ pub async fn get_authorized_users() -> Result, String> { #[cfg(test)] mod tests { use super::*; + #[test] fn captures_all_shas() { let comment_body = r#" @@ -215,4 +271,68 @@ Going to do perf runs for a few of these: ] ); } + + #[test] + fn command_missing() { + assert!(parse_queue_command("").is_none()); + } + + #[test] + fn unknown_command() { + assert!(parse_queue_command("@rust-timer foo").is_none()); + } + + #[test] + fn queue_command() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue"), + @"Some(Ok(QueueCommand { include: None, exclude: None, runs: None }))"); + } + + #[test] + fn queue_command_unknown_arg() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue foo=bar"), + @r###"Some(Err("Unknown command argument foo"))"###); + } + + #[test] + fn queue_command_duplicate_arg() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=a exclude=c include=b"), + @r###"Some(Err("Duplicate command argument `include`"))"###); + } + + #[test] + fn queue_command_include() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=abcd,feih"), + @r###"Some(Ok(QueueCommand { include: Some("abcd,feih"), exclude: None, runs: None }))"###); + } + + #[test] + fn queue_command_exclude() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue exclude=foo134,barzbaz41baf"), + @r###"Some(Ok(QueueCommand { include: None, exclude: Some("foo134,barzbaz41baf"), runs: None }))"###); + } + + #[test] + fn queue_command_runs() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=5"), + @"Some(Ok(QueueCommand { include: None, exclude: None, runs: Some(5) }))"); + } + + #[test] + fn queue_command_runs_nan() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue runs=xxx"), + @r###"Some(Err("Cannot parse runs xxx as a number"))"###); + } + + #[test] + fn queue_command_combination() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include=acda,13asd exclude=c13,DA runs=5"), + @r###"Some(Ok(QueueCommand { include: Some("acda,13asd"), exclude: Some("c13,DA"), runs: Some(5) }))"###); + } + + #[test] + fn queue_command_spaces() { + insta::assert_compact_debug_snapshot!(parse_queue_command("@rust-timer queue include = abcd,das exclude=ada21"), + @r###"Some(Err("Invalid command argument `include` (there may be no spaces around the `=` character)"))"###); + } }