Simplify string caching (#1158)

Since `StringCacheValue` is always a String, there's not much point in
having a separate type for it. This adds a marker trait that gets
implemented in the macro so that we can have still have type safety
around cache keys that store strings, but otherwise change the relevant
cache methods to just take/return `&str`/`String` as appropriate.

Author: jaymell

server/svix-server/src/core/cache/memory.rs

@@ -106,7 +106,7 @@ fn check_is_expired(vw: &ValueWrapper) -> bool {
 #[cfg(test)]
 mod tests {
     use super::{
-        super::{kv_def, CacheValue, StringCacheValue},
+        super::{kv_def, CacheValue},
         *,
     };
     use crate::core::cache::string_kv_def;
@@ -134,26 +134,13 @@ mod tests {
 
     #[derive(Deserialize, Serialize, Debug, PartialEq)]
     struct StringTestVal(String);
-    string_kv_def!(StringTestKey, StringTestVal);
+    string_kv_def!(StringTestKey);
     impl StringTestKey {
         fn new(id: String) -> StringTestKey {
             StringTestKey(format!("SVIX_TEST_KEY_STRING_{id}"))
         }
     }
 
-    impl std::fmt::Display for StringTestVal {
-        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-            write!(f, "{}", self.0)
-        }
-    }
-
-    impl TryFrom<String> for StringTestVal {
-        type Error = crate::error::Error;
-        fn try_from(s: String) -> crate::error::Result<Self> {
-            Ok(StringTestVal(s))
-        }
-    }
-
     #[tokio::test]
     async fn test_cache_crud_no_ttl() {
         let cache = new();
@@ -167,8 +154,8 @@ mod tests {
         );
         let (third_key, third_val_a, third_val_b) = (
             StringTestKey::new("1".to_owned()),
-            StringTestVal("1".to_owned()),
-            StringTestVal("2".to_owned()),
+            "1".to_owned(),
+            "2".to_owned(),
         );
 
         // Create
@@ -223,10 +210,7 @@ mod tests {
         // Confirm deletion
         assert_eq!(cache.get::<TestValA>(&first_key).await.unwrap(), None);
         assert_eq!(cache.get::<TestValB>(&second_key).await.unwrap(), None);
-        assert_eq!(
-            cache.get_string::<StringTestVal>(&third_key).await.unwrap(),
-            None
-        );
+        assert_eq!(cache.get_string(&third_key).await.unwrap(), None);
     }
 
     #[tokio::test]

server/svix-server/src/core/cache/mod.rs

@@ -40,6 +40,10 @@ type Result<T> = std::result::Result<T, Error>;
 /// A valid key value for the cache -- usually just a wrapper around a [`String`]
 pub trait CacheKey: AsRef<str> + Send + Sync {}
 
+/// A cache key for setting/getting raw [`String`]s -- this is just a marker
+/// trait added in the `string_kv_def macro`
+pub trait StringCacheKey: AsRef<str> + Send + Sync {}
+
 /// Any (de)serializable structure usable as a value in the cache -- it is associated with a
 /// given key type to ensure type checking on creation or reading of values from the cache
 pub trait CacheValue: DeserializeOwned + Serialize + Send + Sync {
@@ -85,7 +89,7 @@ pub(crate) use kv_def;
 /// but it can't be made private or else it couldn't be used in the outer macro.
 #[allow(unused_macros)]
 macro_rules! string_kv_def_inner {
-    ($key_id:ident, $val_struct:ident) => {
+    ($key_id:ident) => {
         #[derive(Clone, Debug)]
         pub struct $key_id(String);
 
@@ -94,10 +98,6 @@ macro_rules! string_kv_def_inner {
                 &self.0
             }
         }
-
-        impl StringCacheValue for $val_struct {
-            type Key = $key_id;
-        }
     };
 }
 #[allow(unused_imports)]
@@ -106,10 +106,12 @@ pub(crate) use string_kv_def_inner;
 // Used downstream and for testing:
 #[allow(unused_macros)]
 macro_rules! string_kv_def {
-    ($key_id:ident, $val_struct:ident) => {
-        crate::core::cache::string_kv_def_inner!($key_id, $val_struct);
+    ($key_id:ident) => {
+        crate::core::cache::string_kv_def_inner!($key_id);
 
-        impl CacheKey for $key_id {}
+        impl crate::core::cache::StringCacheKey for $key_id {}
+        // so key can work w/ other methods, like delete:
+        impl crate::core::cache::CacheKey for $key_id {}
     };
 }
 #[allow(unused_imports)]
@@ -159,16 +161,12 @@ pub trait CacheBehavior: Sync + Send {
 
     async fn get_raw(&self, key: &[u8]) -> Result<Option<Vec<u8>>>;
 
-    async fn get_string<T: StringCacheValue>(&self, key: &T::Key) -> Result<Option<T>> {
+    async fn get_string<T: StringCacheKey>(&self, key: &T) -> Result<Option<String>> {
         run_with_retries(
             || async move {
                 self.get_raw(key.as_ref().as_bytes())
                     .await?
-                    .map(|x| {
-                        String::from_utf8(x)
-                            .map_err(|e| e.into())
-                            .and_then(|x| x.try_into().map_err(|_| Error::DeserializationOther))
-                    })
+                    .map(|x| String::from_utf8(x).map_err(|e| e.into()))
                     .transpose()
             },
             |e| self.should_retry(e),
@@ -195,10 +193,10 @@ pub trait CacheBehavior: Sync + Send {
 
     async fn set_raw(&self, key: &[u8], value: &[u8], ttl: Duration) -> Result<()>;
 
-    async fn set_string<T: StringCacheValue>(
+    async fn set_string<T: StringCacheKey>(
         &self,
-        key: &T::Key,
-        value: &T,
+        key: &T,
+        value: &str,
         ttl: Duration,
     ) -> Result<()> {
         run_with_retries(
@@ -237,10 +235,10 @@ pub trait CacheBehavior: Sync + Send {
 
     async fn set_raw_if_not_exists(&self, key: &[u8], value: &[u8], ttl: Duration) -> Result<bool>;
 
-    async fn set_string_if_not_exists<T: StringCacheValue>(
+    async fn set_string_if_not_exists<T: StringCacheKey>(
         &self,
-        key: &T::Key,
-        value: &T,
+        key: &T,
+        value: &str,
         ttl: Duration,
     ) -> Result<bool> {
         run_with_retries(

server/svix-server/src/core/cache/none.rs

@@ -5,7 +5,7 @@ use std::time::Duration;
 
 use axum::async_trait;
 
-use super::{Cache, CacheBehavior, CacheKey, CacheValue, Result, StringCacheValue};
+use super::{Cache, CacheBehavior, CacheKey, CacheValue, Result, StringCacheKey};
 
 pub fn new() -> Cache {
     tracing::warn!("Running with caching disabled will negatively affect performance. Idempotency is not supported without a cache.");
@@ -29,7 +29,7 @@ impl CacheBehavior for NoCache {
         Ok(None)
     }
 
-    async fn get_string<T: StringCacheValue>(&self, _key: &T::Key) -> Result<Option<T>> {
+    async fn get_string<T: StringCacheKey>(&self, _key: &T) -> Result<Option<String>> {
         Ok(None)
     }
 
@@ -41,10 +41,10 @@ impl CacheBehavior for NoCache {
         Ok(())
     }
 
-    async fn set_string<T: StringCacheValue>(
+    async fn set_string<T: StringCacheKey>(
         &self,
-        _key: &T::Key,
-        _value: &T,
+        _key: &T,
+        _value: &str,
         _ttl: Duration,
     ) -> Result<()> {
         Ok(())
@@ -72,10 +72,10 @@ impl CacheBehavior for NoCache {
         Ok(false)
     }
 
-    async fn set_string_if_not_exists<T: StringCacheValue>(
+    async fn set_string_if_not_exists<T: StringCacheKey>(
         &self,
-        _key: &T::Key,
-        _value: &T,
+        _key: &T,
+        _value: &str,
         _ttl: Duration,
     ) -> Result<bool> {
         Ok(false)

server/svix-server/src/core/cache/redis.rs

@@ -76,7 +76,7 @@ impl CacheBehavior for RedisCache {
 #[cfg(test)]
 mod tests {
     use super::{
-        super::{kv_def, string_kv_def, CacheValue, StringCacheValue},
+        super::{kv_def, string_kv_def, CacheValue},
         *,
     };
     use serde::{Deserialize, Serialize};
@@ -105,26 +105,13 @@ mod tests {
 
     #[derive(Deserialize, Serialize, Debug, PartialEq)]
     struct StringTestVal(String);
-    string_kv_def!(StringTestKey, StringTestVal);
+    string_kv_def!(StringTestKey);
     impl StringTestKey {
         fn new(id: String) -> StringTestKey {
             StringTestKey(format!("SVIX_TEST_KEY_STRING_{id}"))
         }
     }
 
-    impl std::fmt::Display for StringTestVal {
-        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-            write!(f, "{}", self.0)
-        }
-    }
-
-    impl TryFrom<String> for StringTestVal {
-        type Error = crate::error::Error;
-        fn try_from(s: String) -> crate::error::Result<Self> {
-            Ok(StringTestVal(s))
-        }
-    }
-
     async fn get_pool(redis_dsn: &str, cfg: &crate::cfg::Configuration) -> RedisPool {
         match cfg.cache_type {
             CacheType::RedisCluster => crate::redis::new_redis_pool_clustered(redis_dsn, cfg).await,
@@ -149,8 +136,8 @@ mod tests {
         );
         let (third_key, third_val_a, third_val_b) = (
             StringTestKey::new("1".to_owned()),
-            StringTestVal("1".to_owned()),
-            StringTestVal("2".to_owned()),
+            "1".to_owned(),
+            "2".to_owned(),
         );
 
         // Create
@@ -205,10 +192,7 @@ mod tests {
         // Confirm deletion
         assert_eq!(cache.get::<TestValA>(&first_key).await.unwrap(), None);
         assert_eq!(cache.get::<TestValB>(&second_key).await.unwrap(), None);
-        assert_eq!(
-            cache.get_string::<StringTestVal>(&third_key).await.unwrap(),
-            None
-        );
+        assert_eq!(cache.get_string(&third_key).await.unwrap(), None);
     }
 
     #[tokio::test]