Skip to content

Commit

Permalink
Remove client id from timestamp-based chathistory.
Browse files Browse the repository at this point in the history 
Other simplifications and clarifications suggested by review.
  • Loading branch information
@andymandias
andymandias committed May 17, 2024
1 parent 5e77c0f commit cb9b86c
Show file tree
Hide file tree
Showing 7 changed files with 108 additions and 167 deletions.
132 changes: 64 additions & 68 deletions data/src/client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ impl Client {

if matches!(
message_reference,
MessageReference::Timestamp(_, _)
MessageReference::Timestamp(_)
) {
finished
.events
Expand Down Expand Up @@ -346,68 +346,70 @@ impl Client {
return None;
}
_ if batch_tag.is_some() => {
let events = if let Some(target) = batch_tag
.as_ref()
.and_then(|batch| self.batches.get(batch))
.and_then(|batch| batch.chathistory_target.clone())
{
if let Some(ChatHistoryRequest {
let events = if let Some((
ChatHistoryRequest {
subcommand,
message_reference,
..
}) = self.chathistory_requests.get(&target)
{
if Some(User::from(Nick::from("HistServ"))) == message.user() {
// HistServ provides event-playback without event-playback
// which would require client-side parsing to map appropriately.
// Avoid that complexity by only providing that functionality
// via event-playback.
vec![]
} else {
match &message.command {
Command::NICK(_) => {
let target = message::Target::Channel {
channel: target,
source: source::Source::Server(None),
};

vec![Event::ChatHistoryWithTarget(
message,
self.nickname().to_owned(),
target,
subcommand.clone(),
message_reference.clone(),
)]
}
Command::QUIT(_) => {
let target = message::Target::Channel {
channel: target,
source: source::Source::Server(Some(source::Server::new(
source::server::Kind::Quit,
message
.user()
.map(|user| Nick::from(user.nickname().as_ref())),
))),
};

vec![Event::ChatHistoryWithTarget(
message,
self.nickname().to_owned(),
target,
subcommand.clone(),
message_reference.clone(),
)]
}
_ => vec![Event::ChatHistorySingle(
},
target,
)) = batch_tag
.as_ref()
.and_then(|batch| self.batches.get(batch))
.and_then(|batch| batch.chathistory_target.clone())
.and_then(|target| {
self.chathistory_requests
.get(&target)
.map(|chathistory_request| (chathistory_request, target))
}) {
if Some(User::from(Nick::from("HistServ"))) == message.user() {
// HistServ provides event-playback without event-playback
// which would require client-side parsing to map appropriately.
// Avoid that complexity by only providing that functionality
// via event-playback.
vec![]
} else {
match &message.command {
Command::NICK(_) => {
let target = message::Target::Channel {
channel: target,
source: source::Source::Server(None),
};

vec![Event::ChatHistoryWithTarget(
message,
self.nickname().to_owned(),
target,
subcommand.clone(),
message_reference.clone(),
)]
}
Command::QUIT(_) => {
let target = message::Target::Channel {
channel: target,
source: source::Source::Server(Some(source::Server::new(
source::server::Kind::Quit,
message
.user()
.map(|user| Nick::from(user.nickname().as_ref())),
))),
};

vec![Event::ChatHistoryWithTarget(
message,
self.nickname().to_owned(),
target,
subcommand.clone(),
message_reference.clone(),
)],
)]
}
_ => vec![Event::ChatHistorySingle(
message,
self.nickname().to_owned(),
subcommand.clone(),
message_reference.clone(),
)],
}
} else {
self.handle(message, context)?
}
} else {
self.handle(message, context)?
Expand Down Expand Up @@ -485,10 +487,10 @@ impl Client {
requested.push("batch");

// We require batch for our chathistory support
let requesting_chathistory = if contains_starting_with("chathistory") {
let requesting_chathistory = if contains("chathistory") {
requested.push("chathistory");
true
} else if contains_starting_with("draft/chathistory") {
} else if contains("draft/chathistory") {
requested.push("draft/chathistory");
true
} else {
Expand Down Expand Up @@ -578,9 +580,6 @@ impl Client {

let newly_contains = |s| new_caps.iter().any(|cap| cap == s);

let newly_contains_starting_with =
|s| new_caps.iter().any(|cap| cap.starts_with(s));

let contains = |s| self.listed_caps.iter().any(|cap| cap == s);

if newly_contains("invite-notify") {
Expand All @@ -604,10 +603,10 @@ impl Client {
}

// We require batch for our chathistory support
let requesting_chathistory = if newly_contains_starting_with("chathistory") {
let requesting_chathistory = if newly_contains("chathistory") {
requested.push("chathistory");
true
} else if newly_contains_starting_with("draft/chathistory") {
} else if newly_contains("draft/chathistory") {
requested.push("draft/chathistory");
true
} else {
Expand Down Expand Up @@ -1252,14 +1251,14 @@ impl Client {
match subcommand {
ChatHistorySubcommand::Latest(_) => {
let command_message_reference = match message_reference {
MessageReference::Timestamp(server_time, _) => {
MessageReference::Timestamp(server_time) => {
if let Some(fuzzed_server_time) =
TimeDelta::try_seconds(isupport::CHATHISTORY_FUZZ_SECONDS)
.and_then(|time_delta| {
server_time.checked_sub_signed(time_delta)
})
{
MessageReference::Timestamp(fuzzed_server_time, ":".to_string())
MessageReference::Timestamp(fuzzed_server_time)
} else {
message_reference
}
Expand All @@ -1282,17 +1281,14 @@ impl Client {
}
ChatHistorySubcommand::Before => {
let command_message_reference = match message_reference {
MessageReference::Timestamp(reference_timestamp, _) => {
MessageReference::Timestamp(reference_timestamp) => {
if let Some(fuzzed_reference_timestamp) =
TimeDelta::try_seconds(isupport::CHATHISTORY_FUZZ_SECONDS)
.and_then(|time_delta| {
reference_timestamp.checked_add_signed(time_delta)
})
{
MessageReference::Timestamp(
fuzzed_reference_timestamp,
":".to_string(),
)
MessageReference::Timestamp(fuzzed_reference_timestamp)
} else {
message_reference
}
Expand Down
20 changes: 6 additions & 14 deletions data/src/history.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,21 +485,13 @@ fn is_referenceable_message(
) -> bool {
if matches!(message.target.source(), message::Source::Internal(_)) {
false
} else if matches!(
message_reference_type,
Some(isupport::MessageReferenceType::MessageId)
) {
message.id.is_some()
} else {
match message_reference_type {
Some(isupport::MessageReferenceType::MessageId) => message
.id
.as_ref()
.is_some_and(|message_id| !message_id.starts_with(':')),
Some(isupport::MessageReferenceType::Timestamp) => message
.id
.as_ref()
.is_some_and(|message_id| message_id.starts_with(':') && message_id != ":"),
None => message
.id
.as_ref()
.is_some_and(|message_id| message_id != ":"),
}
true
}
}

Expand Down
8 changes: 3 additions & 5 deletions data/src/isupport.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -650,15 +650,15 @@ pub struct CommandTargetLimit {

#[derive(Clone, Debug)]
pub enum MessageReference {
Timestamp(DateTime<Utc>, String),
Timestamp(DateTime<Utc>),
MessageId(String),
None,
}

impl fmt::Display for MessageReference {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
MessageReference::Timestamp(server_time, _) => write!(
MessageReference::Timestamp(server_time) => write!(
f,
"timestamp={}",
server_time.to_rfc3339_opts(SecondsFormat::Millis, true)
Expand All @@ -672,9 +672,7 @@ impl fmt::Display for MessageReference {
impl PartialEq<Message> for MessageReference {
fn eq(&self, other: &Message) -> bool {
match self {
MessageReference::Timestamp(server_time, client_id) => {
other.server_time == *server_time && other.id.as_deref() == Some(client_id.as_str())
}
MessageReference::Timestamp(server_time) => other.server_time == *server_time,
MessageReference::MessageId(id) => other.id.as_deref() == Some(id.as_str()),
MessageReference::None => false,
}
Expand Down
17 changes: 2 additions & 15 deletions data/src/message.rs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
use chrono::{DateTime, Utc};
use irc::proto;
use irc::proto::Command;
use seahash::SeaHasher;
use serde::{Deserialize, Serialize};
use std::hash::{Hash, Hasher};

pub use self::source::Source;
use crate::time::{self, Posix};
Expand All @@ -15,7 +13,7 @@ pub type Channel = String;
pub(crate) mod broadcast;
pub mod source;

#[derive(Debug, Clone, Hash)]
#[derive(Debug, Clone)]
pub struct Encoded(proto::Message);

impl Encoded {
Expand Down Expand Up @@ -99,10 +97,9 @@ impl Message {
our_nick: Nick,
config: &Config,
resolve_attributes: impl Fn(&User, &str) -> Option<User>,
generate_missing_id: bool,
) -> Option<Message> {
let server_time = server_time(&encoded);
let id = message_id(&encoded).or(generate_missing_id.then_some(client_id(&encoded)));
let id = message_id(&encoded);
let text = text(&encoded, &our_nick, config, &resolve_attributes)?;
let target = target(encoded, &our_nick, &resolve_attributes)?;

Expand Down Expand Up @@ -322,16 +319,6 @@ fn target(
}
}

pub fn client_id(message: &Encoded) -> String {
let mut hasher = SeaHasher::new();

message.hash(&mut hasher);

// Prefix hash with ':' fort client id, since that character
// is not allowed in message ids provided by servers
format!(":{:x}", hasher.finish())
}

pub fn message_id(message: &Encoded) -> Option<String> {
message
.tags
Expand Down
8 changes: 4 additions & 4 deletions irc/proto/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pub mod command;
pub mod format;
pub mod parse;

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Message {
pub tags: Vec<Tag>,
pub source: Option<Source>,
Expand All @@ -21,19 +21,19 @@ impl From<Command> for Message {
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Tag {
pub key: String,
pub value: Option<String>,
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum Source {
Server(String),
User(User),
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct User {
pub nickname: String,
pub username: Option<String>,
Expand Down
Loading

0 comments on commit cb9b86c

Please sign in to comment.