From 7c61fdf668d847f94aaadbf51f090365310d31fd Mon Sep 17 00:00:00 2001 From: Tobias de Bruijn Date: Wed, 13 Mar 2024 15:40:06 +0100 Subject: [PATCH] Account metadata caching Signed-off-by: Tobias de Bruijn --- frontend/package.json | 2 +- server/chroma/Cargo.toml | 2 +- server/chroma/src/main.rs | 11 ++-- server/chroma/src/routes/appdata.rs | 3 +- server/chroma/src/routes/v1/album/delete.rs | 2 +- server/chroma/src/routes/v1/album/get.rs | 14 +++-- server/chroma/src/routes/v1/album/list.rs | 53 ++++++++++++++-- server/chroma/src/routes/v1/album/update.rs | 14 +++-- server/chroma/src/routes/v1/photo/create.rs | 9 ++- server/dal/src/database/album.rs | 67 ++++++++++++--------- server/dal/src/database/photo.rs | 6 +- 11 files changed, 125 insertions(+), 58 deletions(-) diff --git a/frontend/package.json b/frontend/package.json index 603b3d9..2040395 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "chroma", - "version": "0.1.20", + "version": "0.1.21", "private": true, "scripts": { "serve": "vue-cli-service serve --port 8008", diff --git a/server/chroma/Cargo.toml b/server/chroma/Cargo.toml index 2cccc45..e8eed18 100644 --- a/server/chroma/Cargo.toml +++ b/server/chroma/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "chroma" -version = "0.1.20" +version = "0.1.21" edition = "2021" [dependencies] diff --git a/server/chroma/src/main.rs b/server/chroma/src/main.rs index f3d6b2a..f1a38c9 100644 --- a/server/chroma/src/main.rs +++ b/server/chroma/src/main.rs @@ -1,10 +1,10 @@ extern crate core; use crate::config::Config; -use crate::routes::appdata::{AppData, SessionIdCache, WebData}; +use crate::routes::appdata::{AlbumIdCache, AppData, SessionIdCache, WebData}; use crate::routes::routable::Routable; use actix_cors::Cors; -use actix_web::{App, HttpServer}; +use actix_web::{web, App, HttpServer}; use cabbage::KoalaApi; use color_eyre::eyre::Error; use color_eyre::Result; @@ -76,12 +76,15 @@ async fn main() -> Result<()> { .wrap(Cors::permissive()) .wrap(TracingLogger::::new()) .app_data(WebData::new(appdata.clone())) - .app_data( + .app_data(web::Data::new( SessionIdCache::builder() .max_capacity(10000) .time_to_live(Duration::from_secs(30)) .build(), - ) + )) + .app_data(web::Data::new( + AlbumIdCache::builder().max_capacity(10000).build(), + )) .configure(routes::Router::configure) }) .bind(&format!( diff --git a/server/chroma/src/routes/appdata.rs b/server/chroma/src/routes/appdata.rs index c67bb06..352995e 100644 --- a/server/chroma/src/routes/appdata.rs +++ b/server/chroma/src/routes/appdata.rs @@ -2,12 +2,13 @@ use crate::config::Config; use crate::routes::authorization::Authorization; use actix_web::web; use cabbage::KoalaApi; -use dal::database::Database; +use dal::database::{Album, Database}; use dal::storage_engine::StorageEngine; use moka::future::Cache; pub type WebData = web::Data; pub type SessionIdCache = Cache; +pub type AlbumIdCache = Cache; #[derive(Debug, Clone)] pub struct AppData { diff --git a/server/chroma/src/routes/v1/album/delete.rs b/server/chroma/src/routes/v1/album/delete.rs index 22d8011..c3de5fa 100644 --- a/server/chroma/src/routes/v1/album/delete.rs +++ b/server/chroma/src/routes/v1/album/delete.rs @@ -49,7 +49,7 @@ pub async fn delete( .await?; } - album.delete().await?; + album.delete(&data.db).await?; Ok(Empty) } diff --git a/server/chroma/src/routes/v1/album/get.rs b/server/chroma/src/routes/v1/album/get.rs index bd0dbde..4149617 100644 --- a/server/chroma/src/routes/v1/album/get.rs +++ b/server/chroma/src/routes/v1/album/get.rs @@ -1,4 +1,4 @@ -use crate::routes::appdata::WebData; +use crate::routes::appdata::{AlbumIdCache, WebData}; use crate::routes::authorization::Authorization; use crate::routes::error::{Error, WebResult}; use actix_multiresponse::Payload; @@ -29,11 +29,15 @@ pub struct Query { pub async fn get( _: Authorization, data: WebData, + album_id_cache: web::Data, query: web::Query, ) -> WebResult> { - let album = Album::get_by_id(&data.db, &query.id) - .await? - .ok_or(Error::NotFound)?; + let album = match album_id_cache.get(&query.id).await { + Some(v) => v, + None => Album::get_by_id(&data.db, &query.id) + .await? + .ok_or(Error::NotFound)?, + }; // If the user requests that photos are not returned, return an empty list. let photos = match query.without_photos { @@ -91,7 +95,7 @@ pub async fn get( Ok(Payload(GetAlbumResponse { photos, album: Some(AlbumWithCoverPhoto { - album: Some(album.to_proto().await?), + album: Some(album.to_proto(&data.db).await?), cover_photo, }), })) diff --git a/server/chroma/src/routes/v1/album/list.rs b/server/chroma/src/routes/v1/album/list.rs index c66d7af..3c87054 100644 --- a/server/chroma/src/routes/v1/album/list.rs +++ b/server/chroma/src/routes/v1/album/list.rs @@ -1,4 +1,4 @@ -use crate::routes::appdata::WebData; +use crate::routes::appdata::{AlbumIdCache, WebData}; use crate::routes::authorization::Authorization; use crate::routes::error::{Error, WebResult}; use crate::routes::v1::PhotoQuality; @@ -7,7 +7,7 @@ use actix_web::web; use dal::database::{Album, Photo}; use dal::storage_engine::EngineType; use dal::DalError; -use futures::future::join_all; +use futures::future::{join_all, try_join_all}; use proto::{AlbumWithCoverPhoto, ListAlbumsResponse}; use serde::Deserialize; @@ -26,11 +26,54 @@ pub struct Query { /// - If something went wrong pub async fn list( auth: Authorization, + album_id_cache: web::Data, data: WebData, query: web::Query, ) -> WebResult> { - let mut albums = Album::list(&data.db).await?; + // Fetch only IDs, so we can grab the rest from cache + let ids = Album::list_ids(&data.db).await?; + // Fetch cached albums + let cached_albums = join_all(ids.into_iter().map(|f| { + let cache = &**album_id_cache; + async move { + let album = cache.get(&f).await; + (f, album) + } + })) + .await; + + // Fetch albums from database of which there is nothing cached + let fetched_albums = try_join_all( + cached_albums + .iter() + .filter(|(_, cached)| cached.is_none()) + .map(|(id, _)| Album::get_by_id(&data.db, id)), + ) + .await? + .into_iter() + .filter_map(|f| f) + .collect::>(); + + // Insert the newly fetched into the cache + join_all(fetched_albums.iter().map(|album| { + let cache = &**album_id_cache; + async move { cache.insert(album.id.clone(), album.clone()).await } + })) + .await; + + // Merge the two sets + let mut albums = vec![ + fetched_albums, + cached_albums + .into_iter() + .map(|(_, v)| v) + .filter_map(|v| v) + .collect::>(), + ] + .concat(); + + // Check if we should include draft albums let include_draft = auth.is_admin || auth .has_scope(&data.db, "nl.svsticky.chroma.album.list.draft") @@ -40,6 +83,7 @@ pub async fn list( albums.retain(|f| !f.is_draft); } + // Transform them all to the proto response let albums = join_all(albums.into_iter().map(|album| { let storage = data.storage.clone(); let database = data.db.clone(); @@ -47,6 +91,7 @@ pub async fn list( let include_cover_photo = query.include_cover_photo; async move { + // Fetch the cover photo if it is requested let cover_photo = if include_cover_photo { if let Some(id) = &album.cover_photo_id { let photo = Photo::get_by_id(&database, id).await?.unwrap(); @@ -71,7 +116,7 @@ pub async fn list( }; Ok(AlbumWithCoverPhoto { - album: Some(album.to_proto().await?), + album: Some(album.to_proto(&database).await?), cover_photo, }) } diff --git a/server/chroma/src/routes/v1/album/update.rs b/server/chroma/src/routes/v1/album/update.rs index 6560836..7cf668e 100644 --- a/server/chroma/src/routes/v1/album/update.rs +++ b/server/chroma/src/routes/v1/album/update.rs @@ -1,8 +1,9 @@ -use crate::routes::appdata::WebData; +use crate::routes::appdata::{AlbumIdCache, WebData}; use crate::routes::authorization::Authorization; use crate::routes::empty::Empty; use crate::routes::error::{Error, WebResult}; use actix_multiresponse::Payload; +use actix_web::web; use dal::database::{Album, Photo}; use proto::UpdateAlbumRequest; @@ -21,6 +22,7 @@ use proto::UpdateAlbumRequest; pub async fn update( auth: Authorization, data: WebData, + album_id_cache: web::Data, payload: Payload, ) -> WebResult { if !auth.is_admin @@ -48,7 +50,7 @@ pub async fn update( ))); } - album.update_name(name).await?; + album.update_name(name, &data.db).await?; } if let Some(cover_photo_id) = &payload.cover_photo_id { @@ -65,7 +67,7 @@ pub async fn update( ))); } - album.update_cover_photo(&photo).await?; + album.update_cover_photo(&photo, &data.db).await?; } if let Some(draft_settings) = &payload.draft_settings { @@ -76,16 +78,18 @@ pub async fn update( match draft_settings { proto::update_album_request::DraftSettings::SetDraft(v) if *v => { - album.set_draft().await?; + album.set_draft(&data.db).await?; } proto::update_album_request::DraftSettings::SetPublished(v) if *v => { album - .set_published(auth.to_dal_user_type(&data.db).await?) + .set_published(auth.to_dal_user_type(&data.db).await?, &data.db) .await?; } _ => {} } } + album_id_cache.insert(album.id.clone(), album).await; + Ok(Empty) } diff --git a/server/chroma/src/routes/v1/photo/create.rs b/server/chroma/src/routes/v1/photo/create.rs index a9c831e..84d7b18 100644 --- a/server/chroma/src/routes/v1/photo/create.rs +++ b/server/chroma/src/routes/v1/photo/create.rs @@ -46,7 +46,7 @@ pub async fn create( } // TODO Update actix-multiresponse to support moving out the payload, avoids another clone - let photo_id = image_pipeline(&data, payload.photo_data.clone(), &album).await?; + let photo_id = image_pipeline(&data, payload.photo_data.clone(), &album, &data.db).await?; Ok(Payload(CreatePhotoResponse { photo_id })) } @@ -57,7 +57,12 @@ pub async fn create( /// /// If any step in the pipeline fails #[instrument(skip(data, image))] -async fn image_pipeline(data: &WebData, image: Vec, album: &Album<'_>) -> WebResult { +async fn image_pipeline( + data: &WebData, + image: Vec, + album: &Album, + db: &Database, +) -> WebResult { // This pipeline modifies the image. The idea is that each 'step' outputs // a variable 'image', which the next step can then use. diff --git a/server/dal/src/database/album.rs b/server/dal/src/database/album.rs index 85bd734..9a7cd0f 100644 --- a/server/dal/src/database/album.rs +++ b/server/dal/src/database/album.rs @@ -6,8 +6,8 @@ use std::fmt; use std::fmt::Formatter; use time::OffsetDateTime; -pub struct Album<'a> { - db: &'a Database, +#[derive(Clone)] +pub struct Album { pub id: String, pub name: String, pub created_at: i64, @@ -19,7 +19,7 @@ pub struct Album<'a> { } // Manually impl debug as to not print the `db` field -impl fmt::Debug for Album<'_> { +impl fmt::Debug for Album { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { f.debug_struct("Album") .field("id", &self.id) @@ -62,9 +62,8 @@ enum _UserType { } impl _Album { - fn into_album(self, db: &Database) -> Album { + fn into_album(self) -> Album { Album { - db, id: self.id, name: self.name, created_at: self.created_at, @@ -84,7 +83,7 @@ impl _Album { } } -impl<'a> Album<'a> { +impl Album { pub const MAX_NAME_LENGTH: usize = 64; pub const ID_PREFIX: &'static str = "ALB_"; pub const MAX_ID_LEN: usize = 32; @@ -118,16 +117,16 @@ impl<'a> Album<'a> { }) } - pub async fn to_proto(self) -> DbResult { + pub async fn to_proto(self, db: &Database) -> DbResult { Ok(proto::Album { id: self.id, name: self.name, created_at: self.created_at, cover_photo_id: self.cover_photo_id, is_draft: self.is_draft, - created_by: Some(Self::user_type_to_proto(self.db, self.created_by).await?), + created_by: Some(Self::user_type_to_proto(db, self.created_by).await?), published_by: match self.published_by { - Some(published_by) => Some(Self::user_type_to_proto(self.db, published_by).await?), + Some(published_by) => Some(Self::user_type_to_proto(db, published_by).await?), None => None, }, published_at: self.published_at, @@ -135,11 +134,11 @@ impl<'a> Album<'a> { } pub async fn create( - db: &'a Database, + db: &Database, name: impl Into>, is_draft: bool, created_by: UserType, - ) -> DbResult> { + ) -> DbResult { let name = name.into(); let id = Self::generate_id(); let created_at = OffsetDateTime::now_utc().unix_timestamp(); @@ -176,7 +175,6 @@ impl<'a> Album<'a> { .await?; Ok(Self { - db, id, name: name.to_string(), created_at, @@ -188,41 +186,46 @@ impl<'a> Album<'a> { }) } - pub async fn get_by_id + Sync>( - db: &'a Database, - id: S, - ) -> DbResult>> { + pub async fn get_by_id + Sync>(db: &Database, id: S) -> DbResult> { let album: Option<_Album> = sqlx::query_as("SELECT * FROM album_metadata WHERE id = $1") .bind(id.as_ref()) .fetch_optional(&**db) .await?; - Ok(album.map(|x| x.into_album(db))) + Ok(album.map(|x| x.into_album())) } - pub async fn update_cover_photo(&mut self, cover_photo: &Photo<'_>) -> DbResult<()> { + pub async fn update_cover_photo( + &mut self, + cover_photo: &Photo<'_>, + db: &Database, + ) -> DbResult<()> { sqlx::query("UPDATE album_metadata SET cover_photo_id = $1 WHERE id = $2") .bind(&cover_photo.id) .bind(&self.id) - .execute(&**self.db) + .execute(&**db) .await?; self.cover_photo_id = Some(cover_photo.id.clone()); Ok(()) } - pub async fn update_name(&mut self, new_name: impl Into>) -> DbResult<()> { + pub async fn update_name( + &mut self, + new_name: impl Into>, + db: &Database, + ) -> DbResult<()> { let new_name = new_name.into(); sqlx::query("UPDATE album_metadata SET name = $1 WHERE id = $2") .bind(&new_name) .bind(&self.id) - .execute(&**self.db) + .execute(&**db) .await?; self.name = new_name.to_string(); Ok(()) } - pub async fn set_published(&mut self, published_by: UserType) -> DbResult<()> { + pub async fn set_published(&mut self, published_by: UserType, db: &Database) -> DbResult<()> { let published_at = OffsetDateTime::now_utc().unix_timestamp(); let (published_by_type, published_by_id) = match &published_by { @@ -235,7 +238,7 @@ impl<'a> Album<'a> { .bind(published_at) .bind(published_by_type) .bind(&self.id) - .execute(&**self.db) + .execute(&**db) .await?; self.is_draft = false; @@ -245,10 +248,10 @@ impl<'a> Album<'a> { Ok(()) } - pub async fn set_draft(&mut self) -> DbResult<()> { + pub async fn set_draft(&mut self, db: &Database) -> DbResult<()> { sqlx::query("UPDATE album_metadata SET published_by = NULL, published_by_type = NULL, published_at = NULL, is_draft = true WHERE id = $1") .bind(&self.id) - .execute(&**self.db) + .execute(&**db) .await?; self.is_draft = true; @@ -258,8 +261,8 @@ impl<'a> Album<'a> { Ok(()) } - pub async fn delete(self) -> DbResult<()> { - let mut tx = self.db.begin().await?; + pub async fn delete(self, db: &Database) -> DbResult<()> { + let mut tx = db.begin().await?; // Must satisfy the foreign key constraint // So unset the cover photo before removing all photoss @@ -283,11 +286,17 @@ impl<'a> Album<'a> { Ok(()) } - pub async fn list(db: &'a Database) -> DbResult>> { + pub async fn list_ids(db: &Database) -> DbResult> { + Ok(sqlx::query_scalar("SELECT id FROM album_metadata") + .fetch_all(&**db) + .await?) + } + + pub async fn list(db: &Database) -> DbResult> { let selfs: Vec<_Album> = sqlx::query_as("SELECT * FROM album_metadata") .fetch_all(&**db) .await?; - Ok(selfs.into_iter().map(|x| x.into_album(db)).collect()) + Ok(selfs.into_iter().map(|x| x.into_album()).collect()) } } diff --git a/server/dal/src/database/photo.rs b/server/dal/src/database/photo.rs index e9af623..a7c2df6 100644 --- a/server/dal/src/database/photo.rs +++ b/server/dal/src/database/photo.rs @@ -99,11 +99,7 @@ impl<'a> Photo<'a> { format!("{}{random}", Self::ID_PREFIX) } - pub async fn create( - db: &'a Database, - album: &Album<'_>, - created_at: i64, - ) -> DbResult> { + pub async fn create(db: &'a Database, album: &Album, created_at: i64) -> DbResult> { let id = Self::generate_id(); sqlx::query("INSERT INTO photo_metadata (id, album_id, created_at) VALUES ($1, $2, $3)")