From 777e4281431a036eb663b5eec70f347b7425737d Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Thu, 17 Oct 2024 01:45:58 -0400 Subject: Retain deleted messages and channels temporarily, to preserve events for replay. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when a channel (message) was deleted, `hi` would send events to all _connected_ clients to inform them of the deletion, then delete all memory of the channel (message). Any disconnected client, on reconnecting, would not receive the deletion event, and would de-synch with the service. The creation events were also immediately retconned out of the event stream, as well. With this change, `hi` keeps a record of deleted channels (messages). When replaying events, these records are used to replay the deletion event. After 7 days, the retained data is deleted, both to keep storage under control and to conform to users' expectations that deleted means gone. To match users' likely intuitions about what deletion does, deleting a channel (message) _does_ immediately delete some of its associated data. Channels' names are blanked, and messages' bodies are also blanked. When the event stream is replayed, the original channel.created (message.sent) event is "tombstoned", with an additional `deleted_at` field to inform clients. The included client does not use this field, at least yet. The migration is, once again, screamingingly complicated due to sqlite's limited ALTER TABLE … ALTER COLUMN support. This change also contains capabilities that would allow the API to return 410 Gone for deleted channels or messages, instead of 404. I did experiment with this, but it's tricky to do pervasively, especially since most app-level interfaces return an `Option` or `Option`. Redesigning these to return either `Ok(Channel)` (`Ok(Message)`) or `Err(Error::NotFound)` or `Err(Error::Deleted)` is more work than I wanted to take on for this change, and the utility of 410 Gone responses is not obvious to me. We have other, more pressing API design warts to address. --- src/channel/routes/channel/delete.rs | 2 +- src/channel/routes/channel/test.rs | 6 +- src/channel/routes/test.rs | 106 +++++++++++++++++++++++++++++------ 3 files changed, 93 insertions(+), 21 deletions(-) (limited to 'src/channel/routes') diff --git a/src/channel/routes/channel/delete.rs b/src/channel/routes/channel/delete.rs index efac0c0..5f40ddf 100644 --- a/src/channel/routes/channel/delete.rs +++ b/src/channel/routes/channel/delete.rs @@ -32,7 +32,7 @@ impl IntoResponse for Error { let Self(error) = self; #[allow(clippy::match_wildcard_for_single_variants)] match error { - app::Error::NotFound(_) => NotFound(error).into_response(), + app::Error::NotFound(_) | app::Error::Deleted(_) => NotFound(error).into_response(), other => Internal::from(other).into_response(), } } diff --git a/src/channel/routes/channel/test.rs b/src/channel/routes/channel/test.rs index bc02b20..c6541cd 100644 --- a/src/channel/routes/channel/test.rs +++ b/src/channel/routes/channel/test.rs @@ -4,7 +4,7 @@ use futures::stream::StreamExt; use super::post; use crate::{ channel, - event::{self, Sequenced}, + event::Sequenced, message::{self, app::SendError}, test::fixtures::{self, future::Immediately as _}, }; @@ -45,7 +45,7 @@ async fn messages_in_order() { .subscribe(None) .await .expect("subscribing to a valid channel") - .filter(fixtures::filter::messages()) + .filter_map(fixtures::message::events) .take(requests.len()); let events = events.collect::>().immediately().await; @@ -54,7 +54,7 @@ async fn messages_in_order() { assert_eq!(*sent_at, event.at()); assert!(matches!( event, - event::Event::Message(message::Event::Sent(event)) + message::Event::Sent(event) if event.message.sender == sender.id && event.message.body == message )); diff --git a/src/channel/routes/test.rs b/src/channel/routes/test.rs index 81f1465..ffd8484 100644 --- a/src/channel/routes/test.rs +++ b/src/channel/routes/test.rs @@ -1,10 +1,11 @@ +use std::future; + use axum::extract::{Json, State}; use futures::stream::StreamExt as _; use super::post; use crate::{ - channel::{self, app}, - event, + channel::app, test::fixtures::{self, future::Immediately as _}, }; @@ -19,29 +20,35 @@ async fn new_channel() { let name = fixtures::channel::propose(); let request = post::Request { name: name.clone() }; - let Json(response_channel) = - post::handler(State(app.clone()), creator, fixtures::now(), Json(request)) - .await - .expect("new channel in an empty app"); + let Json(response) = post::handler(State(app.clone()), creator, fixtures::now(), Json(request)) + .await + .expect("creating a channel in an empty app succeeds"); // Verify the structure of the response - assert_eq!(name, response_channel.name); + assert_eq!(name, response.name); // Verify the semantics let snapshot = app.boot().snapshot().await.expect("boot always succeeds"); - assert!(snapshot - .channels - .iter() - .any(|channel| channel.name == response_channel.name && channel.id == response_channel.id)); + assert!(snapshot.channels.iter().any(|channel| channel == &response)); + + let channel = app + .channels() + .get(&response.id) + .await + .expect("searching for channels by ID never fails") + .expect("the newly-created channel exists"); + assert_eq!(response, channel); let mut events = app .events() .subscribe(None) .await .expect("subscribing never fails") - .filter(fixtures::filter::created()); + .filter_map(fixtures::channel::events) + .filter_map(fixtures::channel::created) + .filter(|event| future::ready(event.channel == response)); let event = events .next() @@ -49,11 +56,7 @@ async fn new_channel() { .await .expect("creation event published"); - assert!(matches!( - event, - event::Event::Channel(channel::Event::Created(event)) - if event.channel == response_channel - )); + assert_eq!(event.channel, response); } #[tokio::test] @@ -81,3 +84,72 @@ async fn duplicate_name() { app::CreateError::DuplicateName(name) if channel.name == name )); } + +#[tokio::test] +async fn name_reusable_after_delete() { + // Set up the environment + + let app = fixtures::scratch_app().await; + let creator = fixtures::login::create(&app, &fixtures::now()).await; + let name = fixtures::channel::propose(); + + // Call the endpoint (first time) + + let request = post::Request { name: name.clone() }; + let Json(response) = post::handler( + State(app.clone()), + creator.clone(), + fixtures::now(), + Json(request), + ) + .await + .expect("new channel in an empty app"); + + // Delete the channel + + app.channels() + .delete(&response.id, &fixtures::now()) + .await + .expect("deleting a newly-created channel succeeds"); + + // Call the endpoint (second time) + + let request = post::Request { name: name.clone() }; + let Json(response) = post::handler(State(app.clone()), creator, fixtures::now(), Json(request)) + .await + .expect("new channel in an empty app"); + + // Verify the structure of the response + + assert_eq!(name, response.name); + + // Verify the semantics + + let snapshot = app.boot().snapshot().await.expect("boot always succeeds"); + assert!(snapshot.channels.iter().any(|channel| channel == &response)); + + let channel = app + .channels() + .get(&response.id) + .await + .expect("searching for channels by ID never fails") + .expect("the newly-created channel exists"); + assert_eq!(response, channel); + + let mut events = app + .events() + .subscribe(None) + .await + .expect("subscribing never fails") + .filter_map(fixtures::channel::events) + .filter_map(fixtures::channel::created) + .filter(|event| future::ready(event.channel == response)); + + let event = events + .next() + .immediately() + .await + .expect("creation event published"); + + assert_eq!(event.channel, response); +} -- cgit v1.2.3