Merge pull request #5 from rambler-digital-solutions/issue-4

issue-4: Fix problem with unwrap inside cache_ttl

actix-cache-derive/Cargo.toml

@@ -1,8 +1,9 @@
 [package]
 name = "actix-cache-derive"
 version = "0.1.0"
-authors = ["Andrey Ermilov <andrerm@ya.ru>"]
+authors = ["Andrey Ermilov <andrerm@ya.ru>", "Belousow Makc <lib.bmw@gmail.com>"]
 edition = "2018"
+workspace = ".."
 
 [dependencies]
 syn = "1"

actix-cache-derive/LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2020 Actix-Cache project contributors
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.

actix-cache-derive/src/cacheable_macro.rs

@@ -40,8 +40,9 @@ pub fn impl_cacheable_macro(ast: &syn::DeriveInput) -> TokenStream {
 
     let gen = quote! {
         impl Cacheable for #name {
-            fn cache_key(&self) -> String {
-                actix_cache::serde_qs::to_string(self).unwrap()
+            fn cache_key(&self) -> Result<String, CacheError> {
+                actix_cache::serde_qs::to_string(self)
+                    .map_err(|error| CacheError::CacheKeyGenerationError(error.to_string()))
             }
             #cache_ttl_implement
             #cache_stale_ttl_implement

actix-cache-derive/src/lib.rs

@@ -2,6 +2,7 @@
 //!
 //! ```edition2018
 //! use actix_cache::cache::Cacheable;
+//! use actix_cache::error::CacheError;
 //! use serde::Serialize;
 //!
 //! #[derive(Cacheable, Serialize)]
@@ -12,7 +13,7 @@
 //!     field: i32,
 //! };
 //! let message = Message { field: 42 };
-//! assert_eq!(message.cache_key(), "field=42".to_string());
+//! assert_eq!(message.cache_key().unwrap(), "field=42".to_string());
 //! ```
 mod cacheable_macro;
 mod macro_attributes;

actix-cache-derive/tests/test_cacheable_derive.rs

@@ -0,0 +1,112 @@
+use actix_cache::cache::Cacheable;
+use serde::Serialize;
+use actix_cache::CacheError;
+
+#[derive(Cacheable, Serialize)]
+struct Message {
+    id: i32,
+    alias: String,
+}
+
+#[test]
+fn test_all_keys() {
+    let message = Message { id: 0, alias: "alias".to_string() };
+    assert_eq!(message.cache_key().unwrap(), "id=0&alias=alias".to_string());
+}
+
+
+#[derive(Cacheable, Serialize)]
+#[allow(dead_code)]
+struct PartialSerializeMessage {
+    id: i32,
+    #[serde(skip_serializing)]
+    alias: String,
+}
+
+#[test]
+fn test_partial() {
+    let message = PartialSerializeMessage { id: 0, alias: "alias".to_string() };
+    assert_eq!(message.cache_key().unwrap(), "id=0".to_string());
+}
+
+#[derive(Cacheable, Serialize)]
+struct VecMessage {
+    id: Vec<i32>,
+}
+
+#[test]
+fn test_message_with_vector() {
+    let message = VecMessage { id: vec![1, 2, 3] };
+    assert_eq!(message.cache_key().unwrap(), "id[0]=1&id[1]=2&id[2]=3".to_string());
+}
+
+#[derive(Serialize)]
+enum MessageType {
+    External,
+}
+
+#[derive(Cacheable, Serialize)]
+struct EnumMessage {
+    message_type: MessageType
+}
+
+#[test]
+fn test_message_with_enum() {
+    let message = EnumMessage { message_type: MessageType::External };
+    assert_eq!(message.cache_key().unwrap(), "message_type=External".to_string());
+}
+
+#[derive(Serialize)]
+enum TupleMessageType {
+    External(i32),
+}
+
+#[derive(Cacheable, Serialize)]
+struct TupleEnumMessage {
+    message_type: TupleMessageType
+}
+
+#[test]
+fn test_message_with_enum_tuple() {
+    let message = TupleEnumMessage { message_type: TupleMessageType::External(1) };
+    assert_eq!(message.cache_key().unwrap(), "message_type[External]=1".to_string());
+}
+
+// Should we support tuple struct?
+#[derive(Cacheable, Serialize)]
+struct TupleMessage(i32);
+
+#[test]
+fn test_tuple_returns_error() {
+    let message = TupleMessage(1);
+    assert!(message.cache_key().is_err());
+}
+
+#[derive(Cacheable, Serialize)]
+#[cache_ttl(42)]
+#[cache_stale_ttl(30)]
+#[cache_version(1)]
+struct MacroHelpersMessage {
+    message_type: i32
+}
+
+#[test]
+fn test_macro_helpers_work() {
+    let message = MacroHelpersMessage { message_type: 1 };
+    assert_eq!(message.cache_ttl(), 42);
+    assert_eq!(message.cache_stale_ttl(), 30);
+    assert_eq!(message.cache_version(), 1);
+}
+
+#[derive(Cacheable, Serialize)]
+struct DefaultMessage {
+    message_type: i32
+}
+
+#[test]
+fn test_default_ttl_stale_ttl_version_work() {
+    let message = DefaultMessage { message_type: 1 };
+    assert_eq!(message.cache_ttl(), 60);
+    assert_eq!(message.cache_stale_ttl(), 55);
+    assert_eq!(message.cache_version(), 0);
+}

src/cache.rs

@@ -30,25 +30,27 @@ pub trait Cacheable {
     ///
     /// ```
     /// use actix_cache::cache::Cacheable;
+    /// use actix_cache::CacheError;
     ///
     /// struct QueryNothing {
     ///     id: Option<i32>,
     /// }
     ///
     /// impl Cacheable for QueryNothing {
-    ///     fn cache_key(&self) -> String {
-    ///         format!("database::QueryNothing::id::{}", self.id.map_or_else(
+    ///     fn cache_key(&self) -> Result<String, CacheError> {
+    ///         let key = format!("database::QueryNothing::id::{}", self.id.map_or_else(
     ///             || "None".to_owned(), |id| id.to_string())
-    ///         )
+    ///         );
+    ///         Ok(key)
     ///     }
     /// }
     ///
     /// let query = QueryNothing { id: Some(1) };
-    /// assert_eq!(query.cache_key(), "database::QueryNothing::id::1");
+    /// assert_eq!(query.cache_key().unwrap(), "database::QueryNothing::id::1");
     /// let query = QueryNothing { id: None };
-    /// assert_eq!(query.cache_key(), "database::QueryNothing::id::None");
+    /// assert_eq!(query.cache_key().unwrap(), "database::QueryNothing::id::None");
     /// ```
-    fn cache_key(&self) -> String;
+    fn cache_key(&self) -> Result<String, CacheError>;
 
     /// Describe time-to-live (ttl) value for cache storage in seconds.
     ///
@@ -75,18 +77,6 @@ pub trait Cacheable {
         0
     }
 
-    /// Describe cache label.
-    ///
-    /// By default cache label is a cache key prefix: `{actor}::{message type}`.
-    /// This value used for aggregating metrics by actors and message.
-    fn label(&self) -> String {
-        self.cache_key()
-            .split("::")
-            .take(2)
-            .collect::<Vec<&str>>()
-            .join("::")
-    }
-
     fn into_cache<A>(self, upstream: Addr<A>) -> QueryCache<A, Self>
     where
         A: Actor,
@@ -130,12 +120,17 @@ where
 
     fn handle(&mut self, msg: QueryCache<A, M>, _: &mut Self::Context) -> Self::Result {
         let backend = self.backend.clone();
-        let key = msg.message.cache_key();
-        let enabled = self.enabled;
+        let (enabled, cache_key) = match msg.message.cache_key() {
+            Ok(value) => (self.enabled, value),
+            Err(error) => {
+                warn!("Creating cache key error: {}", error);
+                (false, String::new())
+            }
+        };
         let res = async move {
-            debug!("Try retrive cached value from backend");
+            debug!("Try retrieve cached value from backend");
             let cached = if enabled {
-                CachedValue::retrive(&backend, &msg).await
+                CachedValue::retrieve(&backend, cache_key.clone()).await
             } else {
                 None
             };
@@ -157,7 +152,7 @@ where
                         let _ = backend
                             .send(Set {
                                 value: serde_json::to_string(&cached)?,
-                                key,
+                                key: cache_key,
                                 ttl: Some(cache_ttl),
                             })
                             .await?
@@ -194,17 +189,12 @@ impl<T> CachedValue<T> {
         }
     }
 
-    async fn retrive<A, M>(backend: &Addr<RedisActor>, msg: &QueryCache<A, M>) -> Option<Self>
+    async fn retrieve(backend: &Addr<RedisActor>, cache_key: String) -> Option<Self>
     where
-        A: Actor,
-        M: Message + Cacheable + Send,
-        M::Result: MessageResponse<A, M> + Send + 'static,
         T: DeserializeOwned,
     {
         let value = backend
-            .send(Get {
-                key: msg.message.cache_key(),
-            })
+            .send(Get { key: cache_key })
             .await;
         let serialized = match value {
             Ok(Ok(value)) => value,
@@ -242,8 +232,8 @@ mod tests {
     struct Message;
 
     impl Cacheable for Message {
-        fn cache_key(&self) -> String {
-            "Message".to_owned()
+        fn cache_key(&self) -> Result<String, CacheError> {
+            Ok("Message".to_owned())
         }
 
         fn cache_ttl(&self) -> u32 {

src/error.rs

@@ -10,4 +10,6 @@ pub enum CacheError {
     SerializeError(#[from] serde_json::Error),
     #[error("Cached data deserialization error")]
     DeserializeError,
+    #[error("Cache key generation error")]
+    CacheKeyGenerationError(String),
 }