From 8e134fa56c45020eb11675e6f767515bcbad7b65 Mon Sep 17 00:00:00 2001 From: o0Ignition0o Date: Thu, 17 Aug 2023 16:26:12 +0200 Subject: [PATCH 1/4] log an error in the event of failure to serialize a redis value to json, instead of panic. --- apollo-router/src/cache/redis.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apollo-router/src/cache/redis.rs b/apollo-router/src/cache/redis.rs index 1dae895287..e05a4de90d 100644 --- a/apollo-router/src/cache/redis.rs +++ b/apollo-router/src/cache/redis.rs @@ -101,8 +101,12 @@ where type Error = RedisError; fn try_into(self) -> Result { - let v = serde_json::to_vec(&self.0) - .expect("JSON serialization should not fail for redis values"); + let v = serde_json::to_vec(&self.0).map_err(|e| { + RedisError::new( + RedisErrorKind::Parse, + format!("couldn't serialize value to redis {}. This is a bug in the router, please file an issue.", e), + ) + })?; Ok(fred::types::RedisValue::Bytes(v.into())) } From 14c295bb88bae98f6c7e653a988f12d9571009d5 Mon Sep 17 00:00:00 2001 From: o0Ignition0o Date: Thu, 17 Aug 2023 17:04:11 +0200 Subject: [PATCH 2/4] add a test --- apollo-router/src/cache/redis.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/apollo-router/src/cache/redis.rs b/apollo-router/src/cache/redis.rs index e05a4de90d..8f761c96cd 100644 --- a/apollo-router/src/cache/redis.rs +++ b/apollo-router/src/cache/redis.rs @@ -102,9 +102,10 @@ where fn try_into(self) -> Result { let v = serde_json::to_vec(&self.0).map_err(|e| { + println!("couldn't serialize value to redis {}. This is a bug in the router, please file an issue: https://github.com/apollographql/router/issues/new", e); RedisError::new( RedisErrorKind::Parse, - format!("couldn't serialize value to redis {}. This is a bug in the router, please file an issue.", e), + format!("couldn't serialize value to redis {}", e), ) })?; @@ -336,3 +337,25 @@ impl RedisCacheStorage { tracing::trace!("insert result {:?}", r); } } + +#[cfg(test)] +mod test { + use std::time::SystemTime; + + #[test] + fn ensure_invalid_payload_serialization_doesnt_fail() { + #[derive(Clone, Debug, serde::Serialize, serde::Deserialize)] + struct Stuff { + time: SystemTime, + } + + let invalid_json_payload = super::RedisValue(Stuff { + // this systemtime is invalid, serialization will fail + time: std::time::UNIX_EPOCH - std::time::Duration::new(1, 0), + }); + + let as_value: Result = invalid_json_payload.try_into(); + + assert!(as_value.is_err()); + } +} From 1e4e880d522b4423f71b2e3b4d80d29e9bdbea5f Mon Sep 17 00:00:00 2001 From: o0Ignition0o Date: Thu, 17 Aug 2023 17:06:57 +0200 Subject: [PATCH 3/4] tracing::error --- apollo-router/src/cache/redis.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-router/src/cache/redis.rs b/apollo-router/src/cache/redis.rs index 8f761c96cd..67c070a3b6 100644 --- a/apollo-router/src/cache/redis.rs +++ b/apollo-router/src/cache/redis.rs @@ -102,7 +102,7 @@ where fn try_into(self) -> Result { let v = serde_json::to_vec(&self.0).map_err(|e| { - println!("couldn't serialize value to redis {}. This is a bug in the router, please file an issue: https://github.com/apollographql/router/issues/new", e); + tracing::error!("couldn't serialize value to redis {}. This is a bug in the router, please file an issue: https://github.com/apollographql/router/issues/new", e); RedisError::new( RedisErrorKind::Parse, format!("couldn't serialize value to redis {}", e), From 374ffbcf61ff6c9edfe1e793f086b0d94e412eed Mon Sep 17 00:00:00 2001 From: o0Ignition0o Date: Thu, 17 Aug 2023 17:17:28 +0200 Subject: [PATCH 4/4] changeset --- .changesets/fix_igni_safely_deal_with_invalid_redis_keys.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changesets/fix_igni_safely_deal_with_invalid_redis_keys.md diff --git a/.changesets/fix_igni_safely_deal_with_invalid_redis_keys.md b/.changesets/fix_igni_safely_deal_with_invalid_redis_keys.md new file mode 100644 index 0000000000..7c1169450c --- /dev/null +++ b/.changesets/fix_igni_safely_deal_with_invalid_redis_keys.md @@ -0,0 +1,6 @@ +### Redis storage: return an error instead if a non serializable value is sent. ([#3594](https://github.com/apollographql/router/issues/3594)) + +This changeset returns an error if a value couldn't be serialized before being sent to the redis storage backend. +It also logs the error in console and prompts you to open an issue (This message showing up would be a router bug!). + +By [@o0Ignition0o](https://github.com/o0Ignition0o) in https://github.com/apollographql/router/pull/3597