From 165b19e75c1723bb4617ade7c28791f32d06b7ed Mon Sep 17 00:00:00 2001 From: Nutomic Date: Thu, 30 Mar 2023 17:03:13 +0200 Subject: [PATCH] Optimize federated language updates to avoid unnecessary db writes (#2786) * Optimize federated language updates to avoid unnecessary db writes (fixes #2772) * fix tests * fix test, rename functions --------- Co-authored-by: Dessalines --- Cargo.lock | 1 + crates/api/src/site/leave_admin.rs | 2 +- crates/api_crud/src/community/create.rs | 2 +- crates/api_crud/src/community/update.rs | 2 +- crates/api_crud/src/site/read.rs | 2 +- crates/apub/src/api/read_community.rs | 2 +- crates/apub/src/objects/instance.rs | 2 +- crates/db_schema/Cargo.toml | 1 + crates/db_schema/src/impls/actor_language.rs | 77 ++++++++++++++------ crates/db_schema/src/impls/community.rs | 16 ++-- crates/db_schema/src/impls/local_user.rs | 2 +- crates/db_schema/src/impls/site.rs | 16 +++- 12 files changed, 86 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3dc23a462..fa1cded72 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2517,6 +2517,7 @@ dependencies = [ "diesel-derive-newtype", "diesel_ltree", "diesel_migrations", + "futures", "lemmy_utils", "once_cell", "regex", diff --git a/crates/api/src/site/leave_admin.rs b/crates/api/src/site/leave_admin.rs index 84515b9fe..c0f20eaf0 100644 --- a/crates/api/src/site/leave_admin.rs +++ b/crates/api/src/site/leave_admin.rs @@ -63,7 +63,7 @@ impl Perform for LeaveAdmin { let admins = PersonView::admins(context.pool()).await?; let all_languages = Language::read_all(context.pool()).await?; - let discussion_languages = SiteLanguage::read_local(context.pool()).await?; + let discussion_languages = SiteLanguage::read_local_raw(context.pool()).await?; let taglines = Tagline::get_all(context.pool(), site_view.local_site.id).await?; let custom_emojis = CustomEmojiView::get_all(context.pool(), site_view.local_site.id).await?; diff --git a/crates/api_crud/src/community/create.rs b/crates/api_crud/src/community/create.rs index 8aaeb8f6a..907cf1b20 100644 --- a/crates/api_crud/src/community/create.rs +++ b/crates/api_crud/src/community/create.rs @@ -135,7 +135,7 @@ impl PerformCrud for CreateCommunity { // Update the discussion_languages if that's provided let community_id = inserted_community.id; if let Some(languages) = data.discussion_languages.clone() { - let site_languages = SiteLanguage::read_local(context.pool()).await?; + let site_languages = SiteLanguage::read_local_raw(context.pool()).await?; // check that community languages are a subset of site languages // https://stackoverflow.com/a/64227550 let is_subset = languages.iter().all(|item| site_languages.contains(item)); diff --git a/crates/api_crud/src/community/update.rs b/crates/api_crud/src/community/update.rs index abfa217f9..e3d1acfa3 100644 --- a/crates/api_crud/src/community/update.rs +++ b/crates/api_crud/src/community/update.rs @@ -53,7 +53,7 @@ impl PerformCrud for EditCommunity { let community_id = data.community_id; if let Some(languages) = data.discussion_languages.clone() { - let site_languages = SiteLanguage::read_local(context.pool()).await?; + let site_languages = SiteLanguage::read_local_raw(context.pool()).await?; // check that community languages are a subset of site languages // https://stackoverflow.com/a/64227550 let is_subset = languages.iter().all(|item| site_languages.contains(item)); diff --git a/crates/api_crud/src/site/read.rs b/crates/api_crud/src/site/read.rs index c89dbef70..0687fdf66 100644 --- a/crates/api_crud/src/site/read.rs +++ b/crates/api_crud/src/site/read.rs @@ -87,7 +87,7 @@ impl PerformCrud for GetSite { build_federated_instances(&site_view.local_site, context.pool()).await?; let all_languages = Language::read_all(context.pool()).await?; - let discussion_languages = SiteLanguage::read_local(context.pool()).await?; + let discussion_languages = SiteLanguage::read_local_raw(context.pool()).await?; let taglines = Tagline::get_all(context.pool(), site_view.local_site.id).await?; let custom_emojis = CustomEmojiView::get_all(context.pool(), site_view.local_site.id).await?; diff --git a/crates/apub/src/api/read_community.rs b/crates/apub/src/api/read_community.rs index ee62275da..5547fcff0 100644 --- a/crates/apub/src/api/read_community.rs +++ b/crates/apub/src/api/read_community.rs @@ -80,7 +80,7 @@ impl PerformApub for GetCommunity { let site_id = Site::instance_actor_id_from_url(community_view.community.actor_id.clone().into()); - let mut site = Site::read_from_apub_id(context.pool(), site_id).await?; + let mut site = Site::read_from_apub_id(context.pool(), &site_id.into()).await?; // no need to include metadata for local site (its already available through other endpoints). // this also prevents us from leaking the federation private key. if let Some(s) = &site { diff --git a/crates/apub/src/objects/instance.rs b/crates/apub/src/objects/instance.rs index 4cb96a0fb..72d133441 100644 --- a/crates/apub/src/objects/instance.rs +++ b/crates/apub/src/objects/instance.rs @@ -71,7 +71,7 @@ impl Object for ApubSite { data: &Data, ) -> Result, LemmyError> { Ok( - Site::read_from_apub_id(data.pool(), object_id) + Site::read_from_apub_id(data.pool(), &object_id.into()) .await? .map(Into::into), ) diff --git a/crates/db_schema/Cargo.toml b/crates/db_schema/Cargo.toml index c21467ebc..75884d588 100644 --- a/crates/db_schema/Cargo.toml +++ b/crates/db_schema/Cargo.toml @@ -41,6 +41,7 @@ async-trait = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } tracing-error = { workspace = true } +futures = { workspace = true } deadpool = { version = "0.9.5", features = ["rt_tokio_1"], optional = true } [dev-dependencies] diff --git a/crates/db_schema/src/impls/actor_language.rs b/crates/db_schema/src/impls/actor_language.rs index bf3e7ac98..624eaf54f 100644 --- a/crates/db_schema/src/impls/actor_language.rs +++ b/crates/db_schema/src/impls/actor_language.rs @@ -25,7 +25,11 @@ use diesel::{ ExpressionMethods, QueryDsl, }; -use diesel_async::{AsyncPgConnection, RunQueryDsl}; +use diesel_async::{ + pooled_connection::deadpool::Object as PooledConnection, + AsyncPgConnection, + RunQueryDsl, +}; use lemmy_utils::error::LemmyError; use tokio::sync::OnceCell; @@ -68,6 +72,13 @@ impl LocalUserLanguage { for_local_user_id: LocalUserId, ) -> Result<(), Error> { let conn = &mut get_conn(pool).await?; + let lang_ids = convert_update_languages(conn, language_ids).await?; + + // No need to update if languages are unchanged + let current = LocalUserLanguage::read(pool, for_local_user_id).await?; + if current == lang_ids { + return Ok(()); + } conn .build_transaction() @@ -79,7 +90,6 @@ impl LocalUserLanguage { .execute(conn) .await?; - let lang_ids = convert_update_languages(conn, language_ids).await?; for l in lang_ids { let form = LocalUserLanguageForm { local_user_id: for_local_user_id, @@ -98,7 +108,7 @@ impl LocalUserLanguage { } impl SiteLanguage { - pub async fn read_local(pool: &DbPool) -> Result, Error> { + pub async fn read_local_raw(pool: &DbPool) -> Result, Error> { let conn = &mut get_conn(pool).await?; site::table .inner_join(local_site::table) @@ -109,15 +119,22 @@ impl SiteLanguage { .await } - pub async fn read(pool: &DbPool, for_site_id: SiteId) -> Result, Error> { - let conn = &mut get_conn(pool).await?; - - let langs = site_language::table + async fn read_raw( + conn: &mut PooledConnection, + for_site_id: SiteId, + ) -> Result, Error> { + site_language::table .filter(site_language::site_id.eq(for_site_id)) .order(site_language::language_id) .select(site_language::language_id) .load(conn) - .await?; + .await + } + + pub async fn read(pool: &DbPool, for_site_id: SiteId) -> Result, Error> { + let conn = &mut get_conn(pool).await?; + let langs = Self::read_raw(conn, for_site_id).await?; + convert_read_languages(conn, langs).await } @@ -129,6 +146,13 @@ impl SiteLanguage { let conn = &mut get_conn(pool).await?; let for_site_id = site.id; let instance_id = site.instance_id; + let lang_ids = convert_update_languages(conn, language_ids).await?; + + // No need to update if languages are unchanged + let current = SiteLanguage::read(pool, site.id).await?; + if current == lang_ids { + return Ok(()); + } conn .build_transaction() @@ -141,7 +165,6 @@ impl SiteLanguage { .execute(conn) .await?; - let lang_ids = convert_update_languages(conn, language_ids).await?; for l in lang_ids { let form = SiteLanguageForm { site_id: for_site_id, @@ -221,19 +244,25 @@ impl CommunityLanguage { Ok(()) } - pub async fn read( - pool: &DbPool, + async fn read_raw( + conn: &mut PooledConnection, for_community_id: CommunityId, ) -> Result, Error> { use crate::schema::community_language::dsl::{community_id, community_language, language_id}; - let conn = &mut get_conn(pool).await?; - - let langs = community_language + community_language .filter(community_id.eq(for_community_id)) .order(language_id) .select(language_id) .get_results(conn) - .await?; + .await + } + + pub async fn read( + pool: &DbPool, + for_community_id: CommunityId, + ) -> Result, Error> { + let conn = &mut get_conn(pool).await?; + let langs = Self::read_raw(conn, for_community_id).await?; convert_read_languages(conn, langs).await } @@ -243,9 +272,15 @@ impl CommunityLanguage { for_community_id: CommunityId, ) -> Result<(), Error> { let conn = &mut get_conn(pool).await?; - if language_ids.is_empty() { - language_ids = SiteLanguage::read_local(pool).await?; + language_ids = SiteLanguage::read_local_raw(pool).await?; + } + let lang_ids = convert_update_languages(conn, language_ids).await?; + + // No need to update if languages are unchanged + let current = CommunityLanguage::read_raw(conn, for_community_id).await?; + if current == lang_ids { + return Ok(()); } conn @@ -258,7 +293,7 @@ impl CommunityLanguage { .execute(conn) .await?; - for l in language_ids { + for l in lang_ids { let form = CommunityLanguageForm { community_id: for_community_id, language_id: l, @@ -464,7 +499,7 @@ mod tests { let pool = &build_db_pool_for_tests().await; let (site, instance) = create_test_site(pool).await; - let site_languages1 = SiteLanguage::read_local(pool).await.unwrap(); + let site_languages1 = SiteLanguage::read_local_raw(pool).await.unwrap(); // site is created with all languages assert_eq!(184, site_languages1.len()); @@ -473,7 +508,7 @@ mod tests { .await .unwrap(); - let site_languages2 = SiteLanguage::read_local(pool).await.unwrap(); + let site_languages2 = SiteLanguage::read_local_raw(pool).await.unwrap(); // after update, site only has new languages assert_eq!(test_langs, site_languages2); @@ -539,7 +574,7 @@ mod tests { assert_eq!(test_langs, read_site_langs); // Test the local ones are the same - let read_local_site_langs = SiteLanguage::read_local(pool).await.unwrap(); + let read_local_site_langs = SiteLanguage::read_local_raw(pool).await.unwrap(); assert_eq!(test_langs, read_local_site_langs); let community_form = CommunityInsertForm::builder() diff --git a/crates/db_schema/src/impls/community.rs b/crates/db_schema/src/impls/community.rs index fb22a5e34..fe41d4d58 100644 --- a/crates/db_schema/src/impls/community.rs +++ b/crates/db_schema/src/impls/community.rs @@ -2,7 +2,7 @@ use crate::{ newtypes::{CommunityId, DbUrl, PersonId}, schema::community::dsl::{actor_id, community, deleted, local, name, removed}, source::{ - actor_language::{CommunityLanguage, SiteLanguage}, + actor_language::CommunityLanguage, community::{ Community, CommunityFollower, @@ -41,6 +41,12 @@ impl Crud for Community { async fn create(pool: &DbPool, form: &Self::InsertForm) -> Result { let conn = &mut get_conn(pool).await?; + let is_new_community = match &form.actor_id { + Some(id) => Community::read_from_apub_id(pool, id).await?.is_none(), + None => true, + }; + + // Can't do separate insert/update commands because InsertForm/UpdateForm aren't convertible let community_ = insert_into(community) .values(form) .on_conflict(actor_id) @@ -49,12 +55,8 @@ impl Crud for Community { .get_result::(conn) .await?; - let site_languages = SiteLanguage::read_local(pool).await; - if let Ok(langs) = site_languages { - // if site exists, init user with site languages - CommunityLanguage::update(pool, langs, community_.id).await?; - } else { - // otherwise, init with all languages (this only happens during tests) + // Initialize languages for new community + if is_new_community { CommunityLanguage::update(pool, vec![], community_.id).await?; } diff --git a/crates/db_schema/src/impls/local_user.rs b/crates/db_schema/src/impls/local_user.rs index a35096dca..53e57ff85 100644 --- a/crates/db_schema/src/impls/local_user.rs +++ b/crates/db_schema/src/impls/local_user.rs @@ -83,7 +83,7 @@ impl Crud for LocalUser { .await .expect("couldnt create local user"); - let site_languages = SiteLanguage::read_local(pool).await; + let site_languages = SiteLanguage::read_local_raw(pool).await; if let Ok(langs) = site_languages { // if site exists, init user with site languages LocalUserLanguage::update(pool, langs, local_user_.id).await?; diff --git a/crates/db_schema/src/impls/site.rs b/crates/db_schema/src/impls/site.rs index 7120b8a67..3363edc93 100644 --- a/crates/db_schema/src/impls/site.rs +++ b/crates/db_schema/src/impls/site.rs @@ -25,6 +25,12 @@ impl Crud for Site { async fn create(pool: &DbPool, form: &Self::InsertForm) -> Result { let conn = &mut get_conn(pool).await?; + let is_new_site = match &form.actor_id { + Some(id_) => Site::read_from_apub_id(pool, id_).await?.is_none(), + None => true, + }; + + // Can't do separate insert/update commands because InsertForm/UpdateForm aren't convertible let site_ = insert_into(site) .values(form) .on_conflict(actor_id) @@ -33,8 +39,11 @@ impl Crud for Site { .get_result::(conn) .await?; - // initialize with all languages - SiteLanguage::update(pool, vec![], &site_).await?; + // initialize languages if site is newly created + if is_new_site { + // initialize with all languages + SiteLanguage::update(pool, vec![], &site_).await?; + } Ok(site_) } @@ -57,9 +66,8 @@ impl Crud for Site { } impl Site { - pub async fn read_from_apub_id(pool: &DbPool, object_id: Url) -> Result, Error> { + pub async fn read_from_apub_id(pool: &DbPool, object_id: &DbUrl) -> Result, Error> { let conn = &mut get_conn(pool).await?; - let object_id: DbUrl = object_id.into(); Ok( site .filter(actor_id.eq(object_id))