From 5a0afcc83541a725b3dc140b1982b159f671abfd Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 13 Oct 2023 15:49:31 -0700 Subject: [PATCH] Simplify notification serialization --- crates/collab/src/db.rs | 3 +- crates/collab/src/db/queries/notifications.rs | 8 +-- .../notifications/src/notification_store.rs | 8 +-- crates/rpc/src/notification.rs | 50 ++++++++----------- 4 files changed, 29 insertions(+), 40 deletions(-) diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 67055d27ee..1bf5c95f6b 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -13,6 +13,7 @@ use anyhow::anyhow; use collections::{BTreeMap, HashMap, HashSet}; use dashmap::DashMap; use futures::StreamExt; +use queries::channels::ChannelGraph; use rand::{prelude::StdRng, Rng, SeedableRng}; use rpc::{ proto::{self}, @@ -47,8 +48,6 @@ pub use ids::*; pub use sea_orm::ConnectOptions; pub use tables::user::Model as User; -use self::queries::channels::ChannelGraph; - pub struct Database { options: ConnectOptions, pool: DatabaseConnection, diff --git a/crates/collab/src/db/queries/notifications.rs b/crates/collab/src/db/queries/notifications.rs index 2ea5fd149f..bf9c9d74ef 100644 --- a/crates/collab/src/db/queries/notifications.rs +++ b/crates/collab/src/db/queries/notifications.rs @@ -76,10 +76,10 @@ impl Database { notification: Notification, tx: &DatabaseTransaction, ) -> Result { - let notification = notification.to_any(); + let notification = notification.to_proto(); let kind = *self .notification_kinds_by_name - .get(notification.kind.as_ref()) + .get(¬ification.kind) .ok_or_else(|| anyhow!("invalid notification kind {:?}", notification.kind))?; let model = notification::ActiveModel { @@ -110,10 +110,10 @@ impl Database { notification: Notification, tx: &DatabaseTransaction, ) -> Result> { - let notification = notification.to_any(); + let notification = notification.to_proto(); let kind = *self .notification_kinds_by_name - .get(notification.kind.as_ref()) + .get(¬ification.kind) .ok_or_else(|| anyhow!("invalid notification kind {:?}", notification.kind))?; let actor_id = notification.actor_id.map(|id| UserId::from_proto(id)); let notification = notification::Entity::find() diff --git a/crates/notifications/src/notification_store.rs b/crates/notifications/src/notification_store.rs index 087637a100..af39941d2f 100644 --- a/crates/notifications/src/notification_store.rs +++ b/crates/notifications/src/notification_store.rs @@ -4,7 +4,7 @@ use client::{Client, UserStore}; use collections::HashMap; use db::smol::stream::StreamExt; use gpui::{AppContext, AsyncAppContext, Entity, ModelContext, ModelHandle, Task}; -use rpc::{proto, AnyNotification, Notification, TypedEnvelope}; +use rpc::{proto, Notification, TypedEnvelope}; use std::{ops::Range, sync::Arc}; use sum_tree::{Bias, SumTree}; use time::OffsetDateTime; @@ -185,11 +185,7 @@ impl NotificationStore { is_read: message.is_read, timestamp: OffsetDateTime::from_unix_timestamp(message.timestamp as i64) .ok()?, - notification: Notification::from_any(&AnyNotification { - actor_id: message.actor_id, - kind: message.kind.into(), - content: message.content, - })?, + notification: Notification::from_proto(&message)?, }) }) .collect::>(); diff --git a/crates/rpc/src/notification.rs b/crates/rpc/src/notification.rs index 8224c2696c..6ff9660159 100644 --- a/crates/rpc/src/notification.rs +++ b/crates/rpc/src/notification.rs @@ -1,7 +1,7 @@ +use crate::proto; use serde::{Deserialize, Serialize}; use serde_json::{map, Value}; -use std::borrow::Cow; -use strum::{EnumVariantNames, IntoStaticStr, VariantNames as _}; +use strum::{EnumVariantNames, VariantNames as _}; const KIND: &'static str = "kind"; const ACTOR_ID: &'static str = "actor_id"; @@ -9,10 +9,12 @@ const ACTOR_ID: &'static str = "actor_id"; /// A notification that can be stored, associated with a given user. /// /// This struct is stored in the collab database as JSON, so it shouldn't be -/// changed in a backward-incompatible way. +/// changed in a backward-incompatible way. For example, when renaming a +/// variant, add a serde alias for the old name. /// -/// For example, when renaming a variant, add a serde alias for the old name. -#[derive(Debug, Clone, PartialEq, Eq, EnumVariantNames, IntoStaticStr, Serialize, Deserialize)] +/// When a notification is initiated by a user, use the `actor_id` field +/// to store the user's id. +#[derive(Debug, Clone, PartialEq, Eq, EnumVariantNames, Serialize, Deserialize)] #[serde(tag = "kind")] pub enum Notification { ContactRequest { @@ -32,36 +34,28 @@ pub enum Notification { }, } -/// The representation of a notification that is stored in the database and -/// sent over the wire. -#[derive(Debug)] -pub struct AnyNotification { - pub kind: Cow<'static, str>, - pub actor_id: Option, - pub content: String, -} - impl Notification { - pub fn to_any(&self) -> AnyNotification { - let kind: &'static str = self.into(); + pub fn to_proto(&self) -> proto::Notification { let mut value = serde_json::to_value(self).unwrap(); let mut actor_id = None; - if let Some(value) = value.as_object_mut() { - value.remove(KIND); - if let map::Entry::Occupied(e) = value.entry(ACTOR_ID) { - if e.get().is_u64() { - actor_id = e.remove().as_u64(); - } + let value = value.as_object_mut().unwrap(); + let Some(Value::String(kind)) = value.remove(KIND) else { + unreachable!() + }; + if let map::Entry::Occupied(e) = value.entry(ACTOR_ID) { + if e.get().is_u64() { + actor_id = e.remove().as_u64(); } } - AnyNotification { - kind: Cow::Borrowed(kind), + proto::Notification { + kind, actor_id, content: serde_json::to_string(&value).unwrap(), + ..Default::default() } } - pub fn from_any(notification: &AnyNotification) -> Option { + pub fn from_proto(notification: &proto::Notification) -> Option { let mut value = serde_json::from_str::(¬ification.content).ok()?; let object = value.as_object_mut()?; object.insert(KIND.into(), notification.kind.to_string().into()); @@ -92,13 +86,13 @@ fn test_notification() { message_id: 1, }, ] { - let serialized = notification.to_any(); - let deserialized = Notification::from_any(&serialized).unwrap(); + let message = notification.to_proto(); + let deserialized = Notification::from_proto(&message).unwrap(); assert_eq!(deserialized, notification); } // When notifications are serialized, the `kind` and `actor_id` fields are // stored separately, and do not appear redundantly in the JSON. let notification = Notification::ContactRequest { actor_id: 1 }; - assert_eq!(notification.to_any().content, "{}"); + assert_eq!(notification.to_proto().content, "{}"); }