From 1c0846b50c9e974ba77c222f77c69409da7902c3 Mon Sep 17 00:00:00 2001 From: suneettipirneni Date: Mon, 6 Feb 2023 12:44:08 -0500 Subject: [PATCH 1/3] refactor!(model, cache, gateway): Make `Presence::guild_id` an `Option` --- twilight-cache-inmemory/src/event/presence.rs | 10 +- twilight-cache-inmemory/src/model/presence.rs | 4 +- twilight-model/src/channel/thread/member.rs | 36 +--- twilight-model/src/gateway/event/mod.rs | 2 +- .../gateway/payload/incoming/member_chunk.rs | 175 +----------------- .../payload/incoming/thread_members_update.rs | 66 +------ twilight-model/src/gateway/presence/mod.rs | 160 +--------------- twilight-model/src/guild/mod.rs | 9 +- 8 files changed, 30 insertions(+), 432 deletions(-) diff --git a/twilight-cache-inmemory/src/event/presence.rs b/twilight-cache-inmemory/src/event/presence.rs index 54873eba6cc..d03d3ae2cb3 100644 --- a/twilight-cache-inmemory/src/event/presence.rs +++ b/twilight-cache-inmemory/src/event/presence.rs @@ -34,7 +34,11 @@ impl UpdateCache for PresenceUpdate { let presence = CachedPresence::from_model(self.0.clone()); - cache.cache_presence(self.guild_id, presence); + // Presence updates should always contain a guild ID as per the API docs. + cache.cache_presence( + self.guild_id.expect("Presence update without guild id"), + presence, + ); } } @@ -54,7 +58,7 @@ mod tests { fn presence_update() { let cache = InMemoryCache::new(); - let guild_id = Id::new(1); + let guild_id = Some(Id::new(1)); let user_id = Id::new(1); let payload = PresenceUpdate(Presence { @@ -74,7 +78,7 @@ mod tests { assert_eq!(1, cache.guild_presences.len()); assert!(cache .guild_presences - .get(&guild_id) + .get(&guild_id.unwrap()) .unwrap() .contains(&user_id)); } diff --git a/twilight-cache-inmemory/src/model/presence.rs b/twilight-cache-inmemory/src/model/presence.rs index 8f3a0aaa1d3..149266749f2 100644 --- a/twilight-cache-inmemory/src/model/presence.rs +++ b/twilight-cache-inmemory/src/model/presence.rs @@ -14,7 +14,7 @@ use twilight_model::{ pub struct CachedPresence { pub(crate) activities: Vec, pub(crate) client_status: ClientStatus, - pub(crate) guild_id: Id, + pub(crate) guild_id: Option>, pub(crate) status: Status, pub(crate) user_id: Id, } @@ -31,7 +31,7 @@ impl CachedPresence { } /// ID of the guild. - pub const fn guild_id(&self) -> Id { + pub const fn guild_id(&self) -> Option> { self.guild_id } diff --git a/twilight-model/src/channel/thread/member.rs b/twilight-model/src/channel/thread/member.rs index 612fffcb9d7..846229dbfc2 100644 --- a/twilight-model/src/channel/thread/member.rs +++ b/twilight-model/src/channel/thread/member.rs @@ -1,8 +1,8 @@ use crate::{ - gateway::presence::{Presence, PresenceIntermediary}, + gateway::presence::Presence, guild::Member, id::{ - marker::{ChannelMarker, GuildMarker, UserMarker}, + marker::{ChannelMarker, UserMarker}, Id, }, util::Timestamp, @@ -24,38 +24,6 @@ pub struct ThreadMember { pub user_id: Option>, } -/// Version of [`ThreadMember`], but without a guild ID in the -/// [`Self::member`] field. -#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq)] -pub(crate) struct ThreadMemberIntermediary { - // Values currently unknown and undocumented. - pub flags: u64, - #[serde(skip_serializing_if = "Option::is_none")] - pub id: Option>, - pub join_timestamp: Timestamp, - #[serde(skip_serializing_if = "Option::is_none")] - pub member: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub presence: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub user_id: Option>, -} - -impl ThreadMemberIntermediary { - /// Inject a guild ID into a thread member intermediary - pub fn into_thread_member(self, guild_id: Id) -> ThreadMember { - let presence = self.presence.map(|p| p.into_presence(guild_id)); - ThreadMember { - flags: self.flags, - id: self.id, - join_timestamp: self.join_timestamp, - member: self.member, - presence, - user_id: self.user_id, - } - } -} - #[cfg(test)] mod tests { use super::ThreadMember; diff --git a/twilight-model/src/gateway/event/mod.rs b/twilight-model/src/gateway/event/mod.rs index bcb282caa25..71903eb8dc6 100644 --- a/twilight-model/src/gateway/event/mod.rs +++ b/twilight-model/src/gateway/event/mod.rs @@ -214,7 +214,7 @@ impl Event { Event::MemberRemove(e) => Some(e.guild_id), Event::MemberUpdate(e) => Some(e.guild_id), Event::MessageCreate(e) => e.0.guild_id, - Event::PresenceUpdate(e) => Some(e.0.guild_id), + Event::PresenceUpdate(e) => e.0.guild_id, Event::ReactionAdd(e) => e.0.guild_id, Event::ReactionRemove(e) => e.0.guild_id, Event::ReactionRemoveAll(e) => e.guild_id, diff --git a/twilight-model/src/gateway/payload/incoming/member_chunk.rs b/twilight-model/src/gateway/payload/incoming/member_chunk.rs index 73e7365be5e..96c09c008ab 100644 --- a/twilight-model/src/gateway/payload/incoming/member_chunk.rs +++ b/twilight-model/src/gateway/payload/incoming/member_chunk.rs @@ -1,18 +1,15 @@ +use serde::{Deserialize, Serialize}; + use crate::{ - gateway::presence::{Presence, PresenceListDeserializer}, + gateway::presence::Presence, guild::Member, id::{ marker::{GuildMarker, UserMarker}, Id, }, }; -use serde::{ - de::{Deserializer, Error as DeError, IgnoredAny, MapAccess, Visitor}, - Deserialize, Serialize, -}; -use std::fmt::{Formatter, Result as FmtResult}; -#[derive(Clone, Debug, Eq, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] pub struct MemberChunk { pub chunk_count: u32, pub chunk_index: u32, @@ -25,164 +22,6 @@ pub struct MemberChunk { pub presences: Vec, } -#[derive(Debug, Deserialize)] -#[serde(field_identifier, rename_all = "snake_case")] -enum Field { - ChunkCount, - ChunkIndex, - GuildId, - Members, - Nonce, - NotFound, - Presences, -} - -struct MemberChunkVisitor; - -impl<'de> Visitor<'de> for MemberChunkVisitor { - type Value = MemberChunk; - - fn expecting(&self, f: &mut Formatter<'_>) -> FmtResult { - f.write_str("struct MemberChunk") - } - - #[allow(clippy::too_many_lines)] - fn visit_map>(self, mut map: V) -> Result { - let mut chunk_count = None; - let mut chunk_index = None; - let mut guild_id = None; - let mut members = None; - let mut nonce = None; - let mut not_found = None; - let mut presences = None; - - let span = tracing::trace_span!("deserializing member chunk"); - let _span_enter = span.enter(); - - loop { - let span_child = tracing::trace_span!("iterating over element"); - let _span_child_enter = span_child.enter(); - - let key = match map.next_key() { - Ok(Some(key)) => { - tracing::trace!(?key, "found key"); - - key - } - Ok(None) => break, - Err(why) => { - // Encountered when we run into an unknown key. - map.next_value::()?; - - tracing::trace!("ran into an unknown key: {why:?}"); - - continue; - } - }; - - match key { - Field::ChunkCount => { - if chunk_count.is_some() { - return Err(DeError::duplicate_field("chunk_count")); - } - - chunk_count = Some(map.next_value()?); - } - Field::ChunkIndex => { - if chunk_index.is_some() { - return Err(DeError::duplicate_field("chunk_index")); - } - - chunk_index = Some(map.next_value()?); - } - Field::GuildId => { - if guild_id.is_some() { - return Err(DeError::duplicate_field("guild_id")); - } - - guild_id = Some(map.next_value()?); - } - Field::Members => { - if members.is_some() { - return Err(DeError::duplicate_field("members")); - } - - members = Some(map.next_value()?); - } - Field::Nonce => { - if nonce.is_some() { - return Err(DeError::duplicate_field("nonce")); - } - - nonce = Some(map.next_value()?); - } - Field::NotFound => { - if not_found.is_some() { - return Err(DeError::duplicate_field("not_found")); - } - - not_found = Some(map.next_value()?); - } - Field::Presences => { - if presences.is_some() { - return Err(DeError::duplicate_field("presences")); - } - - let deserializer = PresenceListDeserializer::new(Id::new(1)); - - presences = Some(map.next_value_seed(deserializer)?); - } - } - } - - let chunk_count = chunk_count.ok_or_else(|| DeError::missing_field("chunk_count"))?; - let chunk_index = chunk_index.ok_or_else(|| DeError::missing_field("chunk_index"))?; - let guild_id = guild_id.ok_or_else(|| DeError::missing_field("guild_id"))?; - let members = members.ok_or_else(|| DeError::missing_field("members"))?; - let not_found = not_found.unwrap_or_default(); - let mut presences = presences.unwrap_or_default(); - - tracing::trace!( - %chunk_count, - %chunk_index, - ?guild_id, - ?members, - ?not_found, - ?presences, - ); - - for presence in &mut presences { - presence.guild_id = guild_id; - } - - Ok(MemberChunk { - chunk_count, - chunk_index, - guild_id, - members, - nonce, - not_found, - presences, - }) - } -} - -impl<'de> Deserialize<'de> for MemberChunk { - fn deserialize>(deserializer: D) -> Result { - const FIELDS: &[&str] = &[ - "chunk_count", - "chunk_index", - "guild_id", - "members", - "nonce", - "not_found", - "presences", - ]; - - deserializer.deserialize_struct("MemberChunk", FIELDS, MemberChunkVisitor) - } -} - #[cfg(test)] mod tests { use super::super::MemberChunk; @@ -439,7 +278,7 @@ mod tests { mobile: None, web: Some(Status::Online), }, - guild_id: Id::new(1), + guild_id: Some(Id::new(1)), status: Status::Online, user: UserOrId::UserId { id: Id::new(2) }, }, @@ -450,7 +289,7 @@ mod tests { mobile: None, web: Some(Status::Online), }, - guild_id: Id::new(1), + guild_id: Some(Id::new(1)), status: Status::Online, user: UserOrId::UserId { id: Id::new(3) }, }, @@ -461,7 +300,7 @@ mod tests { mobile: None, web: None, }, - guild_id: Id::new(1), + guild_id: Some(Id::new(1)), status: Status::DoNotDisturb, user: UserOrId::UserId { id: Id::new(5) }, }, diff --git a/twilight-model/src/gateway/payload/incoming/thread_members_update.rs b/twilight-model/src/gateway/payload/incoming/thread_members_update.rs index f61e82d786f..5850f514e45 100644 --- a/twilight-model/src/gateway/payload/incoming/thread_members_update.rs +++ b/twilight-model/src/gateway/payload/incoming/thread_members_update.rs @@ -1,17 +1,13 @@ use crate::{ - channel::thread::member::{ThreadMember, ThreadMemberIntermediary}, + channel::thread::member::ThreadMember, id::{ marker::{ChannelMarker, GuildMarker, UserMarker}, Id, }, }; -use serde::{ - de::{value::MapAccessDeserializer, MapAccess, Visitor}, - Deserialize, Deserializer, Serialize, -}; -use std::fmt::{Formatter, Result as FmtResult}; +use serde::{Deserialize, Serialize}; -#[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)] pub struct ThreadMembersUpdate { /// List of thread members. /// @@ -30,60 +26,6 @@ pub struct ThreadMembersUpdate { pub removed_member_ids: Vec>, } -impl<'de> Deserialize<'de> for ThreadMembersUpdate { - fn deserialize>(deserializer: D) -> Result { - deserializer.deserialize_map(ThreadMembersUpdateVisitor) - } -} - -#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq)] -struct ThreadMembersUpdateIntermediary { - /// ThreadMembers without the guild ID. - #[serde(default)] - pub added_members: Vec, - pub guild_id: Id, - pub id: Id, - pub member_count: i32, - #[serde(default)] - pub removed_member_ids: Vec>, -} - -impl ThreadMembersUpdateIntermediary { - fn into_thread_members_update(self) -> ThreadMembersUpdate { - let guild_id = self.guild_id; - let added_members = self - .added_members - .into_iter() - .map(|tm| tm.into_thread_member(guild_id)) - .collect(); - - ThreadMembersUpdate { - added_members, - guild_id, - id: self.id, - member_count: self.member_count, - removed_member_ids: self.removed_member_ids, - } - } -} - -struct ThreadMembersUpdateVisitor; - -impl<'de> Visitor<'de> for ThreadMembersUpdateVisitor { - type Value = ThreadMembersUpdate; - - fn expecting(&self, f: &mut Formatter<'_>) -> FmtResult { - f.write_str("struct ThreadMembersUpdate") - } - - fn visit_map>(self, map: A) -> Result { - let deser = MapAccessDeserializer::new(map); - let update = ThreadMembersUpdateIntermediary::deserialize(deser)?; - - Ok(update.into_thread_members_update()) - } -} - #[cfg(test)] mod tests { use super::ThreadMembersUpdate; @@ -170,7 +112,7 @@ mod tests { mobile: None, web: None, }, - guild_id: Id::new(2), + guild_id: Some(Id::new(2)), status: Status::Online, user: UserOrId::UserId { id: Id::new(3) }, }; diff --git a/twilight-model/src/gateway/presence/mod.rs b/twilight-model/src/gateway/presence/mod.rs index 1c1ce303900..4309d1e40f5 100644 --- a/twilight-model/src/gateway/presence/mod.rs +++ b/twilight-model/src/gateway/presence/mod.rs @@ -27,20 +27,14 @@ use crate::{ }, user::User, }; -use serde::{ - de::{ - value::MapAccessDeserializer, DeserializeSeed, Deserializer, MapAccess, SeqAccess, Visitor, - }, - Deserialize, Serialize, -}; -use std::fmt::{Formatter, Result as FmtResult}; +use serde::{Deserialize, Serialize}; #[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)] pub struct Presence { #[serde(default)] pub activities: Vec, pub client_status: ClientStatus, - pub guild_id: Id, + pub guild_id: Option>, pub status: Status, pub user: UserOrId, } @@ -62,120 +56,10 @@ impl UserOrId { } } -#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq)] -pub(crate) struct PresenceIntermediary { - #[serde(default)] - pub activities: Vec, - pub client_status: ClientStatus, - pub guild_id: Option>, - pub nick: Option, - pub status: Status, - pub user: UserOrId, -} - -impl PresenceIntermediary { - /// Inject guild ID into presence if not already present. - pub fn into_presence(self, guild_id: Id) -> Presence { - Presence { - activities: self.activities, - client_status: self.client_status, - guild_id: self.guild_id.unwrap_or(guild_id), - status: self.status, - user: self.user, - } - } -} - -struct PresenceVisitor(Id); - -impl<'de> Visitor<'de> for PresenceVisitor { - type Value = Presence; - - fn expecting(&self, f: &mut Formatter<'_>) -> FmtResult { - f.write_str("Presence struct") - } - - fn visit_map>(self, map: M) -> Result { - let deser = MapAccessDeserializer::new(map); - let presence = PresenceIntermediary::deserialize(deser)?; - - Ok(Presence { - activities: presence.activities, - client_status: presence.client_status, - guild_id: presence.guild_id.unwrap_or(self.0), - status: presence.status, - user: presence.user, - }) - } -} - -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct PresenceDeserializer(Id); - -impl PresenceDeserializer { - /// Create a new deserializer for a presence when you know the guild ID but - /// the payload probably doesn't contain it. - pub const fn new(guild_id: Id) -> Self { - Self(guild_id) - } -} - -impl<'de> DeserializeSeed<'de> for PresenceDeserializer { - type Value = Presence; - - fn deserialize>(self, deserializer: D) -> Result { - deserializer.deserialize_map(PresenceVisitor(self.0)) - } -} - -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct PresenceListDeserializer(Id); - -impl PresenceListDeserializer { - /// Create a new deserializer for a map of presences when you know the - /// Guild ID but the payload probably doesn't contain it. - pub const fn new(guild_id: Id) -> Self { - Self(guild_id) - } -} - -struct PresenceListDeserializerVisitor(Id); - -impl<'de> Visitor<'de> for PresenceListDeserializerVisitor { - type Value = Vec; - - fn expecting(&self, f: &mut Formatter<'_>) -> FmtResult { - f.write_str("a sequence of presences") - } - - fn visit_seq>(self, mut seq: S) -> Result { - let mut list = seq.size_hint().map_or_else(Vec::new, Vec::with_capacity); - - while let Some(presence) = seq.next_element_seed(PresenceDeserializer(self.0))? { - list.push(presence); - } - - Ok(list) - } -} - -impl<'de> DeserializeSeed<'de> for PresenceListDeserializer { - type Value = Vec; - - fn deserialize>(self, deserializer: D) -> Result { - deserializer.deserialize_any(PresenceListDeserializerVisitor(self.0)) - } -} - #[cfg(test)] mod tests { - use super::{ - Activity, ActivityEmoji, ActivityType, ClientStatus, Presence, PresenceListDeserializer, - Status, UserOrId, - }; + use super::{Activity, ActivityEmoji, ActivityType, ClientStatus, Presence, Status, UserOrId}; use crate::id::Id; - use serde::de::DeserializeSeed; - use serde_json::Deserializer; use serde_test::Token; #[test] @@ -210,7 +94,7 @@ mod tests { mobile: None, web: None, }, - guild_id: Id::new(2), + guild_id: Some(Id::new(2)), status: Status::Online, user: UserOrId::UserId { id: Id::new(1) }, }; @@ -287,40 +171,4 @@ mod tests { ], ); } - - // Test that presences through the deserializer are given a default guild ID - // if they have none. - // - // Can't test seeded deserializers with serde_test. - #[test] - fn presence_map_guild_id_default() { - let input = r#"[{ - "user": { - "id": "1" - }, - "status": "online", - "client_status": { - "desktop": "online" - }, - "activities": [] - }]"#; - - let expected = Vec::from([Presence { - activities: vec![], - client_status: ClientStatus { - desktop: Some(Status::Online), - mobile: None, - web: None, - }, - guild_id: Id::new(2), - status: Status::Online, - user: UserOrId::UserId { id: Id::new(1) }, - }]); - - let mut json_deserializer = Deserializer::from_str(input); - let deserializer = PresenceListDeserializer::new(Id::new(2)); - let actual = deserializer.deserialize(&mut json_deserializer).unwrap(); - - assert_eq!(actual, expected); - } } diff --git a/twilight-model/src/guild/mod.rs b/twilight-model/src/guild/mod.rs index 65f0aa570f7..64a553b7094 100644 --- a/twilight-model/src/guild/mod.rs +++ b/twilight-model/src/guild/mod.rs @@ -57,7 +57,6 @@ pub use self::{ verification_level::VerificationLevel, widget::GuildWidget, }; -use super::gateway::presence::PresenceListDeserializer; use crate::{ channel::{message::sticker::Sticker, Channel, StageInstance}, gateway::presence::Presence, @@ -250,7 +249,7 @@ impl<'de> Deserialize<'de> for Guild { let mut premium_progress_bar_enabled = None; let mut premium_subscription_count = None::>; let mut premium_tier = None; - let mut presences = None; + let mut presences = None::>; let mut public_updates_channel_id = None::>; let mut roles = None; let mut splash = None::>; @@ -525,9 +524,7 @@ impl<'de> Deserialize<'de> for Guild { return Err(DeError::duplicate_field("presences")); } - let deserializer = PresenceListDeserializer::new(Id::new(1)); - - presences = Some(map.next_value_seed(deserializer)?); + presences = Some(map.next_value()?); } Field::PublicUpdatesChannelId => { if public_updates_channel_id.is_some() { @@ -753,7 +750,7 @@ impl<'de> Deserialize<'de> for Guild { } for presence in &mut presences { - presence.guild_id = id; + presence.guild_id = Some(id); } for thread in &mut threads { From 10710e732aecb4e45829afb59d1fe7bfb8a81493 Mon Sep 17 00:00:00 2001 From: suneettipirneni Date: Tue, 7 Feb 2023 09:24:05 -0500 Subject: [PATCH 2/3] refactor: only cache if guild id is present --- twilight-cache-inmemory/src/event/presence.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/twilight-cache-inmemory/src/event/presence.rs b/twilight-cache-inmemory/src/event/presence.rs index d03d3ae2cb3..c00c046289f 100644 --- a/twilight-cache-inmemory/src/event/presence.rs +++ b/twilight-cache-inmemory/src/event/presence.rs @@ -34,11 +34,9 @@ impl UpdateCache for PresenceUpdate { let presence = CachedPresence::from_model(self.0.clone()); - // Presence updates should always contain a guild ID as per the API docs. - cache.cache_presence( - self.guild_id.expect("Presence update without guild id"), - presence, - ); + if let Some(guild_id) = self.guild_id { + cache.cache_presence(guild_id, presence); + } } } From 4680cf8be37b8fdd44a993d3c3983304517583bc Mon Sep 17 00:00:00 2001 From: suneettipirneni Date: Tue, 7 Feb 2023 12:33:41 -0500 Subject: [PATCH 3/3] test: fix tests --- twilight-model/src/gateway/payload/incoming/member_chunk.rs | 1 + .../src/gateway/payload/incoming/thread_members_update.rs | 6 +++--- twilight-model/src/gateway/presence/mod.rs | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/twilight-model/src/gateway/payload/incoming/member_chunk.rs b/twilight-model/src/gateway/payload/incoming/member_chunk.rs index 96c09c008ab..e73d780b488 100644 --- a/twilight-model/src/gateway/payload/incoming/member_chunk.rs +++ b/twilight-model/src/gateway/payload/incoming/member_chunk.rs @@ -17,6 +17,7 @@ pub struct MemberChunk { pub members: Vec, #[serde(default, skip_serializing_if = "Option::is_none")] pub nonce: Option, + #[serde(default)] pub not_found: Vec>, #[serde(default)] pub presences: Vec, diff --git a/twilight-model/src/gateway/payload/incoming/thread_members_update.rs b/twilight-model/src/gateway/payload/incoming/thread_members_update.rs index 5850f514e45..c7718f62901 100644 --- a/twilight-model/src/gateway/payload/incoming/thread_members_update.rs +++ b/twilight-model/src/gateway/payload/incoming/thread_members_update.rs @@ -138,13 +138,13 @@ mod tests { &value, &[ Token::Struct { - name: "ThreadMemberUpdate", + name: "ThreadMembersUpdate", len: 6, }, Token::Str("added_members"), Token::Seq { len: Some(1) }, Token::Struct { - name: "ThreadMemberIntermediary", + name: "ThreadMember", len: 6, }, Token::Str("flags"), @@ -213,7 +213,7 @@ mod tests { Token::Str("presence"), Token::Some, Token::Struct { - name: "PresenceIntermediary", + name: "Presence", len: 5, }, Token::Str("activities"), diff --git a/twilight-model/src/gateway/presence/mod.rs b/twilight-model/src/gateway/presence/mod.rs index 4309d1e40f5..afb3cfbfaf4 100644 --- a/twilight-model/src/gateway/presence/mod.rs +++ b/twilight-model/src/gateway/presence/mod.rs @@ -115,6 +115,7 @@ mod tests { Token::Str("1"), Token::StructEnd, Token::Str("guild_id"), + Token::Some, Token::NewtypeStruct { name: "Id" }, Token::Str("2"), Token::Str("status"),