From 3da38d986e175189c8fa5b9bb7fa53cc8702af4e Mon Sep 17 00:00:00 2001
From: Wez Furlong <wez@wezfurlong.org>
Date: Wed, 29 Jan 2025 10:57:18 -0700
Subject: [PATCH] throttle: serialize in string notation

Prior to exposing max_burst in the throttle string parser, we
couldn't guarantee to represent any arbitrary throttle as a string,
so we serialized as the underlying struct fields.

That's a bit unwieldy to read, so let's switch to the string
representation.

This commit makes the string repr infallible (previously it could fail
if you had specified max_burst), then tackles the fanout from that.

Apparently none of our snapshot tests include any of these throttles,
so I manually spot checked it with `kcli inspect-sched-q` to observe
the `max_message_rate`.
---
 crates/kumo-api-types/src/shaping.rs | 26 ++++++++-------------
 crates/throttle/src/lib.rs           | 34 +++++++++++++---------------
 2 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/crates/kumo-api-types/src/shaping.rs b/crates/kumo-api-types/src/shaping.rs
index ee3006c3..b8cec393 100644
--- a/crates/kumo-api-types/src/shaping.rs
+++ b/crates/kumo-api-types/src/shaping.rs
@@ -1219,22 +1219,16 @@ impl ProviderEntry {
             );
         }
         if let Some(rate) = &self.provider_max_message_rate {
-            match rate.as_string() {
-                Ok(rate) => {
-                    let mut limits = toml::Table::new();
-                    limits.insert(
-                        format!("shaping-provider-{}-{source}-rate", self.provider_name),
-                        rate.into(),
-                    );
-                    implied.insert(
-                        "additional_message_rate_throttles".to_string(),
-                        toml::Value::Table(limits),
-                    );
-                }
-                Err(err) => {
-                    tracing::error!("Error representing provider_max_message_rate: {err}");
-                }
-            }
+            let rate = rate.as_string();
+            let mut limits = toml::Table::new();
+            limits.insert(
+                format!("shaping-provider-{}-{source}-rate", self.provider_name),
+                rate.into(),
+            );
+            implied.insert(
+                "additional_message_rate_throttles".to_string(),
+                toml::Value::Table(limits),
+            );
         }
 
         toml_table_merge_from(target, &implied);
diff --git a/crates/throttle/src/lib.rs b/crates/throttle/src/lib.rs
index f44701cf..282e23d1 100644
--- a/crates/throttle/src/lib.rs
+++ b/crates/throttle/src/lib.rs
@@ -82,7 +82,7 @@ pub enum Error {
 }
 
 #[derive(Eq, PartialEq, Clone, Copy, Serialize, Deserialize, Hash)]
-#[serde(try_from = "String")]
+#[serde(try_from = "String", into = "String")]
 pub struct ThrottleSpec {
     pub limit: u64,
     /// Period, in seconds
@@ -129,24 +129,18 @@ impl ThrottleSpec {
 
 impl std::fmt::Debug for ThrottleSpec {
     fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
-        match self.as_string() {
-            Ok(s) => write!(fmt, "{}", s),
-            Err(_) => Err(std::fmt::Error),
-        }
+        write!(fmt, "{}", self.as_string())
     }
 }
 
 impl std::fmt::Display for ThrottleSpec {
     fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
-        match self.as_string() {
-            Ok(s) => write!(fmt, "{}", s),
-            Err(_) => Err(std::fmt::Error),
-        }
+        write!(fmt, "{}", self.as_string())
     }
 }
 
 impl ThrottleSpec {
-    pub fn as_string(&self) -> Result<String, String> {
+    pub fn as_string(&self) -> String {
         let mut period_scale = None;
         let period = match self.period {
             86400 => "d",
@@ -164,7 +158,7 @@ impl ThrottleSpec {
             None => String::new(),
         };
 
-        Ok(format!(
+        format!(
             "{}{}/{}{period}{burst}",
             if self.force_local { "local:" } else { "" },
             self.limit,
@@ -172,7 +166,13 @@ impl ThrottleSpec {
                 Some(scale) => scale.as_str(),
                 None => "",
             }
-        ))
+        )
+    }
+}
+
+impl From<ThrottleSpec> for String {
+    fn from(spec: ThrottleSpec) -> String {
+        spec.as_string()
     }
 }
 
@@ -519,8 +519,7 @@ mod test {
                 max_burst: None,
                 force_local: false,
             }
-            .as_string()
-            .unwrap(),
+            .as_string(),
             "100/h"
         );
         assert_eq!(
@@ -530,8 +529,7 @@ mod test {
                 max_burst: None,
                 force_local: true,
             }
-            .as_string()
-            .unwrap(),
+            .as_string(),
             "local:100/h"
         );
 
@@ -545,7 +543,7 @@ mod test {
                 force_local: true,
             }
         );
-        assert_eq!(weird_duration.as_string().unwrap(), "local:100/7380s");
+        assert_eq!(weird_duration.as_string(), "local:100/7380s");
 
         assert_eq!(
             ThrottleSpec::try_from("1_0,0/hour").unwrap(),
@@ -575,7 +573,7 @@ mod test {
                 force_local: false,
             }
         );
-        assert_eq!(burst.as_string().unwrap(), "50/d,max_burst=1");
+        assert_eq!(burst.as_string(), "50/d,max_burst=1");
     }
 
     #[test]