From 5d5acd7e501c5b6de28026e9c20054dda3a0fdc9 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Wed, 29 Oct 2025 23:53:08 -0400 Subject: [PATCH 01/20] Add site type and mapping fields to DbSite Extend DbSite to support multiple site types (self-hosted, WordPress.com). Add DbSiteType enum and mapped_site_id field to reference type-specific tables. Changes: - Add DbSiteType enum with SelfHosted and WordPressCom variants - Implement ToSql/FromSql for DbSiteType using i64 representation - Add site_type and mapped_site_id fields to DbSite struct - Update all DbSite construction in tests and fixtures - Add TODO comments in repository code for fetching site data from sites table --- wp_mobile_cache/src/lib.rs | 44 ++++++++++++++----- .../src/mappings/term_relationships.rs | 3 ++ wp_mobile_cache/src/repository/posts.rs | 9 ++++ .../src/repository/posts_constraint_tests.rs | 7 ++- .../src/repository/posts_transaction_tests.rs | 7 ++- wp_mobile_cache/src/test_fixtures.rs | 8 +++- 6 files changed, 65 insertions(+), 13 deletions(-) diff --git a/wp_mobile_cache/src/lib.rs b/wp_mobile_cache/src/lib.rs index 88934056f..f0972fbf2 100644 --- a/wp_mobile_cache/src/lib.rs +++ b/wp_mobile_cache/src/lib.rs @@ -85,6 +85,32 @@ impl From for i64 { } } +/// Type of WordPress site stored in the database. +/// +/// Uses integer representation in the database for performance. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] +#[repr(i64)] +pub enum DbSiteType { + SelfHosted = 0, + WordPressCom = 1, +} + +impl ToSql for DbSiteType { + fn to_sql(&self) -> rusqlite::Result> { + Ok(ToSqlOutput::from(*self as i64)) + } +} + +impl FromSql for DbSiteType { + fn column_result(value: rusqlite::types::ValueRef<'_>) -> FromSqlResult { + match i64::column_result(value)? { + 0 => Ok(DbSiteType::SelfHosted), + 1 => Ok(DbSiteType::WordPressCom), + other => Err(rusqlite::types::FromSqlError::OutOfRange(other)), + } + } +} + /// Represents a cached WordPress site in the database. /// /// # Design Rationale @@ -96,21 +122,19 @@ impl From for i64 { /// - **Not a self-hosted site identifier**: Self-hosted sites don't have numeric IDs /// - **Internal cache identifier only**: This ID exists only for our local database's multi-site support /// -/// # Future Extension +/// # Site Type Mapping /// -/// When site type tables are added (e.g., `self_hosted_sites`, `wordpress_com_sites`), this -/// struct will gain additional fields: +/// The `site_type` field indicates which type-specific table contains additional data: +/// - `DbSiteType::SelfHosted` → `mapped_site_id` references `self_hosted_sites` table +/// - `DbSiteType::WordPressCom` → `mapped_site_id` references `wordpress_com_sites` table (future) /// -/// ```ignore -/// pub struct DbSite { -/// pub row_id: RowId, -/// pub site_type: SiteType, // SelfHosted | WordPressCom -/// pub mapped_site_id: RowId, // Foreign key to specific site type table -/// } -/// ``` +/// Note: `mapped_site_id` is a reference column, not a foreign key constraint, since it can +/// point to different tables based on `site_type`. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] pub struct DbSite { pub row_id: RowId, + pub site_type: DbSiteType, + pub mapped_site_id: RowId, } /// Get the SQLite version string from the database. diff --git a/wp_mobile_cache/src/mappings/term_relationships.rs b/wp_mobile_cache/src/mappings/term_relationships.rs index 22b390ea2..b9640854f 100644 --- a/wp_mobile_cache/src/mappings/term_relationships.rs +++ b/wp_mobile_cache/src/mappings/term_relationships.rs @@ -33,6 +33,9 @@ impl DbTermRelationship { row_id: row.get_column(Col::Rowid)?, site: DbSite { row_id: row.get_column(Col::DbSiteId)?, + // TODO: These should be fetched from sites table via JOIN or separate query + site_type: crate::DbSiteType::SelfHosted, + mapped_site_id: crate::RowId(0), }, object_id: row.get_column(Col::ObjectId)?, term_id: TermId(row.get_column(Col::TermId)?), diff --git a/wp_mobile_cache/src/repository/posts.rs b/wp_mobile_cache/src/repository/posts.rs index b7c714404..e63802e3c 100644 --- a/wp_mobile_cache/src/repository/posts.rs +++ b/wp_mobile_cache/src/repository/posts.rs @@ -273,6 +273,9 @@ impl PostContext for EditContext { let row_id: RowId = row.get_column(Rowid)?; let site = DbSite { row_id: row.get_column(PostEditContextColumn::SiteId)?, + // TODO: These should be fetched from sites table via JOIN or separate query + site_type: crate::DbSiteType::SelfHosted, + mapped_site_id: RowId(0), }; // EditContext uses term relationships (categories and tags) @@ -358,6 +361,9 @@ impl PostContext for ViewContext { let row_id: RowId = row.get_column(Rowid)?; let site = DbSite { row_id: row.get_column(PostViewContextColumn::SiteId)?, + // TODO: These should be fetched from sites table via JOIN or separate query + site_type: crate::DbSiteType::SelfHosted, + mapped_site_id: RowId(0), }; // ViewContext uses term relationships (categories and tags) @@ -436,6 +442,9 @@ impl PostContext for EmbedContext { let row_id: RowId = row.get_column(Rowid)?; let site = DbSite { row_id: row.get_column(PostEmbedContextColumn::SiteId)?, + // TODO: These should be fetched from sites table via JOIN or separate query + site_type: crate::DbSiteType::SelfHosted, + mapped_site_id: RowId(0), }; // EmbedContext does not use term relationships (no categories/tags in embed context) diff --git a/wp_mobile_cache/src/repository/posts_constraint_tests.rs b/wp_mobile_cache/src/repository/posts_constraint_tests.rs index 5dfbab78b..47015b573 100644 --- a/wp_mobile_cache/src/repository/posts_constraint_tests.rs +++ b/wp_mobile_cache/src/repository/posts_constraint_tests.rs @@ -57,7 +57,12 @@ fn test_duplicate_post_id_in_same_site_updates_on_upsert(mut test_ctx: TestConte #[rstest] fn test_invalid_site_id_fails_foreign_key_constraint(mut test_ctx: TestContext) { - let non_existent_site = DbSite { row_id: RowId(999) }; // Site doesn't exist + // Site doesn't exist in database - intentionally invalid for testing error handling + let non_existent_site = DbSite { + row_id: RowId(999), + site_type: crate::DbSiteType::SelfHosted, + mapped_site_id: RowId(999), + }; let post = PostBuilder::minimal().build(); let result = test_ctx diff --git a/wp_mobile_cache/src/repository/posts_transaction_tests.rs b/wp_mobile_cache/src/repository/posts_transaction_tests.rs index 737fd3009..5350c0042 100644 --- a/wp_mobile_cache/src/repository/posts_transaction_tests.rs +++ b/wp_mobile_cache/src/repository/posts_transaction_tests.rs @@ -75,7 +75,12 @@ fn test_upsert_batch_handles_duplicate_ids_by_updating(mut test_ctx: TestContext #[rstest] fn test_upsert_batch_fails_on_foreign_key_violation(mut test_ctx: TestContext) { - let invalid_site = DbSite { row_id: RowId(999) }; + // Site doesn't exist in database - intentionally invalid for testing error handling + let invalid_site = DbSite { + row_id: RowId(999), + site_type: crate::DbSiteType::SelfHosted, + mapped_site_id: RowId(999), + }; let post1 = PostBuilder::minimal().build(); let post2 = PostBuilder::minimal().build(); diff --git a/wp_mobile_cache/src/test_fixtures.rs b/wp_mobile_cache/src/test_fixtures.rs index 0fd104ee0..5d8fe4792 100644 --- a/wp_mobile_cache/src/test_fixtures.rs +++ b/wp_mobile_cache/src/test_fixtures.rs @@ -33,7 +33,11 @@ pub struct TestContext { pub fn test_ctx() -> TestContext { TestContext { conn: test_db(), - site: DbSite { row_id: RowId(1) }, + site: DbSite { + row_id: RowId(1), + site_type: crate::DbSiteType::SelfHosted, + mapped_site_id: RowId(1), + }, post_repo: PostRepository::new(), term_repo: TermRelationshipRepository, } @@ -62,6 +66,8 @@ pub fn create_test_site(conn: &Connection, id: i64) -> DbSite { .expect("Failed to create test site"); DbSite { row_id: RowId(id as u64), + site_type: crate::DbSiteType::SelfHosted, + mapped_site_id: RowId(id as u64), } } From e3eea4a12922b98d546358b456465fa3f8cf6f6a Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Wed, 29 Oct 2025 23:55:17 -0400 Subject: [PATCH 02/20] Move DbSite and DbSiteType to db_types module Relocate site-related types to db_types for better organization. Changes: - Create db_types/db_site.rs module with DbSite and DbSiteType - Re-export types from lib.rs for backward compatibility - Update module exports in db_types.rs --- wp_mobile_cache/src/db_types.rs | 1 + wp_mobile_cache/src/db_types/db_site.rs | 54 +++++++++++++++++++++++++ wp_mobile_cache/src/lib.rs | 53 +----------------------- 3 files changed, 57 insertions(+), 51 deletions(-) create mode 100644 wp_mobile_cache/src/db_types/db_site.rs diff --git a/wp_mobile_cache/src/db_types.rs b/wp_mobile_cache/src/db_types.rs index 83d81e78b..f73e52ec1 100644 --- a/wp_mobile_cache/src/db_types.rs +++ b/wp_mobile_cache/src/db_types.rs @@ -1 +1,2 @@ +pub mod db_site; pub mod posts; diff --git a/wp_mobile_cache/src/db_types/db_site.rs b/wp_mobile_cache/src/db_types/db_site.rs new file mode 100644 index 000000000..3eb5e4857 --- /dev/null +++ b/wp_mobile_cache/src/db_types/db_site.rs @@ -0,0 +1,54 @@ +use crate::RowId; +use rusqlite::types::{FromSql, FromSqlResult, ToSql, ToSqlOutput}; + +/// Type of WordPress site stored in the database. +/// +/// Uses integer representation in the database for performance. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] +#[repr(i64)] +pub enum DbSiteType { + SelfHosted = 0, + WordPressCom = 1, +} + +impl ToSql for DbSiteType { + fn to_sql(&self) -> rusqlite::Result> { + Ok(ToSqlOutput::from(*self as i64)) + } +} + +impl FromSql for DbSiteType { + fn column_result(value: rusqlite::types::ValueRef<'_>) -> FromSqlResult { + match i64::column_result(value)? { + 0 => Ok(DbSiteType::SelfHosted), + 1 => Ok(DbSiteType::WordPressCom), + other => Err(rusqlite::types::FromSqlError::OutOfRange(other)), + } + } +} + +/// Represents a cached WordPress site in the database. +/// +/// # Design Rationale +/// +/// This is intentionally a database-specific type (hence the `Db` prefix) rather than +/// a domain type representing a WordPress site. This design choice prevents confusion: +/// +/// - **Not a WordPress.com site ID**: The `row_id` has no relationship to WordPress.com site IDs +/// - **Not a self-hosted site identifier**: Self-hosted sites don't have numeric IDs +/// - **Internal cache identifier only**: This ID exists only for our local database's multi-site support +/// +/// # Site Type Mapping +/// +/// The `site_type` field indicates which type-specific table contains additional data: +/// - `DbSiteType::SelfHosted` → `mapped_site_id` references `self_hosted_sites` table +/// - `DbSiteType::WordPressCom` → `mapped_site_id` references `wordpress_com_sites` table (future) +/// +/// Note: `mapped_site_id` is a reference column, not a foreign key constraint, since it can +/// point to different tables based on `site_type`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] +pub struct DbSite { + pub row_id: RowId, + pub site_type: DbSiteType, + pub mapped_site_id: RowId, +} diff --git a/wp_mobile_cache/src/lib.rs b/wp_mobile_cache/src/lib.rs index f0972fbf2..ae6b74024 100644 --- a/wp_mobile_cache/src/lib.rs +++ b/wp_mobile_cache/src/lib.rs @@ -85,57 +85,8 @@ impl From for i64 { } } -/// Type of WordPress site stored in the database. -/// -/// Uses integer representation in the database for performance. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] -#[repr(i64)] -pub enum DbSiteType { - SelfHosted = 0, - WordPressCom = 1, -} - -impl ToSql for DbSiteType { - fn to_sql(&self) -> rusqlite::Result> { - Ok(ToSqlOutput::from(*self as i64)) - } -} - -impl FromSql for DbSiteType { - fn column_result(value: rusqlite::types::ValueRef<'_>) -> FromSqlResult { - match i64::column_result(value)? { - 0 => Ok(DbSiteType::SelfHosted), - 1 => Ok(DbSiteType::WordPressCom), - other => Err(rusqlite::types::FromSqlError::OutOfRange(other)), - } - } -} - -/// Represents a cached WordPress site in the database. -/// -/// # Design Rationale -/// -/// This is intentionally a database-specific type (hence the `Db` prefix) rather than -/// a domain type representing a WordPress site. This design choice prevents confusion: -/// -/// - **Not a WordPress.com site ID**: The `row_id` has no relationship to WordPress.com site IDs -/// - **Not a self-hosted site identifier**: Self-hosted sites don't have numeric IDs -/// - **Internal cache identifier only**: This ID exists only for our local database's multi-site support -/// -/// # Site Type Mapping -/// -/// The `site_type` field indicates which type-specific table contains additional data: -/// - `DbSiteType::SelfHosted` → `mapped_site_id` references `self_hosted_sites` table -/// - `DbSiteType::WordPressCom` → `mapped_site_id` references `wordpress_com_sites` table (future) -/// -/// Note: `mapped_site_id` is a reference column, not a foreign key constraint, since it can -/// point to different tables based on `site_type`. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] -pub struct DbSite { - pub row_id: RowId, - pub site_type: DbSiteType, - pub mapped_site_id: RowId, -} +// Re-export site types from db_types module +pub use db_types::db_site::{DbSite, DbSiteType}; /// Get the SQLite version string from the database. /// From c0409cf82147c123feb4bef620a23994fae79f2d Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 00:00:16 -0400 Subject: [PATCH 03/20] Reorganize mappings module into db_types Move type-related code from mappings module to db_types for better organization. The mappings name was misleading since we're not doing mappings anymore. Changes: - Move `mappings/term_relationships.rs` to `db_types/db_term_relationship.rs` - Move `mappings/helpers.rs` to `db_types/helpers.rs` - Create `db_types/row_ext.rs` with `ColumnIndex` and `RowExt` traits - Update all imports throughout the codebase - Remove old `mappings` module --- wp_mobile_cache/src/db_types.rs | 3 +++ .../db_term_relationship.rs} | 2 +- .../src/{mappings => db_types}/helpers.rs | 2 +- wp_mobile_cache/src/db_types/posts/edit.rs | 2 +- wp_mobile_cache/src/db_types/posts/embed.rs | 2 +- wp_mobile_cache/src/db_types/posts/view.rs | 2 +- .../src/{mappings.rs => db_types/row_ext.rs} | 3 --- wp_mobile_cache/src/lib.rs | 1 - wp_mobile_cache/src/repository/posts.rs | 14 +++++++------- 9 files changed, 15 insertions(+), 16 deletions(-) rename wp_mobile_cache/src/{mappings/term_relationships.rs => db_types/db_term_relationship.rs} (96%) rename wp_mobile_cache/src/{mappings => db_types}/helpers.rs (99%) rename wp_mobile_cache/src/{mappings.rs => db_types/row_ext.rs} (93%) diff --git a/wp_mobile_cache/src/db_types.rs b/wp_mobile_cache/src/db_types.rs index f73e52ec1..e83eca6f8 100644 --- a/wp_mobile_cache/src/db_types.rs +++ b/wp_mobile_cache/src/db_types.rs @@ -1,2 +1,5 @@ pub mod db_site; +pub mod db_term_relationship; +pub mod helpers; pub mod posts; +pub mod row_ext; diff --git a/wp_mobile_cache/src/mappings/term_relationships.rs b/wp_mobile_cache/src/db_types/db_term_relationship.rs similarity index 96% rename from wp_mobile_cache/src/mappings/term_relationships.rs rename to wp_mobile_cache/src/db_types/db_term_relationship.rs index b9640854f..bdc2e9bb8 100644 --- a/wp_mobile_cache/src/mappings/term_relationships.rs +++ b/wp_mobile_cache/src/db_types/db_term_relationship.rs @@ -1,6 +1,6 @@ use crate::{ DbSite, SqliteDbError, - mappings::{ColumnIndex, RowExt}, + db_types::row_ext::{ColumnIndex, RowExt}, term_relationships::DbTermRelationship, }; use rusqlite::Row; diff --git a/wp_mobile_cache/src/mappings/helpers.rs b/wp_mobile_cache/src/db_types/helpers.rs similarity index 99% rename from wp_mobile_cache/src/mappings/helpers.rs rename to wp_mobile_cache/src/db_types/helpers.rs index 690d2ea27..6d2beb3d5 100644 --- a/wp_mobile_cache/src/mappings/helpers.rs +++ b/wp_mobile_cache/src/db_types/helpers.rs @@ -1,6 +1,6 @@ use crate::{ SqliteDbError, - mappings::{ColumnIndex, RowExt}, + db_types::row_ext::{ColumnIndex, RowExt}, }; use rusqlite::Row; use serde::{Deserialize, Serialize}; diff --git a/wp_mobile_cache/src/db_types/posts/edit.rs b/wp_mobile_cache/src/db_types/posts/edit.rs index 21adf7adc..6ff00ee74 100644 --- a/wp_mobile_cache/src/db_types/posts/edit.rs +++ b/wp_mobile_cache/src/db_types/posts/edit.rs @@ -1,4 +1,4 @@ -use crate::{DbSite, RowId, mappings::ColumnIndex}; +use crate::{DbSite, RowId, db_types::row_ext::ColumnIndex}; use wp_api::posts::AnyPostWithEditContext; /// Column indexes for posts_edit_context table. diff --git a/wp_mobile_cache/src/db_types/posts/embed.rs b/wp_mobile_cache/src/db_types/posts/embed.rs index 9f12d2e11..ce8119a70 100644 --- a/wp_mobile_cache/src/db_types/posts/embed.rs +++ b/wp_mobile_cache/src/db_types/posts/embed.rs @@ -1,4 +1,4 @@ -use crate::{DbSite, RowId, mappings::ColumnIndex}; +use crate::{DbSite, RowId, db_types::row_ext::ColumnIndex}; use wp_api::posts::AnyPostWithEmbedContext; /// Column indexes for posts_embed_context table. diff --git a/wp_mobile_cache/src/db_types/posts/view.rs b/wp_mobile_cache/src/db_types/posts/view.rs index 441650e63..3ecf14798 100644 --- a/wp_mobile_cache/src/db_types/posts/view.rs +++ b/wp_mobile_cache/src/db_types/posts/view.rs @@ -1,4 +1,4 @@ -use crate::{DbSite, RowId, mappings::ColumnIndex}; +use crate::{DbSite, RowId, db_types::row_ext::ColumnIndex}; use wp_api::posts::AnyPostWithViewContext; /// Column indexes for posts_view_context table. diff --git a/wp_mobile_cache/src/mappings.rs b/wp_mobile_cache/src/db_types/row_ext.rs similarity index 93% rename from wp_mobile_cache/src/mappings.rs rename to wp_mobile_cache/src/db_types/row_ext.rs index eb98ff0d2..477354b10 100644 --- a/wp_mobile_cache/src/mappings.rs +++ b/wp_mobile_cache/src/db_types/row_ext.rs @@ -1,8 +1,5 @@ use rusqlite::Row; -pub mod helpers; -pub mod term_relationships; - /// Trait for types that can be used as column indexes. /// Implemented by column enum types to provide type-safe column access. pub trait ColumnIndex { diff --git a/wp_mobile_cache/src/lib.rs b/wp_mobile_cache/src/lib.rs index ae6b74024..7c86ef455 100644 --- a/wp_mobile_cache/src/lib.rs +++ b/wp_mobile_cache/src/lib.rs @@ -5,7 +5,6 @@ use std::sync::{Arc, Mutex}; pub mod context; pub mod db_types; -pub mod mappings; pub mod repository; pub mod term_relationships; diff --git a/wp_mobile_cache/src/repository/posts.rs b/wp_mobile_cache/src/repository/posts.rs index e63802e3c..23260e7a5 100644 --- a/wp_mobile_cache/src/repository/posts.rs +++ b/wp_mobile_cache/src/repository/posts.rs @@ -1,16 +1,16 @@ use crate::{ DbSite, RowId, SqliteDbError, context::{EditContext, EmbedContext, IsContext, ViewContext}, - db_types::posts::{ - DbAnyPostWithEditContext, DbAnyPostWithEmbedContext, DbAnyPostWithViewContext, - PostEditContextColumn, PostEmbedContextColumn, PostViewContextColumn, - }, - mappings::{ - RowExt, + db_types::{ helpers::{ bool_to_integer, deserialize_json_value, get_id, get_optional_id, integer_to_bool, parse_datetime, parse_enum, parse_optional_enum, serialize_value_to_json, }, + posts::{ + DbAnyPostWithEditContext, DbAnyPostWithEmbedContext, DbAnyPostWithViewContext, + PostEditContextColumn, PostEmbedContextColumn, PostViewContextColumn, + }, + row_ext::RowExt, }, repository::{ QueryExecutor, TransactionManager, term_relationships::TermRelationshipRepository, @@ -856,7 +856,7 @@ mod tests { use crate::db_types::posts::{ PostEditContextColumn, PostEmbedContextColumn, PostViewContextColumn, }; - use crate::mappings::ColumnIndex; + use crate::db_types::row_ext::ColumnIndex; use crate::test_fixtures::{ TestContext, assert_recent_timestamp, get_table_column_names, posts::PostBuilder, test_ctx, }; From 60a1e4146cea65c3e6f7dbfa8632026687e4fe46 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 00:22:16 -0400 Subject: [PATCH 04/20] Replace DbSite field with db_site_id in database wrapper types Use `db_site_id: RowId` instead of `site: DbSite` in database wrapper types. This avoids unnecessary joins since we already know the site context when querying. Changes: - Replace `site: DbSite` with `db_site_id: RowId` in DbAnyPostWithEditContext, DbAnyPostWithViewContext, DbAnyPostWithEmbedContext - Replace `site: DbSite` with `db_site_id: RowId` in DbTermRelationship - Rename column enum variants from `SiteId` to `DbSiteId` for clarity - Update all construction and test assertions to use `db_site_id` - Improve comment clarity for db_site_id field in DbTermRelationship --- .../src/db_types/db_term_relationship.rs | 9 +--- wp_mobile_cache/src/db_types/posts/edit.rs | 6 +-- wp_mobile_cache/src/db_types/posts/embed.rs | 6 +-- wp_mobile_cache/src/db_types/posts/view.rs | 6 +-- wp_mobile_cache/src/repository/posts.rs | 41 ++++++------------- wp_mobile_cache/src/term_relationships.rs | 6 +-- 6 files changed, 27 insertions(+), 47 deletions(-) diff --git a/wp_mobile_cache/src/db_types/db_term_relationship.rs b/wp_mobile_cache/src/db_types/db_term_relationship.rs index bdc2e9bb8..c5a3275b0 100644 --- a/wp_mobile_cache/src/db_types/db_term_relationship.rs +++ b/wp_mobile_cache/src/db_types/db_term_relationship.rs @@ -1,5 +1,5 @@ use crate::{ - DbSite, SqliteDbError, + SqliteDbError, db_types::row_ext::{ColumnIndex, RowExt}, term_relationships::DbTermRelationship, }; @@ -31,12 +31,7 @@ impl DbTermRelationship { Ok(Self { row_id: row.get_column(Col::Rowid)?, - site: DbSite { - row_id: row.get_column(Col::DbSiteId)?, - // TODO: These should be fetched from sites table via JOIN or separate query - site_type: crate::DbSiteType::SelfHosted, - mapped_site_id: crate::RowId(0), - }, + db_site_id: row.get_column(Col::DbSiteId)?, object_id: row.get_column(Col::ObjectId)?, term_id: TermId(row.get_column(Col::TermId)?), taxonomy_type: row.get_column::(Col::TaxonomyType)?.into(), diff --git a/wp_mobile_cache/src/db_types/posts/edit.rs b/wp_mobile_cache/src/db_types/posts/edit.rs index 6ff00ee74..41fa5e197 100644 --- a/wp_mobile_cache/src/db_types/posts/edit.rs +++ b/wp_mobile_cache/src/db_types/posts/edit.rs @@ -1,4 +1,4 @@ -use crate::{DbSite, RowId, db_types::row_ext::ColumnIndex}; +use crate::{RowId, db_types::row_ext::ColumnIndex}; use wp_api::posts::AnyPostWithEditContext; /// Column indexes for posts_edit_context table. @@ -7,7 +7,7 @@ use wp_api::posts::AnyPostWithEditContext; #[derive(Debug, Clone, Copy)] pub(crate) enum PostEditContextColumn { Rowid = 0, - SiteId = 1, + DbSiteId = 1, Id = 2, Date = 3, DateGmt = 4, @@ -52,7 +52,7 @@ impl ColumnIndex for PostEditContextColumn { pub struct DbAnyPostWithEditContext { pub row_id: RowId, - pub site: DbSite, + pub db_site_id: RowId, pub post: AnyPostWithEditContext, pub last_fetched_at: String, } diff --git a/wp_mobile_cache/src/db_types/posts/embed.rs b/wp_mobile_cache/src/db_types/posts/embed.rs index ce8119a70..c10cc9f61 100644 --- a/wp_mobile_cache/src/db_types/posts/embed.rs +++ b/wp_mobile_cache/src/db_types/posts/embed.rs @@ -1,4 +1,4 @@ -use crate::{DbSite, RowId, db_types::row_ext::ColumnIndex}; +use crate::{RowId, db_types::row_ext::ColumnIndex}; use wp_api::posts::AnyPostWithEmbedContext; /// Column indexes for posts_embed_context table. @@ -7,7 +7,7 @@ use wp_api::posts::AnyPostWithEmbedContext; #[derive(Debug, Clone, Copy)] pub(crate) enum PostEmbedContextColumn { Rowid = 0, - SiteId = 1, + DbSiteId = 1, Id = 2, Date = 3, Link = 4, @@ -30,7 +30,7 @@ impl ColumnIndex for PostEmbedContextColumn { pub struct DbAnyPostWithEmbedContext { pub row_id: RowId, - pub site: DbSite, + pub db_site_id: RowId, pub post: AnyPostWithEmbedContext, pub last_fetched_at: String, } diff --git a/wp_mobile_cache/src/db_types/posts/view.rs b/wp_mobile_cache/src/db_types/posts/view.rs index 3ecf14798..024ee353f 100644 --- a/wp_mobile_cache/src/db_types/posts/view.rs +++ b/wp_mobile_cache/src/db_types/posts/view.rs @@ -1,4 +1,4 @@ -use crate::{DbSite, RowId, db_types::row_ext::ColumnIndex}; +use crate::{RowId, db_types::row_ext::ColumnIndex}; use wp_api::posts::AnyPostWithViewContext; /// Column indexes for posts_view_context table. @@ -7,7 +7,7 @@ use wp_api::posts::AnyPostWithViewContext; #[derive(Debug, Clone, Copy)] pub(crate) enum PostViewContextColumn { Rowid = 0, - SiteId = 1, + DbSiteId = 1, Id = 2, Date = 3, DateGmt = 4, @@ -45,7 +45,7 @@ impl ColumnIndex for PostViewContextColumn { pub struct DbAnyPostWithViewContext { pub row_id: RowId, - pub site: DbSite, + pub db_site_id: RowId, pub post: AnyPostWithViewContext, pub last_fetched_at: String, } diff --git a/wp_mobile_cache/src/repository/posts.rs b/wp_mobile_cache/src/repository/posts.rs index 23260e7a5..253597c81 100644 --- a/wp_mobile_cache/src/repository/posts.rs +++ b/wp_mobile_cache/src/repository/posts.rs @@ -271,12 +271,7 @@ impl PostContext for EditContext { use PostEditContextColumn::*; let row_id: RowId = row.get_column(Rowid)?; - let site = DbSite { - row_id: row.get_column(PostEditContextColumn::SiteId)?, - // TODO: These should be fetched from sites table via JOIN or separate query - site_type: crate::DbSiteType::SelfHosted, - mapped_site_id: RowId(0), - }; + let db_site_id: RowId = row.get_column(PostEditContextColumn::DbSiteId)?; // EditContext uses term relationships (categories and tags) let term_relationships = fetch_terms()?; @@ -341,7 +336,7 @@ impl PostContext for EditContext { Ok(DbAnyPostWithEditContext { row_id, - site, + db_site_id, post, last_fetched_at: row.get_column(LastFetchedAt)?, }) @@ -359,12 +354,7 @@ impl PostContext for ViewContext { use PostViewContextColumn::*; let row_id: RowId = row.get_column(Rowid)?; - let site = DbSite { - row_id: row.get_column(PostViewContextColumn::SiteId)?, - // TODO: These should be fetched from sites table via JOIN or separate query - site_type: crate::DbSiteType::SelfHosted, - mapped_site_id: RowId(0), - }; + let db_site_id: RowId = row.get_column(PostViewContextColumn::DbSiteId)?; // ViewContext uses term relationships (categories and tags) let term_relationships = fetch_terms()?; @@ -422,7 +412,7 @@ impl PostContext for ViewContext { Ok(DbAnyPostWithViewContext { row_id, - site, + db_site_id, post, last_fetched_at: row.get_column(LastFetchedAt)?, }) @@ -440,12 +430,7 @@ impl PostContext for EmbedContext { use PostEmbedContextColumn::*; let row_id: RowId = row.get_column(Rowid)?; - let site = DbSite { - row_id: row.get_column(PostEmbedContextColumn::SiteId)?, - // TODO: These should be fetched from sites table via JOIN or separate query - site_type: crate::DbSiteType::SelfHosted, - mapped_site_id: RowId(0), - }; + let db_site_id: RowId = row.get_column(PostEmbedContextColumn::DbSiteId)?; // EmbedContext does not use term relationships (no categories/tags in embed context) // The fetch_terms closure is never called, avoiding unnecessary database queries @@ -477,7 +462,7 @@ impl PostContext for EmbedContext { Ok(DbAnyPostWithEmbedContext { row_id, - site, + db_site_id, post, last_fetched_at: row.get_column(LastFetchedAt)?, }) @@ -873,7 +858,7 @@ mod tests { // Verify each enum value maps to the correct column name assert_eq!(columns[Rowid.as_index()], "rowid"); - assert_eq!(columns[SiteId.as_index()], "db_site_id"); + assert_eq!(columns[DbSiteId.as_index()], "db_site_id"); assert_eq!(columns[Id.as_index()], "id"); assert_eq!(columns[Date.as_index()], "date"); assert_eq!(columns[DateGmt.as_index()], "date_gmt"); @@ -925,7 +910,7 @@ mod tests { let columns = get_table_column_names(&test_ctx.conn, "posts_view_context"); assert_eq!(columns[Rowid.as_index()], "rowid"); - assert_eq!(columns[SiteId.as_index()], "db_site_id"); + assert_eq!(columns[DbSiteId.as_index()], "db_site_id"); assert_eq!(columns[Id.as_index()], "id"); assert_eq!(columns[Date.as_index()], "date"); assert_eq!(columns[DateGmt.as_index()], "date_gmt"); @@ -966,7 +951,7 @@ mod tests { let columns = get_table_column_names(&test_ctx.conn, "posts_embed_context"); assert_eq!(columns[Rowid.as_index()], "rowid"); - assert_eq!(columns[SiteId.as_index()], "db_site_id"); + assert_eq!(columns[DbSiteId.as_index()], "db_site_id"); assert_eq!(columns[Id.as_index()], "id"); assert_eq!(columns[Date.as_index()], "date"); assert_eq!(columns[Link.as_index()], "link"); @@ -1002,7 +987,7 @@ mod tests { // Verify round-trip assert_eq!(retrieved.row_id, rowid); - assert_eq!(retrieved.site, test_ctx.site); + assert_eq!(retrieved.db_site_id, test_ctx.site.row_id); assert_recent_timestamp(&retrieved.last_fetched_at); assert_eq!(retrieved.post, original_post); } @@ -1074,7 +1059,7 @@ mod tests { .expect("Post should exist"); assert_eq!(retrieved.row_id, rowid); - assert_eq!(retrieved.site, test_ctx.site); + assert_eq!(retrieved.db_site_id, test_ctx.site.row_id); assert_eq!(retrieved.post, post); } @@ -1096,7 +1081,7 @@ mod tests { .expect("Post should exist"); assert_eq!(retrieved.post.id, PostId(42)); - assert_eq!(retrieved.site, test_ctx.site); + assert_eq!(retrieved.db_site_id, test_ctx.site.row_id); assert_eq!(retrieved.post, post); } @@ -1284,7 +1269,7 @@ mod tests { .expect("Failed to select post by post_id") .expect("Post should exist after insert"); assert_eq!(retrieved.row_id, rowid); - assert_eq!(retrieved.site, test_ctx.site); + assert_eq!(retrieved.db_site_id, test_ctx.site.row_id); assert_eq!(retrieved.post.status, PostStatus::Draft); } diff --git a/wp_mobile_cache/src/term_relationships.rs b/wp_mobile_cache/src/term_relationships.rs index 36ea0d116..264630348 100644 --- a/wp_mobile_cache/src/term_relationships.rs +++ b/wp_mobile_cache/src/term_relationships.rs @@ -1,4 +1,4 @@ -use crate::{DbSite, RowId}; +use crate::RowId; use wp_api::taxonomies::TaxonomyType; use wp_api::terms::TermId; @@ -10,8 +10,8 @@ use wp_api::terms::TermId; pub struct DbTermRelationship { /// SQLite rowid of this relationship pub row_id: RowId, - /// Site this relationship belongs to - pub site: DbSite, + /// Database site ID (rowid from sites table) this relationship belongs to + pub db_site_id: RowId, /// Row ID of the object (post, page, etc.) in its respective table pub object_id: RowId, /// WordPress term ID From 8d3a253daade8313437c01b4006f604dd1521928 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 00:31:25 -0400 Subject: [PATCH 05/20] Add DbSelfHostedSite and self_hosted_sites table migration Create database structure for storing self-hosted WordPress sites with URL and API root. Changes: - Add `DbSelfHostedSite` struct in `db_types/self_hosted_site.rs` - Create `0006-create-self-hosted-sites-table.sql` migration - Modify `sites` table to add `site_type` and `mapped_site_id` columns - Update test fixtures to properly insert into both tables - Add migration to `MIGRATION_QUERIES` array --- .../migrations/0001-create-sites-table.sql | 9 ++++- .../0006-create-self-hosted-sites-table.sql | 12 +++++++ wp_mobile_cache/src/db_types.rs | 1 + .../src/db_types/self_hosted_site.rs | 24 +++++++++++++ wp_mobile_cache/src/lib.rs | 3 +- wp_mobile_cache/src/test_fixtures.rs | 35 ++++++++++++++++--- 6 files changed, 78 insertions(+), 6 deletions(-) create mode 100644 wp_mobile_cache/migrations/0006-create-self-hosted-sites-table.sql create mode 100644 wp_mobile_cache/src/db_types/self_hosted_site.rs diff --git a/wp_mobile_cache/migrations/0001-create-sites-table.sql b/wp_mobile_cache/migrations/0001-create-sites-table.sql index f1a2dc510..ebee11be0 100644 --- a/wp_mobile_cache/migrations/0001-create-sites-table.sql +++ b/wp_mobile_cache/migrations/0001-create-sites-table.sql @@ -1,4 +1,11 @@ CREATE TABLE `sites` ( -- Internal DB field (auto-incrementing) - `id` INTEGER PRIMARY KEY AUTOINCREMENT + `id` INTEGER PRIMARY KEY AUTOINCREMENT, + + -- Type of site (0 = SelfHosted, 1 = WordPressCom) + `site_type` INTEGER NOT NULL, + + -- Reference to type-specific table (self_hosted_sites.id or wordpress_com_sites.id) + -- Note: Not a foreign key constraint since it can point to different tables + `mapped_site_id` INTEGER NOT NULL ) STRICT; diff --git a/wp_mobile_cache/migrations/0006-create-self-hosted-sites-table.sql b/wp_mobile_cache/migrations/0006-create-self-hosted-sites-table.sql new file mode 100644 index 000000000..0898a0c86 --- /dev/null +++ b/wp_mobile_cache/migrations/0006-create-self-hosted-sites-table.sql @@ -0,0 +1,12 @@ +CREATE TABLE `self_hosted_sites` ( + -- Internal DB field (auto-incrementing) + `id` INTEGER PRIMARY KEY AUTOINCREMENT, + + -- Site URL (unique constraint for upsert logic) + `url` TEXT NOT NULL UNIQUE, + + -- WordPress REST API root URL + `api_root` TEXT NOT NULL +) STRICT; + +CREATE INDEX idx_self_hosted_sites_url ON self_hosted_sites(url); diff --git a/wp_mobile_cache/src/db_types.rs b/wp_mobile_cache/src/db_types.rs index e83eca6f8..0a3874799 100644 --- a/wp_mobile_cache/src/db_types.rs +++ b/wp_mobile_cache/src/db_types.rs @@ -3,3 +3,4 @@ pub mod db_term_relationship; pub mod helpers; pub mod posts; pub mod row_ext; +pub mod self_hosted_site; diff --git a/wp_mobile_cache/src/db_types/self_hosted_site.rs b/wp_mobile_cache/src/db_types/self_hosted_site.rs new file mode 100644 index 000000000..864c3e2fa --- /dev/null +++ b/wp_mobile_cache/src/db_types/self_hosted_site.rs @@ -0,0 +1,24 @@ +use crate::{RowId, db_types::row_ext::ColumnIndex}; + +/// Column indexes for self_hosted_sites table. +/// These must match the order of columns in the CREATE TABLE statement. +#[repr(usize)] +#[derive(Debug, Clone, Copy)] +pub(crate) enum SelfHostedSiteColumn { + Rowid = 0, + Url = 1, + ApiRoot = 2, +} + +impl ColumnIndex for SelfHostedSiteColumn { + fn as_index(&self) -> usize { + *self as usize + } +} + +/// Represents a self-hosted WordPress site in the database. +pub struct DbSelfHostedSite { + pub row_id: RowId, + pub url: String, + pub api_root: String, +} diff --git a/wp_mobile_cache/src/lib.rs b/wp_mobile_cache/src/lib.rs index 7c86ef455..332898acc 100644 --- a/wp_mobile_cache/src/lib.rs +++ b/wp_mobile_cache/src/lib.rs @@ -193,12 +193,13 @@ impl WpApiCache { } } -static MIGRATION_QUERIES: [&str; 5] = [ +static MIGRATION_QUERIES: [&str; 6] = [ include_str!("../migrations/0001-create-sites-table.sql"), include_str!("../migrations/0002-create-posts-table.sql"), include_str!("../migrations/0003-create-term-relationships.sql"), include_str!("../migrations/0004-create-posts-view-context-table.sql"), include_str!("../migrations/0005-create-posts-embed-context-table.sql"), + include_str!("../migrations/0006-create-self-hosted-sites-table.sql"), ]; pub struct MigrationManager<'a> { diff --git a/wp_mobile_cache/src/test_fixtures.rs b/wp_mobile_cache/src/test_fixtures.rs index 5d8fe4792..593cc290f 100644 --- a/wp_mobile_cache/src/test_fixtures.rs +++ b/wp_mobile_cache/src/test_fixtures.rs @@ -52,8 +52,19 @@ fn test_db() -> Connection { .expect("All migrations should succeed"); // Insert default test site (id = 1) - conn.execute("INSERT INTO sites (id) VALUES (1)", []) - .expect("Failed to insert test site"); + // First insert into self_hosted_sites + conn.execute( + "INSERT INTO self_hosted_sites (id, url, api_root) VALUES (1, 'https://example.com', 'https://example.com/wp-json')", + [] + ) + .expect("Failed to insert self-hosted site"); + + // Then insert into sites referencing the self_hosted_sites entry + conn.execute( + "INSERT INTO sites (id, site_type, mapped_site_id) VALUES (1, 0, 1)", + [], + ) + .expect("Failed to insert test site"); conn } @@ -62,8 +73,24 @@ fn test_db() -> Connection { /// /// Useful when you need more than 2 sites or specific site IDs. pub fn create_test_site(conn: &Connection, id: i64) -> DbSite { - conn.execute("INSERT INTO sites (id) VALUES (?)", [id]) - .expect("Failed to create test site"); + // First insert into self_hosted_sites + conn.execute( + "INSERT INTO self_hosted_sites (id, url, api_root) VALUES (?, ?, ?)", + ( + id, + format!("https://example-{}.com", id), + format!("https://example-{}.com/wp-json", id), + ), + ) + .expect("Failed to insert self-hosted site"); + + // Then insert into sites + conn.execute( + "INSERT INTO sites (id, site_type, mapped_site_id) VALUES (?, 0, ?)", + (id, id), + ) + .expect("Failed to create test site"); + DbSite { row_id: RowId(id as u64), site_type: crate::DbSiteType::SelfHosted, From e5e9ce9d6972cb44329634ac7497371800f8803c Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 00:43:17 -0400 Subject: [PATCH 06/20] Implement SiteRepository for managing self-hosted sites Add repository layer for upserting and querying self-hosted WordPress sites. Changes: - Create `SiteRepository` in `repository/sites.rs` - Add table name constants for `self_hosted_sites` and `sites` - Implement `upsert_self_hosted_site` with RETURNING optimization - Implement `select_self_hosted_site` to query by DbSite reference - Implement `select_self_hosted_site_by_url` to query by URL - Add module export in `repository/mod.rs` --- wp_mobile_cache/src/repository/mod.rs | 1 + wp_mobile_cache/src/repository/sites.rs | 149 ++++++++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 wp_mobile_cache/src/repository/sites.rs diff --git a/wp_mobile_cache/src/repository/mod.rs b/wp_mobile_cache/src/repository/mod.rs index 791a10a3b..b2d4f9122 100644 --- a/wp_mobile_cache/src/repository/mod.rs +++ b/wp_mobile_cache/src/repository/mod.rs @@ -2,6 +2,7 @@ use crate::{RowId, SqliteDbError}; use rusqlite::Connection; pub mod posts; +pub mod sites; pub mod term_relationships; #[cfg(test)] diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs new file mode 100644 index 000000000..3bc5786d5 --- /dev/null +++ b/wp_mobile_cache/src/repository/sites.rs @@ -0,0 +1,149 @@ +use crate::{ + DbSite, DbSiteType, RowId, SqliteDbError, + db_types::{ + row_ext::RowExt, + self_hosted_site::{DbSelfHostedSite, SelfHostedSiteColumn}, + }, + repository::QueryExecutor, +}; +use rusqlite::OptionalExtension; + +pub struct SiteRepository; + +impl SiteRepository { + const SELF_HOSTED_SITES_TABLE: &'static str = "self_hosted_sites"; + const SITES_TABLE: &'static str = "sites"; + + /// Upsert a self-hosted site and return both the DbSite and DbSelfHostedSite. + /// + /// If a site with the given URL already exists, updates it. Otherwise creates a new one. + /// Uses SQLite's RETURNING clause to get the inserted/updated rowid. + pub fn upsert_self_hosted_site( + &self, + executor: &impl QueryExecutor, + url: &str, + api_root: &str, + ) -> Result<(DbSite, DbSelfHostedSite), SqliteDbError> { + // Upsert into self_hosted_sites and get the rowid + let sql = format!( + "INSERT INTO {} (url, api_root) VALUES (?, ?) + ON CONFLICT(url) DO UPDATE SET api_root = excluded.api_root + RETURNING rowid", + Self::SELF_HOSTED_SITES_TABLE + ); + + let mut stmt = executor.prepare(&sql)?; + let self_hosted_site_id: RowId = stmt + .query_row((url, api_root), |row| row.get(0)) + .map_err(SqliteDbError::from)?; + + // Insert into sites table + let sql = format!( + "INSERT INTO {} (site_type, mapped_site_id) VALUES (?, ?) + RETURNING rowid", + Self::SITES_TABLE + ); + + let mut stmt = executor.prepare(&sql)?; + let site_id: RowId = stmt + .query_row((DbSiteType::SelfHosted, self_hosted_site_id), |row| { + row.get(0) + }) + .map_err(SqliteDbError::from)?; + + let db_site = DbSite { + row_id: site_id, + site_type: DbSiteType::SelfHosted, + mapped_site_id: self_hosted_site_id, + }; + + let db_self_hosted_site = DbSelfHostedSite { + row_id: self_hosted_site_id, + url: url.to_string(), + api_root: api_root.to_string(), + }; + + Ok((db_site, db_self_hosted_site)) + } + + /// Select a self-hosted site by its DbSite reference. + /// + /// Returns None if the site doesn't exist or isn't a self-hosted site. + pub fn select_self_hosted_site( + &self, + executor: &impl QueryExecutor, + site: &DbSite, + ) -> Result, SqliteDbError> { + if site.site_type != DbSiteType::SelfHosted { + return Ok(None); + } + + let sql = format!( + "SELECT * FROM {} WHERE rowid = ?", + Self::SELF_HOSTED_SITES_TABLE + ); + let mut stmt = executor.prepare(&sql)?; + + stmt.query_row([site.mapped_site_id], |row| { + Ok(DbSelfHostedSite { + row_id: row.get_column(SelfHostedSiteColumn::Rowid)?, + url: row.get_column(SelfHostedSiteColumn::Url)?, + api_root: row.get_column(SelfHostedSiteColumn::ApiRoot)?, + }) + }) + .optional() + .map_err(SqliteDbError::from) + } + + /// Select a self-hosted site by URL. + /// + /// Returns both the DbSite and DbSelfHostedSite if found. + pub fn select_self_hosted_site_by_url( + &self, + executor: &impl QueryExecutor, + url: &str, + ) -> Result, SqliteDbError> { + // First get the self_hosted_site + let sql = format!( + "SELECT * FROM {} WHERE url = ?", + Self::SELF_HOSTED_SITES_TABLE + ); + let mut stmt = executor.prepare(&sql)?; + + let self_hosted_site: Option = stmt + .query_row([url], |row| { + Ok(DbSelfHostedSite { + row_id: row.get_column(SelfHostedSiteColumn::Rowid)?, + url: row.get_column(SelfHostedSiteColumn::Url)?, + api_root: row.get_column(SelfHostedSiteColumn::ApiRoot)?, + }) + }) + .optional() + .map_err(SqliteDbError::from)?; + + let Some(self_hosted_site) = self_hosted_site else { + return Ok(None); + }; + + // Then find the corresponding site + let sql = format!( + "SELECT rowid, site_type, mapped_site_id FROM {} + WHERE site_type = ? AND mapped_site_id = ?", + Self::SITES_TABLE + ); + let mut stmt = executor.prepare(&sql)?; + + let db_site: Option = stmt + .query_row((DbSiteType::SelfHosted, self_hosted_site.row_id), |row| { + Ok(DbSite { + row_id: row.get(0)?, + site_type: row.get(1)?, + mapped_site_id: row.get(2)?, + }) + }) + .optional() + .map_err(SqliteDbError::from)?; + + Ok(db_site.map(|site| (site, self_hosted_site))) + } +} From 072cc52061d2897e30b7440c534e254a6919676f Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 19:08:18 -0400 Subject: [PATCH 07/20] Introduce SelfHostedSite domain type for better API design Create a separate domain type for self-hosted sites that doesn't include database metadata. This improves the API design and makes it easier to add more fields in the future. Changes: - Add `SelfHostedSite` type with `url` and `api_root` fields - Update `upsert_self_hosted_site` to take `&SelfHostedSite` instead of individual parameters - Add `to_self_hosted_site()` method on `DbSelfHostedSite` for conversion --- .../src/db_types/self_hosted_site.rs | 22 +++++++++++++++++++ wp_mobile_cache/src/repository/sites.rs | 11 +++++----- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/wp_mobile_cache/src/db_types/self_hosted_site.rs b/wp_mobile_cache/src/db_types/self_hosted_site.rs index 864c3e2fa..c00ccab01 100644 --- a/wp_mobile_cache/src/db_types/self_hosted_site.rs +++ b/wp_mobile_cache/src/db_types/self_hosted_site.rs @@ -16,9 +16,31 @@ impl ColumnIndex for SelfHostedSiteColumn { } } +/// Represents a self-hosted WordPress site (domain model). +/// +/// This type contains the site data without database metadata. +/// Use this when creating or updating sites. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SelfHostedSite { + pub url: String, + pub api_root: String, +} + /// Represents a self-hosted WordPress site in the database. +/// +/// This type includes the database rowid along with the site data. pub struct DbSelfHostedSite { pub row_id: RowId, pub url: String, pub api_root: String, } + +impl DbSelfHostedSite { + /// Convert to the domain model (without row_id). + pub fn to_self_hosted_site(&self) -> SelfHostedSite { + SelfHostedSite { + url: self.url.clone(), + api_root: self.api_root.clone(), + } + } +} diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index 3bc5786d5..cc3c53379 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -2,7 +2,7 @@ use crate::{ DbSite, DbSiteType, RowId, SqliteDbError, db_types::{ row_ext::RowExt, - self_hosted_site::{DbSelfHostedSite, SelfHostedSiteColumn}, + self_hosted_site::{DbSelfHostedSite, SelfHostedSite, SelfHostedSiteColumn}, }, repository::QueryExecutor, }; @@ -21,8 +21,7 @@ impl SiteRepository { pub fn upsert_self_hosted_site( &self, executor: &impl QueryExecutor, - url: &str, - api_root: &str, + site: &SelfHostedSite, ) -> Result<(DbSite, DbSelfHostedSite), SqliteDbError> { // Upsert into self_hosted_sites and get the rowid let sql = format!( @@ -34,7 +33,7 @@ impl SiteRepository { let mut stmt = executor.prepare(&sql)?; let self_hosted_site_id: RowId = stmt - .query_row((url, api_root), |row| row.get(0)) + .query_row((&site.url, &site.api_root), |row| row.get(0)) .map_err(SqliteDbError::from)?; // Insert into sites table @@ -59,8 +58,8 @@ impl SiteRepository { let db_self_hosted_site = DbSelfHostedSite { row_id: self_hosted_site_id, - url: url.to_string(), - api_root: api_root.to_string(), + url: site.url.clone(), + api_root: site.api_root.clone(), }; Ok((db_site, db_self_hosted_site)) From 90a016573172f5ee2e32f0d5bcf2b169aed98074 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 20:44:38 -0400 Subject: [PATCH 08/20] Use SiteRepository in test fixtures with real test credentials Update test fixtures to use `SiteRepository` for creating sites instead of manually constructing `DbSite` instances. This ensures test sites are properly inserted into the database and uses real test credentials from `integration_test_credentials`. Changes: - Add `integration_test_credentials` as dev dependency - Modify `test_db()` to return both `Connection` and `DbSite` - Update `test_ctx()` to use actual `DbSite` from repository - Change `create_test_site()` signature to accept `&SelfHostedSite` parameter - Update all test call sites to create `SelfHostedSite` instances - Use real test credentials in default test site creation --- Cargo.lock | 1 + wp_mobile_cache/Cargo.toml | 1 + .../src/repository/posts_multi_site_tests.rs | 45 +++++++++-- .../term_relationships_multi_site_tests.rs | 13 +++- wp_mobile_cache/src/test_fixtures.rs | 77 +++++++------------ 5 files changed, 80 insertions(+), 57 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9bc0f9629..166c3907a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5306,6 +5306,7 @@ name = "wp_mobile_cache" version = "0.1.0" dependencies = [ "chrono", + "integration_test_credentials", "rstest", "rusqlite", "serde", diff --git a/wp_mobile_cache/Cargo.toml b/wp_mobile_cache/Cargo.toml index ed7e1323a..9816a14ca 100644 --- a/wp_mobile_cache/Cargo.toml +++ b/wp_mobile_cache/Cargo.toml @@ -16,4 +16,5 @@ wp_api = { path = "../wp_api" } [dev-dependencies] chrono = { workspace = true } +integration_test_credentials = { path = "../integration_test_credentials" } rstest = "0.24" diff --git a/wp_mobile_cache/src/repository/posts_multi_site_tests.rs b/wp_mobile_cache/src/repository/posts_multi_site_tests.rs index 7526e16aa..f17175926 100644 --- a/wp_mobile_cache/src/repository/posts_multi_site_tests.rs +++ b/wp_mobile_cache/src/repository/posts_multi_site_tests.rs @@ -1,12 +1,21 @@ //! Multi-site isolation tests for PostRepository. -use crate::test_fixtures::{TestContext, create_test_site, posts::PostBuilder, test_ctx}; +use crate::{ + db_types::self_hosted_site::SelfHostedSite, + test_fixtures::{TestContext, create_test_site, posts::PostBuilder, test_ctx}, +}; use rstest::*; use wp_api::posts::PostId; #[rstest] fn test_posts_in_site_1_invisible_to_site_2(mut test_ctx: TestContext) { - let site2 = create_test_site(&test_ctx.conn, 2); + let site2 = create_test_site( + &test_ctx.conn, + &SelfHostedSite { + url: "https://example-2.com".to_string(), + api_root: "https://example-2.com/wp-json".to_string(), + }, + ); // Insert post in site 1 let post = PostBuilder::minimal().with_id(100).build(); @@ -28,7 +37,13 @@ fn test_posts_in_site_1_invisible_to_site_2(mut test_ctx: TestContext) { #[rstest] fn test_same_post_id_can_exist_in_different_sites(mut test_ctx: TestContext) { - let site2 = create_test_site(&test_ctx.conn, 2); + let site2 = create_test_site( + &test_ctx.conn, + &SelfHostedSite { + url: "https://example-2.com".to_string(), + api_root: "https://example-2.com/wp-json".to_string(), + }, + ); // Create post with same ID in both sites let post_id = PostId(42); @@ -69,7 +84,13 @@ fn test_same_post_id_can_exist_in_different_sites(mut test_ctx: TestContext) { #[rstest] fn test_select_all_only_returns_posts_for_requested_site(mut test_ctx: TestContext) { - let site2 = create_test_site(&test_ctx.conn, 2); + let site2 = create_test_site( + &test_ctx.conn, + &SelfHostedSite { + url: "https://example-2.com".to_string(), + api_root: "https://example-2.com/wp-json".to_string(), + }, + ); // Insert posts in site 1 test_ctx @@ -119,7 +140,13 @@ fn test_select_all_only_returns_posts_for_requested_site(mut test_ctx: TestConte #[rstest] fn test_count_only_counts_posts_for_requested_site(mut test_ctx: TestContext) { - let site2 = create_test_site(&test_ctx.conn, 2); + let site2 = create_test_site( + &test_ctx.conn, + &SelfHostedSite { + url: "https://example-2.com".to_string(), + api_root: "https://example-2.com/wp-json".to_string(), + }, + ); // Insert posts in both sites test_ctx @@ -156,7 +183,13 @@ fn test_count_only_counts_posts_for_requested_site(mut test_ctx: TestContext) { #[rstest] fn test_delete_by_post_id_only_deletes_from_specified_site(mut test_ctx: TestContext) { - let site2 = create_test_site(&test_ctx.conn, 2); + let site2 = create_test_site( + &test_ctx.conn, + &SelfHostedSite { + url: "https://example-2.com".to_string(), + api_root: "https://example-2.com/wp-json".to_string(), + }, + ); let post_id = PostId(999); diff --git a/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs b/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs index 73e52669f..c550af654 100644 --- a/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs +++ b/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs @@ -2,13 +2,22 @@ //! //! These tests verify that term relationships are correctly isolated between sites. -use crate::test_fixtures::{TestContext, create_test_site, posts::PostBuilder, test_ctx}; +use crate::{ + db_types::self_hosted_site::SelfHostedSite, + test_fixtures::{TestContext, create_test_site, posts::PostBuilder, test_ctx}, +}; use rstest::*; use wp_api::{taxonomies::TaxonomyType, terms::TermId}; #[rstest] fn test_term_relationships_isolated_by_site(mut test_ctx: TestContext) { - let site2 = create_test_site(&test_ctx.conn, 2); + let site2 = create_test_site( + &test_ctx.conn, + &SelfHostedSite { + url: "https://example-2.com".to_string(), + api_root: "https://example-2.com/wp-json".to_string(), + }, + ); // Insert post in site 1 with categories let post1 = PostBuilder::minimal() diff --git a/wp_mobile_cache/src/test_fixtures.rs b/wp_mobile_cache/src/test_fixtures.rs index 593cc290f..3059c43ea 100644 --- a/wp_mobile_cache/src/test_fixtures.rs +++ b/wp_mobile_cache/src/test_fixtures.rs @@ -1,9 +1,14 @@ use crate::{ - DbSite, MigrationManager, RowId, + DbSite, MigrationManager, context::EditContext, - repository::{posts::PostRepository, term_relationships::TermRelationshipRepository}, + db_types::self_hosted_site::SelfHostedSite, + repository::{ + posts::PostRepository, sites::SiteRepository, + term_relationships::TermRelationshipRepository, + }, }; use chrono::{DateTime, Utc}; +use integration_test_credentials::TestCredentials; use rstest::*; use rusqlite::Connection; @@ -31,19 +36,16 @@ pub struct TestContext { #[fixture] pub fn test_ctx() -> TestContext { + let (conn, site) = test_db(); TestContext { - conn: test_db(), - site: DbSite { - row_id: RowId(1), - site_type: crate::DbSiteType::SelfHosted, - mapped_site_id: RowId(1), - }, + conn, + site, post_repo: PostRepository::new(), term_repo: TermRelationshipRepository, } } -fn test_db() -> Connection { +fn test_db() -> (Connection, DbSite) { let conn = Connection::open_in_memory().unwrap(); let mut migration_manager = MigrationManager::new(&conn).unwrap(); @@ -51,51 +53,28 @@ fn test_db() -> Connection { .perform_migrations() .expect("All migrations should succeed"); - // Insert default test site (id = 1) - // First insert into self_hosted_sites - conn.execute( - "INSERT INTO self_hosted_sites (id, url, api_root) VALUES (1, 'https://example.com', 'https://example.com/wp-json')", - [] - ) - .expect("Failed to insert self-hosted site"); + // Insert default test site using real test credentials + let test_creds = TestCredentials::instance(); + let self_hosted_site = SelfHostedSite { + url: test_creds.site_url.to_string(), + api_root: format!("{}/wp-json", test_creds.site_url), + }; - // Then insert into sites referencing the self_hosted_sites entry - conn.execute( - "INSERT INTO sites (id, site_type, mapped_site_id) VALUES (1, 0, 1)", - [], - ) - .expect("Failed to insert test site"); + let db_site = create_test_site(&conn, &self_hosted_site); - conn + (conn, db_site) } -/// Helper to create an additional test site with a specific ID. +/// Helper to create a test site. /// -/// Useful when you need more than 2 sites or specific site IDs. -pub fn create_test_site(conn: &Connection, id: i64) -> DbSite { - // First insert into self_hosted_sites - conn.execute( - "INSERT INTO self_hosted_sites (id, url, api_root) VALUES (?, ?, ?)", - ( - id, - format!("https://example-{}.com", id), - format!("https://example-{}.com/wp-json", id), - ), - ) - .expect("Failed to insert self-hosted site"); - - // Then insert into sites - conn.execute( - "INSERT INTO sites (id, site_type, mapped_site_id) VALUES (?, 0, ?)", - (id, id), - ) - .expect("Failed to create test site"); - - DbSite { - row_id: RowId(id as u64), - site_type: crate::DbSiteType::SelfHosted, - mapped_site_id: RowId(id as u64), - } +/// Uses `SiteRepository` to insert the site into the database. +pub fn create_test_site(conn: &Connection, site: &SelfHostedSite) -> DbSite { + let site_repo = SiteRepository; + let (db_site, _) = site_repo + .upsert_self_hosted_site(conn, site) + .expect("Failed to upsert test site"); + + db_site } /// Validates that a timestamp is a recent, valid ISO 8601 UTC timestamp. From 012f410202735cccd4418486de28eb6b6733da8d Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 20:57:48 -0400 Subject: [PATCH 09/20] Remove unnecessary DbSelfHostedSite::to_self_hosted_site method --- wp_mobile_cache/src/db_types/self_hosted_site.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/wp_mobile_cache/src/db_types/self_hosted_site.rs b/wp_mobile_cache/src/db_types/self_hosted_site.rs index c00ccab01..cbfde0e11 100644 --- a/wp_mobile_cache/src/db_types/self_hosted_site.rs +++ b/wp_mobile_cache/src/db_types/self_hosted_site.rs @@ -34,13 +34,3 @@ pub struct DbSelfHostedSite { pub url: String, pub api_root: String, } - -impl DbSelfHostedSite { - /// Convert to the domain model (without row_id). - pub fn to_self_hosted_site(&self) -> SelfHostedSite { - SelfHostedSite { - url: self.url.clone(), - api_root: self.api_root.clone(), - } - } -} From 86777ee761341303aec5ae9ae8389d8b083f9aab Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 21:08:22 -0400 Subject: [PATCH 10/20] Fix SiteRepository upsert bug with unique constraint The `upsert_self_hosted_site` method had a data consistency bug where updating an existing site URL would create orphaned entries in the sites table. This occurred because only the self_hosted_sites table used upsert logic while the sites table always inserted. Changes: - Add unique constraint on `sites(site_type, mapped_site_id)` in migration 0001 - Update `upsert_self_hosted_site` to use ON CONFLICT clause for sites table - Ensures referential integrity between sites and self_hosted_sites tables --- wp_mobile_cache/migrations/0001-create-sites-table.sql | 3 +++ wp_mobile_cache/src/repository/sites.rs | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/wp_mobile_cache/migrations/0001-create-sites-table.sql b/wp_mobile_cache/migrations/0001-create-sites-table.sql index ebee11be0..448cfb074 100644 --- a/wp_mobile_cache/migrations/0001-create-sites-table.sql +++ b/wp_mobile_cache/migrations/0001-create-sites-table.sql @@ -9,3 +9,6 @@ CREATE TABLE `sites` ( -- Note: Not a foreign key constraint since it can point to different tables `mapped_site_id` INTEGER NOT NULL ) STRICT; + +-- Unique constraint to prevent duplicate site mappings +CREATE UNIQUE INDEX idx_sites_unique_site_type_and_mapped_site_id ON sites(site_type, mapped_site_id); diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index cc3c53379..4d9296b5b 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -36,9 +36,12 @@ impl SiteRepository { .query_row((&site.url, &site.api_root), |row| row.get(0)) .map_err(SqliteDbError::from)?; - // Insert into sites table + // Upsert into sites table + // If site_type + mapped_site_id already exists, reuse that entry let sql = format!( "INSERT INTO {} (site_type, mapped_site_id) VALUES (?, ?) + ON CONFLICT(site_type, mapped_site_id) DO UPDATE SET + site_type = excluded.site_type RETURNING rowid", Self::SITES_TABLE ); From 8aaf40ee4328720ea6d2dc43ec4de6cdd3225edc Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 21:51:54 -0400 Subject: [PATCH 11/20] Add comprehensive unit tests for SiteRepository Implemented test suite covering all SiteRepository methods with both positive and negative test cases. Tests use a simple fixture approach without TestContext to avoid pollution from default site creation. Changes: - Rename column from `id` to `rowid` in self_hosted_sites table for consistency with posts tables - Add `Debug`, `Clone`, `PartialEq`, `Eq` derives to `DbSelfHostedSite` for testability - Add `count_all_sites()` method to `SiteRepository` (for testing) - Add schema validation test for `SelfHostedSiteColumn` enum - Add tests for `upsert_self_hosted_site` (insert and update behavior) - Add test verifying the duplicate sites bug fix using `count_all_sites()` method - Add tests for `select_self_hosted_site` (by DbSite reference) - Add tests for `select_self_hosted_site_by_url` - Add edge case tests (non-existent sites, wrong site types) - Add test for multiple sites coexisting - Tests use simple `test_conn` fixture instead of `TestContext` - Tests rely on rowid equality to prove update behavior (no unnecessary counts) All tests pass (67 total, 10 new for SiteRepository). --- .../0006-create-self-hosted-sites-table.sql | 2 +- .../src/db_types/self_hosted_site.rs | 1 + wp_mobile_cache/src/repository/sites.rs | 276 ++++++++++++++++++ 3 files changed, 278 insertions(+), 1 deletion(-) diff --git a/wp_mobile_cache/migrations/0006-create-self-hosted-sites-table.sql b/wp_mobile_cache/migrations/0006-create-self-hosted-sites-table.sql index 0898a0c86..c57e605d0 100644 --- a/wp_mobile_cache/migrations/0006-create-self-hosted-sites-table.sql +++ b/wp_mobile_cache/migrations/0006-create-self-hosted-sites-table.sql @@ -1,6 +1,6 @@ CREATE TABLE `self_hosted_sites` ( -- Internal DB field (auto-incrementing) - `id` INTEGER PRIMARY KEY AUTOINCREMENT, + `rowid` INTEGER PRIMARY KEY AUTOINCREMENT, -- Site URL (unique constraint for upsert logic) `url` TEXT NOT NULL UNIQUE, diff --git a/wp_mobile_cache/src/db_types/self_hosted_site.rs b/wp_mobile_cache/src/db_types/self_hosted_site.rs index cbfde0e11..e98a3c968 100644 --- a/wp_mobile_cache/src/db_types/self_hosted_site.rs +++ b/wp_mobile_cache/src/db_types/self_hosted_site.rs @@ -29,6 +29,7 @@ pub struct SelfHostedSite { /// Represents a self-hosted WordPress site in the database. /// /// This type includes the database rowid along with the site data. +#[derive(Debug, Clone, PartialEq, Eq)] pub struct DbSelfHostedSite { pub row_id: RowId, pub url: String, diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index 4d9296b5b..a2cd655c0 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -148,4 +148,280 @@ impl SiteRepository { Ok(db_site.map(|site| (site, self_hosted_site))) } + + /// Count all sites in the database. + /// + /// This is primarily useful for testing to verify database state. + pub fn count_all_sites(&self, executor: &impl QueryExecutor) -> Result { + let sql = format!("SELECT COUNT(*) FROM {}", Self::SITES_TABLE); + let mut stmt = executor.prepare(&sql)?; + let count: i64 = stmt.query_row([], |row| row.get(0))?; + Ok(count as usize) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_fixtures::get_table_column_names; + use crate::{MigrationManager, db_types::row_ext::ColumnIndex}; + use rstest::*; + use rusqlite::Connection; + + #[fixture] + fn test_conn() -> Connection { + let conn = Connection::open_in_memory().unwrap(); + let mut migration_manager = MigrationManager::new(&conn).unwrap(); + migration_manager + .perform_migrations() + .expect("All migrations should succeed"); + conn + } + + /// Verify that SelfHostedSiteColumn enum values match the actual database schema. + /// This test protects against column reordering in migrations breaking the positional index mapping. + #[rstest] + fn test_self_hosted_site_column_enum_matches_schema(test_conn: Connection) { + use SelfHostedSiteColumn::*; + + let columns = get_table_column_names(&test_conn, "self_hosted_sites"); + + // Verify each enum value maps to the correct column name + assert_eq!(columns[Rowid.as_index()], "rowid"); + assert_eq!(columns[Url.as_index()], "url"); + assert_eq!(columns[ApiRoot.as_index()], "api_root"); + + // Verify total column count matches + assert_eq!(columns.len(), ApiRoot.as_index() + 1); + } + + #[rstest] + fn test_upsert_inserts_new_site(test_conn: Connection) { + let repo = SiteRepository; + let site = SelfHostedSite { + url: "https://example.com".to_string(), + api_root: "https://example.com/wp-json".to_string(), + }; + + let (db_site, db_self_hosted_site) = repo + .upsert_self_hosted_site(&test_conn, &site) + .expect("Failed to upsert site"); + + // Verify self_hosted_sites entry + assert_eq!(db_self_hosted_site.url, site.url); + assert_eq!(db_self_hosted_site.api_root, site.api_root); + + // Verify sites entry + assert_eq!(db_site.site_type, DbSiteType::SelfHosted); + assert_eq!(db_site.mapped_site_id, db_self_hosted_site.row_id); + } + + #[rstest] + fn test_upsert_updates_existing_site_url(test_conn: Connection) { + let repo = SiteRepository; + let url = "https://example.com"; + + // First insert + let site1 = SelfHostedSite { + url: url.to_string(), + api_root: "https://example.com/wp-json".to_string(), + }; + let (db_site1, db_self_hosted_site1) = repo + .upsert_self_hosted_site(&test_conn, &site1) + .expect("Failed to upsert site"); + + // Second upsert with same URL but different api_root + let site2 = SelfHostedSite { + url: url.to_string(), + api_root: "https://example.com/wordpress/wp-json".to_string(), + }; + let (db_site2, db_self_hosted_site2) = repo + .upsert_self_hosted_site(&test_conn, &site2) + .expect("Failed to upsert site"); + + // Verify it updated the same row (same rowid) - this proves update, not insert + assert_eq!(db_self_hosted_site1.row_id, db_self_hosted_site2.row_id); + assert_eq!(db_site1.row_id, db_site2.row_id); + + // Verify api_root was updated + assert_eq!(db_self_hosted_site2.api_root, site2.api_root); + } + + #[rstest] + fn test_upsert_does_not_create_duplicate_sites_entries(test_conn: Connection) { + let repo = SiteRepository; + + // Insert same site multiple times + let site = SelfHostedSite { + url: "https://example.com".to_string(), + api_root: "https://example.com/wp-json".to_string(), + }; + + let (db_site1, _) = repo + .upsert_self_hosted_site(&test_conn, &site) + .expect("First upsert failed"); + let (db_site2, _) = repo + .upsert_self_hosted_site(&test_conn, &site) + .expect("Second upsert failed"); + let (db_site3, _) = repo + .upsert_self_hosted_site(&test_conn, &site) + .expect("Third upsert failed"); + + // All should return the same DbSite + assert_eq!(db_site1.row_id, db_site2.row_id); + assert_eq!(db_site2.row_id, db_site3.row_id); + + // Verify only one entry exists in sites table (ensures bug fix works) + let count = repo.count_all_sites(&test_conn).unwrap(); + assert_eq!( + count, 1, + "Multiple upserts should not create duplicate sites table entries" + ); + } + + #[rstest] + fn test_select_self_hosted_site_by_db_site(test_conn: Connection) { + let repo = SiteRepository; + let site = SelfHostedSite { + url: "https://example.com".to_string(), + api_root: "https://example.com/wp-json".to_string(), + }; + + let (db_site, original_db_self_hosted_site) = repo + .upsert_self_hosted_site(&test_conn, &site) + .expect("Failed to upsert site"); + + // Select using DbSite reference + let retrieved = repo + .select_self_hosted_site(&test_conn, &db_site) + .expect("Failed to select site") + .expect("Site should exist"); + + assert_eq!(retrieved.row_id, original_db_self_hosted_site.row_id); + assert_eq!(retrieved.url, original_db_self_hosted_site.url); + assert_eq!(retrieved.api_root, original_db_self_hosted_site.api_root); + } + + #[rstest] + fn test_select_self_hosted_site_returns_none_for_wrong_site_type(test_conn: Connection) { + let repo = SiteRepository; + + // Create a DbSite with WordPressCom type (not inserted, just for testing) + let non_self_hosted_site = DbSite { + row_id: RowId(999), + site_type: DbSiteType::WordPressCom, + mapped_site_id: RowId(999), + }; + + let result = repo + .select_self_hosted_site(&test_conn, &non_self_hosted_site) + .expect("Query should succeed"); + + assert_eq!( + result, None, + "Should return None for non-SelfHosted site type" + ); + } + + #[rstest] + fn test_select_self_hosted_site_returns_none_for_non_existent_site(test_conn: Connection) { + let repo = SiteRepository; + + let non_existent_site = DbSite { + row_id: RowId(999), + site_type: DbSiteType::SelfHosted, + mapped_site_id: RowId(999), + }; + + let result = repo + .select_self_hosted_site(&test_conn, &non_existent_site) + .expect("Query should succeed"); + + assert_eq!(result, None, "Should return None for non-existent site"); + } + + #[rstest] + fn test_select_self_hosted_site_by_url(test_conn: Connection) { + let repo = SiteRepository; + let site = SelfHostedSite { + url: "https://example.com".to_string(), + api_root: "https://example.com/wp-json".to_string(), + }; + + let (original_db_site, original_db_self_hosted_site) = repo + .upsert_self_hosted_site(&test_conn, &site) + .expect("Failed to upsert site"); + + // Select by URL + let (retrieved_db_site, retrieved_db_self_hosted_site) = repo + .select_self_hosted_site_by_url(&test_conn, &site.url) + .expect("Failed to select site") + .expect("Site should exist"); + + // Verify DbSite matches + assert_eq!(retrieved_db_site.row_id, original_db_site.row_id); + assert_eq!(retrieved_db_site.site_type, original_db_site.site_type); + assert_eq!( + retrieved_db_site.mapped_site_id, + original_db_site.mapped_site_id + ); + + // Verify DbSelfHostedSite matches + assert_eq!( + retrieved_db_self_hosted_site.row_id, + original_db_self_hosted_site.row_id + ); + assert_eq!( + retrieved_db_self_hosted_site.url, + original_db_self_hosted_site.url + ); + assert_eq!( + retrieved_db_self_hosted_site.api_root, + original_db_self_hosted_site.api_root + ); + } + + #[rstest] + fn test_select_self_hosted_site_by_url_returns_none_for_non_existent_url( + test_conn: Connection, + ) { + let repo = SiteRepository; + + let result = repo + .select_self_hosted_site_by_url(&test_conn, "https://non-existent.com") + .expect("Query should succeed"); + + assert_eq!(result, None, "Should return None for non-existent URL"); + } + + #[rstest] + fn test_multiple_different_sites_can_coexist(test_conn: Connection) { + let repo = SiteRepository; + + let site1 = SelfHostedSite { + url: "https://example1.com".to_string(), + api_root: "https://example1.com/wp-json".to_string(), + }; + let site2 = SelfHostedSite { + url: "https://example2.com".to_string(), + api_root: "https://example2.com/wp-json".to_string(), + }; + + let (db_site1, _) = repo.upsert_self_hosted_site(&test_conn, &site1).unwrap(); + let (db_site2, _) = repo.upsert_self_hosted_site(&test_conn, &site2).unwrap(); + + // Verify different sites get different IDs + assert_ne!(db_site1.row_id, db_site2.row_id); + + // Verify both can be retrieved by URL + let retrieved1 = repo + .select_self_hosted_site_by_url(&test_conn, &site1.url) + .unwrap(); + let retrieved2 = repo + .select_self_hosted_site_by_url(&test_conn, &site2.url) + .unwrap(); + + assert!(retrieved1.is_some()); + assert!(retrieved2.is_some()); + } } From 4e5b231e69314ae8fe3425778defe292b3750e1e Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 22:22:27 -0400 Subject: [PATCH 12/20] Rename SelfHostedSiteColumn to DbSelfHostedSiteColumn For consistency with the struct naming pattern where `DbSelfHostedSite` is the database representation, the column enum should be `DbSelfHostedSiteColumn`. Changes: - Rename `SelfHostedSiteColumn` to `DbSelfHostedSiteColumn` - Update all references in `SiteRepository` - Update test name to match new enum name --- .../src/db_types/self_hosted_site.rs | 4 ++-- wp_mobile_cache/src/repository/sites.rs | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/wp_mobile_cache/src/db_types/self_hosted_site.rs b/wp_mobile_cache/src/db_types/self_hosted_site.rs index e98a3c968..88c8c5559 100644 --- a/wp_mobile_cache/src/db_types/self_hosted_site.rs +++ b/wp_mobile_cache/src/db_types/self_hosted_site.rs @@ -4,13 +4,13 @@ use crate::{RowId, db_types::row_ext::ColumnIndex}; /// These must match the order of columns in the CREATE TABLE statement. #[repr(usize)] #[derive(Debug, Clone, Copy)] -pub(crate) enum SelfHostedSiteColumn { +pub(crate) enum DbSelfHostedSiteColumn { Rowid = 0, Url = 1, ApiRoot = 2, } -impl ColumnIndex for SelfHostedSiteColumn { +impl ColumnIndex for DbSelfHostedSiteColumn { fn as_index(&self) -> usize { *self as usize } diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index a2cd655c0..01e9bbc46 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -2,7 +2,7 @@ use crate::{ DbSite, DbSiteType, RowId, SqliteDbError, db_types::{ row_ext::RowExt, - self_hosted_site::{DbSelfHostedSite, SelfHostedSite, SelfHostedSiteColumn}, + self_hosted_site::{DbSelfHostedSite, DbSelfHostedSiteColumn, SelfHostedSite}, }, repository::QueryExecutor, }; @@ -88,9 +88,9 @@ impl SiteRepository { stmt.query_row([site.mapped_site_id], |row| { Ok(DbSelfHostedSite { - row_id: row.get_column(SelfHostedSiteColumn::Rowid)?, - url: row.get_column(SelfHostedSiteColumn::Url)?, - api_root: row.get_column(SelfHostedSiteColumn::ApiRoot)?, + row_id: row.get_column(DbSelfHostedSiteColumn::Rowid)?, + url: row.get_column(DbSelfHostedSiteColumn::Url)?, + api_root: row.get_column(DbSelfHostedSiteColumn::ApiRoot)?, }) }) .optional() @@ -115,9 +115,9 @@ impl SiteRepository { let self_hosted_site: Option = stmt .query_row([url], |row| { Ok(DbSelfHostedSite { - row_id: row.get_column(SelfHostedSiteColumn::Rowid)?, - url: row.get_column(SelfHostedSiteColumn::Url)?, - api_root: row.get_column(SelfHostedSiteColumn::ApiRoot)?, + row_id: row.get_column(DbSelfHostedSiteColumn::Rowid)?, + url: row.get_column(DbSelfHostedSiteColumn::Url)?, + api_root: row.get_column(DbSelfHostedSiteColumn::ApiRoot)?, }) }) .optional() @@ -178,11 +178,11 @@ mod tests { conn } - /// Verify that SelfHostedSiteColumn enum values match the actual database schema. + /// Verify that DbSelfHostedSiteColumn enum values match the actual database schema. /// This test protects against column reordering in migrations breaking the positional index mapping. #[rstest] fn test_self_hosted_site_column_enum_matches_schema(test_conn: Connection) { - use SelfHostedSiteColumn::*; + use DbSelfHostedSiteColumn::*; let columns = get_table_column_names(&test_conn, "self_hosted_sites"); From 7348d524d422fca458ea26733422762b65e3fcc0 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 22:26:18 -0400 Subject: [PATCH 13/20] Remove public re-export of DbSite and DbSiteType These types are only used internally by repositories and tests, not exposed in any public API methods. Keeping them internal provides better encapsulation. Changes: - Remove `pub use db_types::db_site::{DbSite, DbSiteType}` from lib.rs - Update all modules to import directly from `crate::db_types::db_site` - Update test modules that were using `crate::DbSiteType` to use the direct import All tests pass (67 total). --- wp_mobile_cache/src/lib.rs | 3 --- wp_mobile_cache/src/repository/posts.rs | 3 ++- wp_mobile_cache/src/repository/posts_constraint_tests.rs | 5 +++-- wp_mobile_cache/src/repository/posts_transaction_tests.rs | 5 +++-- wp_mobile_cache/src/repository/sites.rs | 3 ++- wp_mobile_cache/src/repository/term_relationships.rs | 3 ++- wp_mobile_cache/src/test_fixtures.rs | 4 ++-- 7 files changed, 14 insertions(+), 12 deletions(-) diff --git a/wp_mobile_cache/src/lib.rs b/wp_mobile_cache/src/lib.rs index 332898acc..f43ebbd84 100644 --- a/wp_mobile_cache/src/lib.rs +++ b/wp_mobile_cache/src/lib.rs @@ -84,9 +84,6 @@ impl From for i64 { } } -// Re-export site types from db_types module -pub use db_types::db_site::{DbSite, DbSiteType}; - /// Get the SQLite version string from the database. /// /// This function queries the database for its SQLite version using `SELECT sqlite_version()`. diff --git a/wp_mobile_cache/src/repository/posts.rs b/wp_mobile_cache/src/repository/posts.rs index 253597c81..c9d4cc95c 100644 --- a/wp_mobile_cache/src/repository/posts.rs +++ b/wp_mobile_cache/src/repository/posts.rs @@ -1,7 +1,8 @@ use crate::{ - DbSite, RowId, SqliteDbError, + RowId, SqliteDbError, context::{EditContext, EmbedContext, IsContext, ViewContext}, db_types::{ + db_site::DbSite, helpers::{ bool_to_integer, deserialize_json_value, get_id, get_optional_id, integer_to_bool, parse_datetime, parse_enum, parse_optional_enum, serialize_value_to_json, diff --git a/wp_mobile_cache/src/repository/posts_constraint_tests.rs b/wp_mobile_cache/src/repository/posts_constraint_tests.rs index 47015b573..6bb1ceccc 100644 --- a/wp_mobile_cache/src/repository/posts_constraint_tests.rs +++ b/wp_mobile_cache/src/repository/posts_constraint_tests.rs @@ -4,7 +4,8 @@ //! and that error cases are handled appropriately. use crate::{ - DbSite, RowId, + RowId, + db_types::db_site::{DbSite, DbSiteType}, test_fixtures::{TestContext, posts::PostBuilder, test_ctx}, }; use rstest::*; @@ -60,7 +61,7 @@ fn test_invalid_site_id_fails_foreign_key_constraint(mut test_ctx: TestContext) // Site doesn't exist in database - intentionally invalid for testing error handling let non_existent_site = DbSite { row_id: RowId(999), - site_type: crate::DbSiteType::SelfHosted, + site_type: DbSiteType::SelfHosted, mapped_site_id: RowId(999), }; diff --git a/wp_mobile_cache/src/repository/posts_transaction_tests.rs b/wp_mobile_cache/src/repository/posts_transaction_tests.rs index 5350c0042..3cf3fe260 100644 --- a/wp_mobile_cache/src/repository/posts_transaction_tests.rs +++ b/wp_mobile_cache/src/repository/posts_transaction_tests.rs @@ -4,7 +4,8 @@ //! ensuring proper rollback on errors without leaving partial writes or corrupted data. use crate::{ - DbSite, RowId, + RowId, + db_types::db_site::{DbSite, DbSiteType}, test_fixtures::{TestContext, posts::PostBuilder, test_ctx}, }; use rstest::*; @@ -78,7 +79,7 @@ fn test_upsert_batch_fails_on_foreign_key_violation(mut test_ctx: TestContext) { // Site doesn't exist in database - intentionally invalid for testing error handling let invalid_site = DbSite { row_id: RowId(999), - site_type: crate::DbSiteType::SelfHosted, + site_type: DbSiteType::SelfHosted, mapped_site_id: RowId(999), }; diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index 01e9bbc46..011055a4b 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -1,6 +1,7 @@ use crate::{ - DbSite, DbSiteType, RowId, SqliteDbError, + RowId, SqliteDbError, db_types::{ + db_site::{DbSite, DbSiteType}, row_ext::RowExt, self_hosted_site::{DbSelfHostedSite, DbSelfHostedSiteColumn, SelfHostedSite}, }, diff --git a/wp_mobile_cache/src/repository/term_relationships.rs b/wp_mobile_cache/src/repository/term_relationships.rs index 0557f5851..c9fbb7acf 100644 --- a/wp_mobile_cache/src/repository/term_relationships.rs +++ b/wp_mobile_cache/src/repository/term_relationships.rs @@ -1,5 +1,6 @@ use crate::{ - DbSite, SqliteDbError, + SqliteDbError, + db_types::db_site::DbSite, repository::{InTransaction, QueryExecutor}, term_relationships::DbTermRelationship, }; diff --git a/wp_mobile_cache/src/test_fixtures.rs b/wp_mobile_cache/src/test_fixtures.rs index 3059c43ea..41b766ab5 100644 --- a/wp_mobile_cache/src/test_fixtures.rs +++ b/wp_mobile_cache/src/test_fixtures.rs @@ -1,7 +1,7 @@ use crate::{ - DbSite, MigrationManager, + MigrationManager, context::EditContext, - db_types::self_hosted_site::SelfHostedSite, + db_types::{db_site::DbSite, self_hosted_site::SelfHostedSite}, repository::{ posts::PostRepository, sites::SiteRepository, term_relationships::TermRelationshipRepository, From 4c4c36a843086ec433399b32c5fa263d8ef29f22 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 22:30:41 -0400 Subject: [PATCH 14/20] Add create_random_test_site helper to reduce test verbosity Multi-site tests were verbose, requiring explicit URL construction for each test site. This helper uses an internal atomic counter to generate unique URLs automatically. Changes: - Add `create_random_test_site()` helper function - Uses `RANDOM_TEST_SITE_COUNTER` static atomic to generate unique URLs - Update `posts_multi_site_tests.rs` to use new helper - Update `term_relationships_multi_site_tests.rs` to use new helper - Reduces test site creation from 6 lines to 1 line All tests pass (67 total). --- .../src/repository/posts_multi_site_tests.rs | 45 +++---------------- .../term_relationships_multi_site_tests.rs | 13 +----- wp_mobile_cache/src/test_fixtures.rs | 24 ++++++++++ 3 files changed, 32 insertions(+), 50 deletions(-) diff --git a/wp_mobile_cache/src/repository/posts_multi_site_tests.rs b/wp_mobile_cache/src/repository/posts_multi_site_tests.rs index f17175926..c3699d470 100644 --- a/wp_mobile_cache/src/repository/posts_multi_site_tests.rs +++ b/wp_mobile_cache/src/repository/posts_multi_site_tests.rs @@ -1,21 +1,12 @@ //! Multi-site isolation tests for PostRepository. -use crate::{ - db_types::self_hosted_site::SelfHostedSite, - test_fixtures::{TestContext, create_test_site, posts::PostBuilder, test_ctx}, -}; +use crate::test_fixtures::{TestContext, create_random_test_site, posts::PostBuilder, test_ctx}; use rstest::*; use wp_api::posts::PostId; #[rstest] fn test_posts_in_site_1_invisible_to_site_2(mut test_ctx: TestContext) { - let site2 = create_test_site( - &test_ctx.conn, - &SelfHostedSite { - url: "https://example-2.com".to_string(), - api_root: "https://example-2.com/wp-json".to_string(), - }, - ); + let site2 = create_random_test_site(&test_ctx.conn); // Insert post in site 1 let post = PostBuilder::minimal().with_id(100).build(); @@ -37,13 +28,7 @@ fn test_posts_in_site_1_invisible_to_site_2(mut test_ctx: TestContext) { #[rstest] fn test_same_post_id_can_exist_in_different_sites(mut test_ctx: TestContext) { - let site2 = create_test_site( - &test_ctx.conn, - &SelfHostedSite { - url: "https://example-2.com".to_string(), - api_root: "https://example-2.com/wp-json".to_string(), - }, - ); + let site2 = create_random_test_site(&test_ctx.conn); // Create post with same ID in both sites let post_id = PostId(42); @@ -84,13 +69,7 @@ fn test_same_post_id_can_exist_in_different_sites(mut test_ctx: TestContext) { #[rstest] fn test_select_all_only_returns_posts_for_requested_site(mut test_ctx: TestContext) { - let site2 = create_test_site( - &test_ctx.conn, - &SelfHostedSite { - url: "https://example-2.com".to_string(), - api_root: "https://example-2.com/wp-json".to_string(), - }, - ); + let site2 = create_random_test_site(&test_ctx.conn); // Insert posts in site 1 test_ctx @@ -140,13 +119,7 @@ fn test_select_all_only_returns_posts_for_requested_site(mut test_ctx: TestConte #[rstest] fn test_count_only_counts_posts_for_requested_site(mut test_ctx: TestContext) { - let site2 = create_test_site( - &test_ctx.conn, - &SelfHostedSite { - url: "https://example-2.com".to_string(), - api_root: "https://example-2.com/wp-json".to_string(), - }, - ); + let site2 = create_random_test_site(&test_ctx.conn); // Insert posts in both sites test_ctx @@ -183,13 +156,7 @@ fn test_count_only_counts_posts_for_requested_site(mut test_ctx: TestContext) { #[rstest] fn test_delete_by_post_id_only_deletes_from_specified_site(mut test_ctx: TestContext) { - let site2 = create_test_site( - &test_ctx.conn, - &SelfHostedSite { - url: "https://example-2.com".to_string(), - api_root: "https://example-2.com/wp-json".to_string(), - }, - ); + let site2 = create_random_test_site(&test_ctx.conn); let post_id = PostId(999); diff --git a/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs b/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs index c550af654..61c78ecec 100644 --- a/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs +++ b/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs @@ -2,22 +2,13 @@ //! //! These tests verify that term relationships are correctly isolated between sites. -use crate::{ - db_types::self_hosted_site::SelfHostedSite, - test_fixtures::{TestContext, create_test_site, posts::PostBuilder, test_ctx}, -}; +use crate::test_fixtures::{TestContext, create_random_test_site, posts::PostBuilder, test_ctx}; use rstest::*; use wp_api::{taxonomies::TaxonomyType, terms::TermId}; #[rstest] fn test_term_relationships_isolated_by_site(mut test_ctx: TestContext) { - let site2 = create_test_site( - &test_ctx.conn, - &SelfHostedSite { - url: "https://example-2.com".to_string(), - api_root: "https://example-2.com/wp-json".to_string(), - }, - ); + let site2 = create_random_test_site(&test_ctx.conn); // Insert post in site 1 with categories let post1 = PostBuilder::minimal() diff --git a/wp_mobile_cache/src/test_fixtures.rs b/wp_mobile_cache/src/test_fixtures.rs index 41b766ab5..747cbb66b 100644 --- a/wp_mobile_cache/src/test_fixtures.rs +++ b/wp_mobile_cache/src/test_fixtures.rs @@ -11,6 +11,7 @@ use chrono::{DateTime, Utc}; use integration_test_credentials::TestCredentials; use rstest::*; use rusqlite::Connection; +use std::sync::atomic::{AtomicU32, Ordering}; pub mod posts; @@ -77,6 +78,29 @@ pub fn create_test_site(conn: &Connection, site: &SelfHostedSite) -> DbSite { db_site } +static RANDOM_TEST_SITE_COUNTER: AtomicU32 = AtomicU32::new(1); + +/// Helper to create a test site with auto-generated URL. +/// +/// Uses an internal counter to generate unique URLs for each call. +/// Useful for tests that need multiple sites but don't care about specific URLs. +/// +/// # Example +/// +/// ```rust +/// let site1 = create_random_test_site(&conn); +/// let site2 = create_random_test_site(&conn); +/// // site1 and site2 will have different URLs +/// ``` +pub fn create_random_test_site(conn: &Connection) -> DbSite { + let counter = RANDOM_TEST_SITE_COUNTER.fetch_add(1, Ordering::Relaxed); + let site = SelfHostedSite { + url: format!("https://test-site-{}.local", counter), + api_root: format!("https://test-site-{}.local/wp-json", counter), + }; + create_test_site(conn, &site) +} + /// Validates that a timestamp is a recent, valid ISO 8601 UTC timestamp. /// /// Checks that the timestamp: From 5f67e2812c184e3510b3f3b3a31f3e6dcb59d875 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 22:44:59 -0400 Subject: [PATCH 15/20] Implement delete methods for SiteRepository with proper cleanup Added core `delete_site()` method that handles deletion across both the sites table and type-specific tables (self_hosted_sites). Also added a convenience wrapper `delete_self_hosted_site_by_url()` for common use cases. The implementation ensures proper cleanup since foreign key constraints cannot be used with polymorphic site references. Panics with a clear message if attempting to delete WordPress.com sites (not yet implemented). Changes: - Add `delete_site()` method that deletes from both tables based on site type - Add `delete_self_hosted_site_by_url()` convenience wrapper - Add `count_all_self_hosted_sites()` for testing - Panic with clear message for unimplemented WordPress.com site deletion - Add 5 comprehensive tests covering: - Deletion from both tables (using repository methods, no raw SQL) - Return value for non-existent sites - Deletion isolation between sites - URL-based deletion - Error cases All tests pass (72 total, 5 new for delete functionality). --- wp_mobile_cache/src/repository/sites.rs | 189 ++++++++++++++++++++++++ 1 file changed, 189 insertions(+) diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index 011055a4b..77a4e7a33 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -159,6 +159,68 @@ impl SiteRepository { let count: i64 = stmt.query_row([], |row| row.get(0))?; Ok(count as usize) } + + /// Count all self-hosted sites in the database. + /// + /// This is primarily useful for testing to verify database state. + pub fn count_all_self_hosted_sites( + &self, + executor: &impl QueryExecutor, + ) -> Result { + let sql = format!("SELECT COUNT(*) FROM {}", Self::SELF_HOSTED_SITES_TABLE); + let mut stmt = executor.prepare(&sql)?; + let count: i64 = stmt.query_row([], |row| row.get(0))?; + Ok(count as usize) + } + + /// Delete a site and its type-specific data. + /// + /// Deletes both the site entry and its corresponding type-specific entry + /// (e.g., from self_hosted_sites or wordpress_com_sites). This ensures proper + /// cleanup since foreign key constraints cannot be used with polymorphic references. + /// + /// Returns `true` if a site was deleted, `false` if the site didn't exist. + pub fn delete_site( + &self, + executor: &impl QueryExecutor, + site: &DbSite, + ) -> Result { + // Delete from type-specific table based on site_type + let _type_table_deleted = match site.site_type { + DbSiteType::SelfHosted => { + let sql = format!( + "DELETE FROM {} WHERE rowid = ?", + Self::SELF_HOSTED_SITES_TABLE + ); + executor.execute(&sql, [site.mapped_site_id])? + } + DbSiteType::WordPressCom => { + panic!("WordPress.com site deletion is not yet implemented") + } + }; + + // Delete from sites table + let sql = format!("DELETE FROM {} WHERE rowid = ?", Self::SITES_TABLE); + let sites_deleted = executor.execute(&sql, [site.row_id])?; + + Ok(sites_deleted > 0) + } + + /// Delete a self-hosted site by URL (convenience wrapper). + /// + /// Returns `true` if a site was deleted, `false` if no site with that URL exists. + pub fn delete_self_hosted_site_by_url( + &self, + executor: &impl QueryExecutor, + url: &str, + ) -> Result { + let site_data = self.select_self_hosted_site_by_url(executor, url)?; + + match site_data { + Some((db_site, _)) => self.delete_site(executor, &db_site), + None => Ok(false), + } + } } #[cfg(test)] @@ -425,4 +487,131 @@ mod tests { assert!(retrieved1.is_some()); assert!(retrieved2.is_some()); } + + #[rstest] + fn test_delete_site_removes_both_tables(test_conn: Connection) { + let repo = SiteRepository; + let site = SelfHostedSite { + url: "https://example.com".to_string(), + api_root: "https://example.com/wp-json".to_string(), + }; + + // Create site + let (db_site, _) = repo + .upsert_self_hosted_site(&test_conn, &site) + .expect("Failed to upsert site"); + + // Verify site exists in both tables + let count_sites = repo.count_all_sites(&test_conn).unwrap(); + let count_self_hosted = repo.count_all_self_hosted_sites(&test_conn).unwrap(); + assert_eq!(count_sites, 1); + assert_eq!(count_self_hosted, 1); + + // Delete site + let deleted = repo.delete_site(&test_conn, &db_site).unwrap(); + assert!(deleted, "Should return true when site is deleted"); + + // Verify site is removed from both tables + let count_sites_after = repo.count_all_sites(&test_conn).unwrap(); + let count_self_hosted_after = repo.count_all_self_hosted_sites(&test_conn).unwrap(); + assert_eq!( + count_sites_after, 0, + "Site should be deleted from sites table" + ); + assert_eq!( + count_self_hosted_after, 0, + "Site should be deleted from self_hosted_sites table" + ); + } + + #[rstest] + fn test_delete_site_returns_false_for_non_existent_site(test_conn: Connection) { + let repo = SiteRepository; + + let non_existent_site = DbSite { + row_id: RowId(999), + site_type: DbSiteType::SelfHosted, + mapped_site_id: RowId(888), + }; + + let deleted = repo.delete_site(&test_conn, &non_existent_site).unwrap(); + assert!(!deleted, "Should return false when site doesn't exist"); + } + + #[rstest] + fn test_delete_site_only_deletes_specified_site(test_conn: Connection) { + let repo = SiteRepository; + + // Create two sites + let site1 = SelfHostedSite { + url: "https://example1.com".to_string(), + api_root: "https://example1.com/wp-json".to_string(), + }; + let site2 = SelfHostedSite { + url: "https://example2.com".to_string(), + api_root: "https://example2.com/wp-json".to_string(), + }; + + let (db_site1, _) = repo.upsert_self_hosted_site(&test_conn, &site1).unwrap(); + let (db_site2, _) = repo.upsert_self_hosted_site(&test_conn, &site2).unwrap(); + + // Delete site1 + let deleted = repo.delete_site(&test_conn, &db_site1).unwrap(); + assert!(deleted); + + // Verify site1 is gone + let retrieved1 = repo + .select_self_hosted_site_by_url(&test_conn, &site1.url) + .unwrap(); + assert_eq!(retrieved1, None, "Site1 should be deleted"); + + // Verify site2 still exists + let retrieved2 = repo + .select_self_hosted_site_by_url(&test_conn, &site2.url) + .unwrap(); + assert!(retrieved2.is_some(), "Site2 should still exist"); + assert_eq!(retrieved2.unwrap().0.row_id, db_site2.row_id); + } + + #[rstest] + fn test_delete_self_hosted_site_by_url_deletes_site(test_conn: Connection) { + let repo = SiteRepository; + let url = "https://example.com"; + let site = SelfHostedSite { + url: url.to_string(), + api_root: "https://example.com/wp-json".to_string(), + }; + + // Create site + repo.upsert_self_hosted_site(&test_conn, &site) + .expect("Failed to upsert site"); + + // Verify site exists + let before_delete = repo + .select_self_hosted_site_by_url(&test_conn, url) + .unwrap(); + assert!(before_delete.is_some()); + + // Delete by URL + let deleted = repo + .delete_self_hosted_site_by_url(&test_conn, url) + .unwrap(); + assert!(deleted, "Should return true when site is deleted"); + + // Verify site is gone + let after_delete = repo + .select_self_hosted_site_by_url(&test_conn, url) + .unwrap(); + assert_eq!(after_delete, None, "Site should be deleted"); + } + + #[rstest] + fn test_delete_self_hosted_site_by_url_returns_false_for_non_existent(test_conn: Connection) { + let repo = SiteRepository; + + let deleted = repo + .delete_self_hosted_site_by_url(&test_conn, "https://non-existent.com") + .unwrap(); + assert!(!deleted, "Should return false when site doesn't exist"); + } } From 7132d4d5a24533460817a436c829b5eb6dffffad Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 23:00:00 -0400 Subject: [PATCH 16/20] Rename sites table to db_sites for consistent naming The `sites` table has been renamed to `db_sites` to match the pattern established by `DbSite` and other database types. This makes the naming more consistent and clear that it's a database-level polymorphic table. Changes: - Rename `sites` table to `db_sites` in migration 0001 - Update unique index name to `idx_db_sites_unique_site_type_and_mapped_site_id` - Rename `SITES_TABLE` constant to `DB_SITES_TABLE` - Update all foreign key references from `sites(id)` to `db_sites(id)` in: - posts_edit_context table - posts_view_context table - posts_embed_context table - term_relationships table - Update all comments to reference `db_sites table` instead of `sites table` - Rename `count_all_sites()` method to `count_all_db_sites()` - Update all test references to use new naming All tests pass (72 total). --- .../migrations/0001-create-sites-table.sql | 4 ++-- .../migrations/0002-create-posts-table.sql | 6 +++--- .../0003-create-term-relationships.sql | 4 ++-- .../0004-create-posts-view-context-table.sql | 6 +++--- .../0005-create-posts-embed-context-table.sql | 6 +++--- wp_mobile_cache/src/repository/sites.rs | 21 +++++++++++-------- 6 files changed, 25 insertions(+), 22 deletions(-) diff --git a/wp_mobile_cache/migrations/0001-create-sites-table.sql b/wp_mobile_cache/migrations/0001-create-sites-table.sql index 448cfb074..f4db11496 100644 --- a/wp_mobile_cache/migrations/0001-create-sites-table.sql +++ b/wp_mobile_cache/migrations/0001-create-sites-table.sql @@ -1,4 +1,4 @@ -CREATE TABLE `sites` ( +CREATE TABLE `db_sites` ( -- Internal DB field (auto-incrementing) `id` INTEGER PRIMARY KEY AUTOINCREMENT, @@ -11,4 +11,4 @@ CREATE TABLE `sites` ( ) STRICT; -- Unique constraint to prevent duplicate site mappings -CREATE UNIQUE INDEX idx_sites_unique_site_type_and_mapped_site_id ON sites(site_type, mapped_site_id); +CREATE UNIQUE INDEX idx_db_sites_unique_site_type_and_mapped_site_id ON db_sites(site_type, mapped_site_id); diff --git a/wp_mobile_cache/migrations/0002-create-posts-table.sql b/wp_mobile_cache/migrations/0002-create-posts-table.sql index 31c1cc53d..3d916eddb 100644 --- a/wp_mobile_cache/migrations/0002-create-posts-table.sql +++ b/wp_mobile_cache/migrations/0002-create-posts-table.sql @@ -2,8 +2,8 @@ CREATE TABLE `posts_edit_context` ( -- Internal DB field (auto-incrementing) `rowid` INTEGER PRIMARY KEY AUTOINCREMENT, - -- Site identifier (foreign key to sites table) - `db_site_id` INTEGER NOT NULL REFERENCES sites(id) ON DELETE CASCADE, + -- Site identifier (foreign key to db_sites table) + `db_site_id` INTEGER NOT NULL REFERENCES db_sites(id) ON DELETE CASCADE, -- Top-level non-nullable fields `id` INTEGER NOT NULL, @@ -57,7 +57,7 @@ CREATE TABLE `posts_edit_context` ( -- Client-side cache metadata: when this post was last fetched from the WordPress API `last_fetched_at` TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - FOREIGN KEY (db_site_id) REFERENCES sites(id) ON DELETE CASCADE + FOREIGN KEY (db_site_id) REFERENCES db_sites(id) ON DELETE CASCADE ) STRICT; CREATE UNIQUE INDEX idx_posts_edit_context_unique_db_site_id_and_id ON posts_edit_context(db_site_id, id); diff --git a/wp_mobile_cache/migrations/0003-create-term-relationships.sql b/wp_mobile_cache/migrations/0003-create-term-relationships.sql index 9c1c3f5a1..6eedb24c6 100644 --- a/wp_mobile_cache/migrations/0003-create-term-relationships.sql +++ b/wp_mobile_cache/migrations/0003-create-term-relationships.sql @@ -2,7 +2,7 @@ CREATE TABLE `term_relationships` ( -- Internal DB field (auto-incrementing) `rowid` INTEGER PRIMARY KEY AUTOINCREMENT, - -- Site identifier (foreign key to sites table) + -- Site identifier (foreign key to db_sites table) `db_site_id` INTEGER NOT NULL, -- Object identifier (rowid of post/page/nav_menu_item/etc) @@ -15,7 +15,7 @@ CREATE TABLE `term_relationships` ( -- Taxonomy type ('category', 'post_tag', or custom taxonomy) `taxonomy_type` TEXT NOT NULL, - FOREIGN KEY (db_site_id) REFERENCES sites(id) ON DELETE CASCADE + FOREIGN KEY (db_site_id) REFERENCES db_sites(id) ON DELETE CASCADE ) STRICT; -- Prevent duplicate associations (same object can't have same term twice in same taxonomy) diff --git a/wp_mobile_cache/migrations/0004-create-posts-view-context-table.sql b/wp_mobile_cache/migrations/0004-create-posts-view-context-table.sql index 3d47612e8..ce544b98b 100644 --- a/wp_mobile_cache/migrations/0004-create-posts-view-context-table.sql +++ b/wp_mobile_cache/migrations/0004-create-posts-view-context-table.sql @@ -2,8 +2,8 @@ CREATE TABLE `posts_view_context` ( -- Internal DB field (auto-incrementing) `rowid` INTEGER PRIMARY KEY AUTOINCREMENT, - -- Site identifier (foreign key to sites table) - `db_site_id` INTEGER NOT NULL REFERENCES sites(id) ON DELETE CASCADE, + -- Site identifier (foreign key to db_sites table) + `db_site_id` INTEGER NOT NULL REFERENCES db_sites(id) ON DELETE CASCADE, -- Top-level non-nullable fields `id` INTEGER NOT NULL, @@ -50,7 +50,7 @@ CREATE TABLE `posts_view_context` ( -- Client-side cache metadata: when this post was last fetched from the WordPress API `last_fetched_at` TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - FOREIGN KEY (db_site_id) REFERENCES sites(id) ON DELETE CASCADE + FOREIGN KEY (db_site_id) REFERENCES db_sites(id) ON DELETE CASCADE ) STRICT; CREATE UNIQUE INDEX idx_posts_view_context_unique_db_site_id_and_id ON posts_view_context(db_site_id, id); diff --git a/wp_mobile_cache/migrations/0005-create-posts-embed-context-table.sql b/wp_mobile_cache/migrations/0005-create-posts-embed-context-table.sql index e2c67b78e..5434b1729 100644 --- a/wp_mobile_cache/migrations/0005-create-posts-embed-context-table.sql +++ b/wp_mobile_cache/migrations/0005-create-posts-embed-context-table.sql @@ -2,8 +2,8 @@ CREATE TABLE `posts_embed_context` ( -- Internal DB field (auto-incrementing) `rowid` INTEGER PRIMARY KEY AUTOINCREMENT, - -- Site identifier (foreign key to sites table) - `db_site_id` INTEGER NOT NULL REFERENCES sites(id) ON DELETE CASCADE, + -- Site identifier (foreign key to db_sites table) + `db_site_id` INTEGER NOT NULL REFERENCES db_sites(id) ON DELETE CASCADE, -- Top-level non-nullable fields (minimal set for embed context) `id` INTEGER NOT NULL, @@ -29,7 +29,7 @@ CREATE TABLE `posts_embed_context` ( -- Client-side cache metadata: when this post was last fetched from the WordPress API `last_fetched_at` TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - FOREIGN KEY (db_site_id) REFERENCES sites(id) ON DELETE CASCADE + FOREIGN KEY (db_site_id) REFERENCES db_sites(id) ON DELETE CASCADE ) STRICT; CREATE UNIQUE INDEX idx_posts_embed_context_unique_db_site_id_and_id ON posts_embed_context(db_site_id, id); diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index 77a4e7a33..e9363bd57 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -13,7 +13,7 @@ pub struct SiteRepository; impl SiteRepository { const SELF_HOSTED_SITES_TABLE: &'static str = "self_hosted_sites"; - const SITES_TABLE: &'static str = "sites"; + const DB_SITES_TABLE: &'static str = "db_sites"; /// Upsert a self-hosted site and return both the DbSite and DbSelfHostedSite. /// @@ -44,7 +44,7 @@ impl SiteRepository { ON CONFLICT(site_type, mapped_site_id) DO UPDATE SET site_type = excluded.site_type RETURNING rowid", - Self::SITES_TABLE + Self::DB_SITES_TABLE ); let mut stmt = executor.prepare(&sql)?; @@ -132,7 +132,7 @@ impl SiteRepository { let sql = format!( "SELECT rowid, site_type, mapped_site_id FROM {} WHERE site_type = ? AND mapped_site_id = ?", - Self::SITES_TABLE + Self::DB_SITES_TABLE ); let mut stmt = executor.prepare(&sql)?; @@ -153,8 +153,11 @@ impl SiteRepository { /// Count all sites in the database. /// /// This is primarily useful for testing to verify database state. - pub fn count_all_sites(&self, executor: &impl QueryExecutor) -> Result { - let sql = format!("SELECT COUNT(*) FROM {}", Self::SITES_TABLE); + pub fn count_all_db_sites( + &self, + executor: &impl QueryExecutor, + ) -> Result { + let sql = format!("SELECT COUNT(*) FROM {}", Self::DB_SITES_TABLE); let mut stmt = executor.prepare(&sql)?; let count: i64 = stmt.query_row([], |row| row.get(0))?; Ok(count as usize) @@ -200,7 +203,7 @@ impl SiteRepository { }; // Delete from sites table - let sql = format!("DELETE FROM {} WHERE rowid = ?", Self::SITES_TABLE); + let sql = format!("DELETE FROM {} WHERE rowid = ?", Self::DB_SITES_TABLE); let sites_deleted = executor.execute(&sql, [site.row_id])?; Ok(sites_deleted > 0) @@ -335,7 +338,7 @@ mod tests { assert_eq!(db_site2.row_id, db_site3.row_id); // Verify only one entry exists in sites table (ensures bug fix works) - let count = repo.count_all_sites(&test_conn).unwrap(); + let count = repo.count_all_db_sites(&test_conn).unwrap(); assert_eq!( count, 1, "Multiple upserts should not create duplicate sites table entries" @@ -502,7 +505,7 @@ mod tests { .expect("Failed to upsert site"); // Verify site exists in both tables - let count_sites = repo.count_all_sites(&test_conn).unwrap(); + let count_sites = repo.count_all_db_sites(&test_conn).unwrap(); let count_self_hosted = repo.count_all_self_hosted_sites(&test_conn).unwrap(); assert_eq!(count_sites, 1); assert_eq!(count_self_hosted, 1); @@ -512,7 +515,7 @@ mod tests { assert!(deleted, "Should return true when site is deleted"); // Verify site is removed from both tables - let count_sites_after = repo.count_all_sites(&test_conn).unwrap(); + let count_sites_after = repo.count_all_db_sites(&test_conn).unwrap(); let count_self_hosted_after = repo.count_all_self_hosted_sites(&test_conn).unwrap(); assert_eq!( count_sites_after, 0, From db9f0d4215da20dccc0bfae607bac8877ecf1fce Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Thu, 30 Oct 2025 23:07:04 -0400 Subject: [PATCH 17/20] Simplify test assertions using struct equality Replace field-by-field comparisons with direct struct equality checks in site repository tests, leveraging the PartialEq implementations on DbSite and DbSelfHostedSite. Changes: - Use assert_eq! on structs instead of comparing individual fields - Reduces test verbosity while maintaining same validation coverage --- wp_mobile_cache/src/repository/sites.rs | 28 ++++--------------------- 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index e9363bd57..cda1b6cf6 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -363,9 +363,7 @@ mod tests { .expect("Failed to select site") .expect("Site should exist"); - assert_eq!(retrieved.row_id, original_db_self_hosted_site.row_id); - assert_eq!(retrieved.url, original_db_self_hosted_site.url); - assert_eq!(retrieved.api_root, original_db_self_hosted_site.api_root); + assert_eq!(retrieved, original_db_self_hosted_site); } #[rstest] @@ -424,27 +422,9 @@ mod tests { .expect("Failed to select site") .expect("Site should exist"); - // Verify DbSite matches - assert_eq!(retrieved_db_site.row_id, original_db_site.row_id); - assert_eq!(retrieved_db_site.site_type, original_db_site.site_type); - assert_eq!( - retrieved_db_site.mapped_site_id, - original_db_site.mapped_site_id - ); - - // Verify DbSelfHostedSite matches - assert_eq!( - retrieved_db_self_hosted_site.row_id, - original_db_self_hosted_site.row_id - ); - assert_eq!( - retrieved_db_self_hosted_site.url, - original_db_self_hosted_site.url - ); - assert_eq!( - retrieved_db_self_hosted_site.api_root, - original_db_self_hosted_site.api_root - ); + // Verify both structs match + assert_eq!(retrieved_db_site, original_db_site); + assert_eq!(retrieved_db_self_hosted_site, original_db_self_hosted_site); } #[rstest] From 844df6a7ed57d6a099bc84ca6cb405522c8a3aa6 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Sat, 1 Nov 2025 00:48:49 -0400 Subject: [PATCH 18/20] Replace unwrap() with expect() in sites.rs tests Standardize error handling across all tests in sites.rs by replacing unwrap() calls with descriptive expect() messages. This improves debugging by providing clear context when tests fail. Changes: - Replace unwrap() with expect() in test_conn fixture (2 calls) - Replace unwrap() with expect() in all test functions (21 calls) - Add descriptive error messages for database operations - Total: 23 unwrap() calls eliminated --- wp_mobile_cache/src/repository/sites.rs | 74 +++++++++++++++++-------- 1 file changed, 51 insertions(+), 23 deletions(-) diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index cda1b6cf6..e3be5d05f 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -236,8 +236,9 @@ mod tests { #[fixture] fn test_conn() -> Connection { - let conn = Connection::open_in_memory().unwrap(); - let mut migration_manager = MigrationManager::new(&conn).unwrap(); + let conn = Connection::open_in_memory().expect("Failed to open in-memory database"); + let mut migration_manager = + MigrationManager::new(&conn).expect("Failed to create MigrationManager"); migration_manager .perform_migrations() .expect("All migrations should succeed"); @@ -338,7 +339,9 @@ mod tests { assert_eq!(db_site2.row_id, db_site3.row_id); // Verify only one entry exists in sites table (ensures bug fix works) - let count = repo.count_all_db_sites(&test_conn).unwrap(); + let count = repo + .count_all_db_sites(&test_conn) + .expect("Failed to count db_sites"); assert_eq!( count, 1, "Multiple upserts should not create duplicate sites table entries" @@ -453,8 +456,12 @@ mod tests { api_root: "https://example2.com/wp-json".to_string(), }; - let (db_site1, _) = repo.upsert_self_hosted_site(&test_conn, &site1).unwrap(); - let (db_site2, _) = repo.upsert_self_hosted_site(&test_conn, &site2).unwrap(); + let (db_site1, _) = repo + .upsert_self_hosted_site(&test_conn, &site1) + .expect("Failed to upsert site1"); + let (db_site2, _) = repo + .upsert_self_hosted_site(&test_conn, &site2) + .expect("Failed to upsert site2"); // Verify different sites get different IDs assert_ne!(db_site1.row_id, db_site2.row_id); @@ -462,10 +469,10 @@ mod tests { // Verify both can be retrieved by URL let retrieved1 = repo .select_self_hosted_site_by_url(&test_conn, &site1.url) - .unwrap(); + .expect("Failed to select site by URL"); let retrieved2 = repo .select_self_hosted_site_by_url(&test_conn, &site2.url) - .unwrap(); + .expect("Failed to select site by URL"); assert!(retrieved1.is_some()); assert!(retrieved2.is_some()); @@ -485,18 +492,28 @@ mod tests { .expect("Failed to upsert site"); // Verify site exists in both tables - let count_sites = repo.count_all_db_sites(&test_conn).unwrap(); - let count_self_hosted = repo.count_all_self_hosted_sites(&test_conn).unwrap(); + let count_sites = repo + .count_all_db_sites(&test_conn) + .expect("Failed to count db_sites"); + let count_self_hosted = repo + .count_all_self_hosted_sites(&test_conn) + .expect("Failed to count self_hosted_sites"); assert_eq!(count_sites, 1); assert_eq!(count_self_hosted, 1); // Delete site - let deleted = repo.delete_site(&test_conn, &db_site).unwrap(); + let deleted = repo + .delete_site(&test_conn, &db_site) + .expect("Failed to delete site"); assert!(deleted, "Should return true when site is deleted"); // Verify site is removed from both tables - let count_sites_after = repo.count_all_db_sites(&test_conn).unwrap(); - let count_self_hosted_after = repo.count_all_self_hosted_sites(&test_conn).unwrap(); + let count_sites_after = repo + .count_all_db_sites(&test_conn) + .expect("Failed to count db_sites after delete"); + let count_self_hosted_after = repo + .count_all_self_hosted_sites(&test_conn) + .expect("Failed to count self_hosted_sites after delete"); assert_eq!( count_sites_after, 0, "Site should be deleted from sites table" @@ -517,7 +534,9 @@ mod tests { mapped_site_id: RowId(888), }; - let deleted = repo.delete_site(&test_conn, &non_existent_site).unwrap(); + let deleted = repo + .delete_site(&test_conn, &non_existent_site) + .expect("Failed to delete site"); assert!(!deleted, "Should return false when site doesn't exist"); } @@ -535,25 +554,34 @@ mod tests { api_root: "https://example2.com/wp-json".to_string(), }; - let (db_site1, _) = repo.upsert_self_hosted_site(&test_conn, &site1).unwrap(); - let (db_site2, _) = repo.upsert_self_hosted_site(&test_conn, &site2).unwrap(); + let (db_site1, _) = repo + .upsert_self_hosted_site(&test_conn, &site1) + .expect("Failed to upsert site1"); + let (db_site2, _) = repo + .upsert_self_hosted_site(&test_conn, &site2) + .expect("Failed to upsert site2"); // Delete site1 - let deleted = repo.delete_site(&test_conn, &db_site1).unwrap(); + let deleted = repo + .delete_site(&test_conn, &db_site1) + .expect("Failed to delete site1"); assert!(deleted); // Verify site1 is gone let retrieved1 = repo .select_self_hosted_site_by_url(&test_conn, &site1.url) - .unwrap(); + .expect("Failed to select site by URL"); assert_eq!(retrieved1, None, "Site1 should be deleted"); // Verify site2 still exists let retrieved2 = repo .select_self_hosted_site_by_url(&test_conn, &site2.url) - .unwrap(); + .expect("Failed to select site by URL"); assert!(retrieved2.is_some(), "Site2 should still exist"); - assert_eq!(retrieved2.unwrap().0.row_id, db_site2.row_id); + assert_eq!( + retrieved2.expect("Site2 should exist").0.row_id, + db_site2.row_id + ); } #[rstest] @@ -572,19 +600,19 @@ mod tests { // Verify site exists let before_delete = repo .select_self_hosted_site_by_url(&test_conn, url) - .unwrap(); + .expect("Failed to select site by URL"); assert!(before_delete.is_some()); // Delete by URL let deleted = repo .delete_self_hosted_site_by_url(&test_conn, url) - .unwrap(); + .expect("Failed to delete site by URL"); assert!(deleted, "Should return true when site is deleted"); // Verify site is gone let after_delete = repo .select_self_hosted_site_by_url(&test_conn, url) - .unwrap(); + .expect("Failed to select site by URL"); assert_eq!(after_delete, None, "Site should be deleted"); } @@ -594,7 +622,7 @@ mod tests { let deleted = repo .delete_self_hosted_site_by_url(&test_conn, "https://non-existent.com") - .unwrap(); + .expect("Failed to delete site by URL"); assert!(!deleted, "Should return false when site doesn't exist"); } } From e24d30db057ac38648c183d74549fba57921ec84 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Sat, 1 Nov 2025 01:04:15 -0400 Subject: [PATCH 19/20] Wrap SiteRepository operations in transactions for atomicity Ensure upsert and delete operations maintain data consistency between the polymorphic sites table and type-specific tables by wrapping them in atomic transactions. Changes: - Wrap `upsert_self_hosted_site()` in transaction to prevent orphaned entries - Wrap `delete_site()` in transaction to ensure proper cleanup of both tables - Scope SQL statements in blocks to ensure they drop before `tx.commit()` - Update method signatures to use `TransactionManager` instead of `QueryExecutor` - Update test helpers to accept `&mut Connection` for transactional operations - Add comment explaining `DO UPDATE SET` requirement for `RETURNING` clause --- .../src/repository/posts_multi_site_tests.rs | 10 +- wp_mobile_cache/src/repository/sites.rs | 143 ++++++++++-------- .../term_relationships_multi_site_tests.rs | 2 +- wp_mobile_cache/src/test_fixtures.rs | 14 +- 4 files changed, 91 insertions(+), 78 deletions(-) diff --git a/wp_mobile_cache/src/repository/posts_multi_site_tests.rs b/wp_mobile_cache/src/repository/posts_multi_site_tests.rs index c3699d470..468934fb8 100644 --- a/wp_mobile_cache/src/repository/posts_multi_site_tests.rs +++ b/wp_mobile_cache/src/repository/posts_multi_site_tests.rs @@ -6,7 +6,7 @@ use wp_api::posts::PostId; #[rstest] fn test_posts_in_site_1_invisible_to_site_2(mut test_ctx: TestContext) { - let site2 = create_random_test_site(&test_ctx.conn); + let site2 = create_random_test_site(&mut test_ctx.conn); // Insert post in site 1 let post = PostBuilder::minimal().with_id(100).build(); @@ -28,7 +28,7 @@ fn test_posts_in_site_1_invisible_to_site_2(mut test_ctx: TestContext) { #[rstest] fn test_same_post_id_can_exist_in_different_sites(mut test_ctx: TestContext) { - let site2 = create_random_test_site(&test_ctx.conn); + let site2 = create_random_test_site(&mut test_ctx.conn); // Create post with same ID in both sites let post_id = PostId(42); @@ -69,7 +69,7 @@ fn test_same_post_id_can_exist_in_different_sites(mut test_ctx: TestContext) { #[rstest] fn test_select_all_only_returns_posts_for_requested_site(mut test_ctx: TestContext) { - let site2 = create_random_test_site(&test_ctx.conn); + let site2 = create_random_test_site(&mut test_ctx.conn); // Insert posts in site 1 test_ctx @@ -119,7 +119,7 @@ fn test_select_all_only_returns_posts_for_requested_site(mut test_ctx: TestConte #[rstest] fn test_count_only_counts_posts_for_requested_site(mut test_ctx: TestContext) { - let site2 = create_random_test_site(&test_ctx.conn); + let site2 = create_random_test_site(&mut test_ctx.conn); // Insert posts in both sites test_ctx @@ -156,7 +156,7 @@ fn test_count_only_counts_posts_for_requested_site(mut test_ctx: TestContext) { #[rstest] fn test_delete_by_post_id_only_deletes_from_specified_site(mut test_ctx: TestContext) { - let site2 = create_random_test_site(&test_ctx.conn); + let site2 = create_random_test_site(&mut test_ctx.conn); let post_id = PostId(999); diff --git a/wp_mobile_cache/src/repository/sites.rs b/wp_mobile_cache/src/repository/sites.rs index e3be5d05f..2904202e7 100644 --- a/wp_mobile_cache/src/repository/sites.rs +++ b/wp_mobile_cache/src/repository/sites.rs @@ -5,7 +5,7 @@ use crate::{ row_ext::RowExt, self_hosted_site::{DbSelfHostedSite, DbSelfHostedSiteColumn, SelfHostedSite}, }, - repository::QueryExecutor, + repository::{QueryExecutor, TransactionManager}, }; use rusqlite::OptionalExtension; @@ -15,44 +15,49 @@ impl SiteRepository { const SELF_HOSTED_SITES_TABLE: &'static str = "self_hosted_sites"; const DB_SITES_TABLE: &'static str = "db_sites"; - /// Upsert a self-hosted site and return both the DbSite and DbSelfHostedSite. + /// Upsert a self-hosted site and return both the DbSite and DbSelfHostedSite (atomic transaction). /// /// If a site with the given URL already exists, updates it. Otherwise creates a new one. /// Uses SQLite's RETURNING clause to get the inserted/updated rowid. pub fn upsert_self_hosted_site( &self, - executor: &impl QueryExecutor, + transaction_manager: &mut impl TransactionManager, site: &SelfHostedSite, ) -> Result<(DbSite, DbSelfHostedSite), SqliteDbError> { - // Upsert into self_hosted_sites and get the rowid - let sql = format!( - "INSERT INTO {} (url, api_root) VALUES (?, ?) - ON CONFLICT(url) DO UPDATE SET api_root = excluded.api_root - RETURNING rowid", - Self::SELF_HOSTED_SITES_TABLE - ); + let tx = transaction_manager.transaction()?; - let mut stmt = executor.prepare(&sql)?; - let self_hosted_site_id: RowId = stmt - .query_row((&site.url, &site.api_root), |row| row.get(0)) - .map_err(SqliteDbError::from)?; + // Upsert into self_hosted_sites and get the rowid + let self_hosted_site_id: RowId = { + let sql = format!( + "INSERT INTO {} (url, api_root) VALUES (?, ?) + ON CONFLICT(url) DO UPDATE SET api_root = excluded.api_root + RETURNING rowid", + Self::SELF_HOSTED_SITES_TABLE + ); + + let mut stmt = tx.prepare(&sql)?; + stmt.query_row((&site.url, &site.api_root), |row| row.get(0)) + .map_err(SqliteDbError::from)? + }; // Upsert into sites table // If site_type + mapped_site_id already exists, reuse that entry - let sql = format!( - "INSERT INTO {} (site_type, mapped_site_id) VALUES (?, ?) - ON CONFLICT(site_type, mapped_site_id) DO UPDATE SET - site_type = excluded.site_type - RETURNING rowid", - Self::DB_SITES_TABLE - ); - - let mut stmt = executor.prepare(&sql)?; - let site_id: RowId = stmt - .query_row((DbSiteType::SelfHosted, self_hosted_site_id), |row| { + // Note: DO UPDATE SET is required for RETURNING to work on conflict (DO NOTHING returns no rows) + let site_id: RowId = { + let sql = format!( + "INSERT INTO {} (site_type, mapped_site_id) VALUES (?, ?) + ON CONFLICT(site_type, mapped_site_id) DO UPDATE SET + site_type = excluded.site_type + RETURNING rowid", + Self::DB_SITES_TABLE + ); + + let mut stmt = tx.prepare(&sql)?; + stmt.query_row((DbSiteType::SelfHosted, self_hosted_site_id), |row| { row.get(0) }) - .map_err(SqliteDbError::from)?; + .map_err(SqliteDbError::from)? + }; let db_site = DbSite { row_id: site_id, @@ -66,6 +71,7 @@ impl SiteRepository { api_root: site.api_root.clone(), }; + tx.commit().map_err(SqliteDbError::from)?; Ok((db_site, db_self_hosted_site)) } @@ -176,7 +182,7 @@ impl SiteRepository { Ok(count as usize) } - /// Delete a site and its type-specific data. + /// Delete a site and its type-specific data (atomic transaction). /// /// Deletes both the site entry and its corresponding type-specific entry /// (e.g., from self_hosted_sites or wordpress_com_sites). This ensures proper @@ -185,17 +191,19 @@ impl SiteRepository { /// Returns `true` if a site was deleted, `false` if the site didn't exist. pub fn delete_site( &self, - executor: &impl QueryExecutor, + transaction_manager: &mut impl TransactionManager, site: &DbSite, ) -> Result { + let tx = transaction_manager.transaction()?; + // Delete from type-specific table based on site_type - let _type_table_deleted = match site.site_type { + match site.site_type { DbSiteType::SelfHosted => { let sql = format!( "DELETE FROM {} WHERE rowid = ?", Self::SELF_HOSTED_SITES_TABLE ); - executor.execute(&sql, [site.mapped_site_id])? + tx.execute(&sql, [site.mapped_site_id])?; } DbSiteType::WordPressCom => { panic!("WordPress.com site deletion is not yet implemented") @@ -203,9 +211,12 @@ impl SiteRepository { }; // Delete from sites table - let sql = format!("DELETE FROM {} WHERE rowid = ?", Self::DB_SITES_TABLE); - let sites_deleted = executor.execute(&sql, [site.row_id])?; + let sites_deleted = { + let sql = format!("DELETE FROM {} WHERE rowid = ?", Self::DB_SITES_TABLE); + tx.execute(&sql, [site.row_id])? + }; + tx.commit().map_err(SqliteDbError::from)?; Ok(sites_deleted > 0) } @@ -214,13 +225,13 @@ impl SiteRepository { /// Returns `true` if a site was deleted, `false` if no site with that URL exists. pub fn delete_self_hosted_site_by_url( &self, - executor: &impl QueryExecutor, + transaction_manager: &mut impl TransactionManager, url: &str, ) -> Result { - let site_data = self.select_self_hosted_site_by_url(executor, url)?; + let site_data = self.select_self_hosted_site_by_url(transaction_manager, url)?; match site_data { - Some((db_site, _)) => self.delete_site(executor, &db_site), + Some((db_site, _)) => self.delete_site(transaction_manager, &db_site), None => Ok(false), } } @@ -263,7 +274,7 @@ mod tests { } #[rstest] - fn test_upsert_inserts_new_site(test_conn: Connection) { + fn test_upsert_inserts_new_site(mut test_conn: Connection) { let repo = SiteRepository; let site = SelfHostedSite { url: "https://example.com".to_string(), @@ -271,7 +282,7 @@ mod tests { }; let (db_site, db_self_hosted_site) = repo - .upsert_self_hosted_site(&test_conn, &site) + .upsert_self_hosted_site(&mut test_conn, &site) .expect("Failed to upsert site"); // Verify self_hosted_sites entry @@ -284,7 +295,7 @@ mod tests { } #[rstest] - fn test_upsert_updates_existing_site_url(test_conn: Connection) { + fn test_upsert_updates_existing_site_url(mut test_conn: Connection) { let repo = SiteRepository; let url = "https://example.com"; @@ -294,7 +305,7 @@ mod tests { api_root: "https://example.com/wp-json".to_string(), }; let (db_site1, db_self_hosted_site1) = repo - .upsert_self_hosted_site(&test_conn, &site1) + .upsert_self_hosted_site(&mut test_conn, &site1) .expect("Failed to upsert site"); // Second upsert with same URL but different api_root @@ -303,7 +314,7 @@ mod tests { api_root: "https://example.com/wordpress/wp-json".to_string(), }; let (db_site2, db_self_hosted_site2) = repo - .upsert_self_hosted_site(&test_conn, &site2) + .upsert_self_hosted_site(&mut test_conn, &site2) .expect("Failed to upsert site"); // Verify it updated the same row (same rowid) - this proves update, not insert @@ -315,7 +326,7 @@ mod tests { } #[rstest] - fn test_upsert_does_not_create_duplicate_sites_entries(test_conn: Connection) { + fn test_upsert_does_not_create_duplicate_sites_entries(mut test_conn: Connection) { let repo = SiteRepository; // Insert same site multiple times @@ -325,13 +336,13 @@ mod tests { }; let (db_site1, _) = repo - .upsert_self_hosted_site(&test_conn, &site) + .upsert_self_hosted_site(&mut test_conn, &site) .expect("First upsert failed"); let (db_site2, _) = repo - .upsert_self_hosted_site(&test_conn, &site) + .upsert_self_hosted_site(&mut test_conn, &site) .expect("Second upsert failed"); let (db_site3, _) = repo - .upsert_self_hosted_site(&test_conn, &site) + .upsert_self_hosted_site(&mut test_conn, &site) .expect("Third upsert failed"); // All should return the same DbSite @@ -349,7 +360,7 @@ mod tests { } #[rstest] - fn test_select_self_hosted_site_by_db_site(test_conn: Connection) { + fn test_select_self_hosted_site_by_db_site(mut test_conn: Connection) { let repo = SiteRepository; let site = SelfHostedSite { url: "https://example.com".to_string(), @@ -357,7 +368,7 @@ mod tests { }; let (db_site, original_db_self_hosted_site) = repo - .upsert_self_hosted_site(&test_conn, &site) + .upsert_self_hosted_site(&mut test_conn, &site) .expect("Failed to upsert site"); // Select using DbSite reference @@ -408,7 +419,7 @@ mod tests { } #[rstest] - fn test_select_self_hosted_site_by_url(test_conn: Connection) { + fn test_select_self_hosted_site_by_url(mut test_conn: Connection) { let repo = SiteRepository; let site = SelfHostedSite { url: "https://example.com".to_string(), @@ -416,7 +427,7 @@ mod tests { }; let (original_db_site, original_db_self_hosted_site) = repo - .upsert_self_hosted_site(&test_conn, &site) + .upsert_self_hosted_site(&mut test_conn, &site) .expect("Failed to upsert site"); // Select by URL @@ -444,7 +455,7 @@ mod tests { } #[rstest] - fn test_multiple_different_sites_can_coexist(test_conn: Connection) { + fn test_multiple_different_sites_can_coexist(mut test_conn: Connection) { let repo = SiteRepository; let site1 = SelfHostedSite { @@ -457,10 +468,10 @@ mod tests { }; let (db_site1, _) = repo - .upsert_self_hosted_site(&test_conn, &site1) + .upsert_self_hosted_site(&mut test_conn, &site1) .expect("Failed to upsert site1"); let (db_site2, _) = repo - .upsert_self_hosted_site(&test_conn, &site2) + .upsert_self_hosted_site(&mut test_conn, &site2) .expect("Failed to upsert site2"); // Verify different sites get different IDs @@ -479,7 +490,7 @@ mod tests { } #[rstest] - fn test_delete_site_removes_both_tables(test_conn: Connection) { + fn test_delete_site_removes_both_tables(mut test_conn: Connection) { let repo = SiteRepository; let site = SelfHostedSite { url: "https://example.com".to_string(), @@ -488,7 +499,7 @@ mod tests { // Create site let (db_site, _) = repo - .upsert_self_hosted_site(&test_conn, &site) + .upsert_self_hosted_site(&mut test_conn, &site) .expect("Failed to upsert site"); // Verify site exists in both tables @@ -503,7 +514,7 @@ mod tests { // Delete site let deleted = repo - .delete_site(&test_conn, &db_site) + .delete_site(&mut test_conn, &db_site) .expect("Failed to delete site"); assert!(deleted, "Should return true when site is deleted"); @@ -525,7 +536,7 @@ mod tests { } #[rstest] - fn test_delete_site_returns_false_for_non_existent_site(test_conn: Connection) { + fn test_delete_site_returns_false_for_non_existent_site(mut test_conn: Connection) { let repo = SiteRepository; let non_existent_site = DbSite { @@ -535,13 +546,13 @@ mod tests { }; let deleted = repo - .delete_site(&test_conn, &non_existent_site) + .delete_site(&mut test_conn, &non_existent_site) .expect("Failed to delete site"); assert!(!deleted, "Should return false when site doesn't exist"); } #[rstest] - fn test_delete_site_only_deletes_specified_site(test_conn: Connection) { + fn test_delete_site_only_deletes_specified_site(mut test_conn: Connection) { let repo = SiteRepository; // Create two sites @@ -555,15 +566,15 @@ mod tests { }; let (db_site1, _) = repo - .upsert_self_hosted_site(&test_conn, &site1) + .upsert_self_hosted_site(&mut test_conn, &site1) .expect("Failed to upsert site1"); let (db_site2, _) = repo - .upsert_self_hosted_site(&test_conn, &site2) + .upsert_self_hosted_site(&mut test_conn, &site2) .expect("Failed to upsert site2"); // Delete site1 let deleted = repo - .delete_site(&test_conn, &db_site1) + .delete_site(&mut test_conn, &db_site1) .expect("Failed to delete site1"); assert!(deleted); @@ -585,7 +596,7 @@ mod tests { } #[rstest] - fn test_delete_self_hosted_site_by_url_deletes_site(test_conn: Connection) { + fn test_delete_self_hosted_site_by_url_deletes_site(mut test_conn: Connection) { let repo = SiteRepository; let url = "https://example.com"; let site = SelfHostedSite { @@ -594,7 +605,7 @@ mod tests { }; // Create site - repo.upsert_self_hosted_site(&test_conn, &site) + repo.upsert_self_hosted_site(&mut test_conn, &site) .expect("Failed to upsert site"); // Verify site exists @@ -605,7 +616,7 @@ mod tests { // Delete by URL let deleted = repo - .delete_self_hosted_site_by_url(&test_conn, url) + .delete_self_hosted_site_by_url(&mut test_conn, url) .expect("Failed to delete site by URL"); assert!(deleted, "Should return true when site is deleted"); @@ -617,11 +628,13 @@ mod tests { } #[rstest] - fn test_delete_self_hosted_site_by_url_returns_false_for_non_existent(test_conn: Connection) { + fn test_delete_self_hosted_site_by_url_returns_false_for_non_existent( + mut test_conn: Connection, + ) { let repo = SiteRepository; let deleted = repo - .delete_self_hosted_site_by_url(&test_conn, "https://non-existent.com") + .delete_self_hosted_site_by_url(&mut test_conn, "https://non-existent.com") .expect("Failed to delete site by URL"); assert!(!deleted, "Should return false when site doesn't exist"); } diff --git a/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs b/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs index 61c78ecec..1108a5f82 100644 --- a/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs +++ b/wp_mobile_cache/src/repository/term_relationships_multi_site_tests.rs @@ -8,7 +8,7 @@ use wp_api::{taxonomies::TaxonomyType, terms::TermId}; #[rstest] fn test_term_relationships_isolated_by_site(mut test_ctx: TestContext) { - let site2 = create_random_test_site(&test_ctx.conn); + let site2 = create_random_test_site(&mut test_ctx.conn); // Insert post in site 1 with categories let post1 = PostBuilder::minimal() diff --git a/wp_mobile_cache/src/test_fixtures.rs b/wp_mobile_cache/src/test_fixtures.rs index 747cbb66b..53f7d4c67 100644 --- a/wp_mobile_cache/src/test_fixtures.rs +++ b/wp_mobile_cache/src/test_fixtures.rs @@ -23,7 +23,7 @@ pub mod posts; /// /// ```rust /// #[rstest] -/// fn test_something(test_ctx: TestContext) { +/// fn test_something(mut test_ctx: TestContext) { /// let post = PostBuilder::new().build(); /// test_ctx.post_repo.upsert(&mut test_ctx.conn, &test_ctx.site, &post).unwrap(); /// } @@ -47,7 +47,7 @@ pub fn test_ctx() -> TestContext { } fn test_db() -> (Connection, DbSite) { - let conn = Connection::open_in_memory().unwrap(); + let mut conn = Connection::open_in_memory().unwrap(); let mut migration_manager = MigrationManager::new(&conn).unwrap(); migration_manager @@ -61,7 +61,7 @@ fn test_db() -> (Connection, DbSite) { api_root: format!("{}/wp-json", test_creds.site_url), }; - let db_site = create_test_site(&conn, &self_hosted_site); + let db_site = create_test_site(&mut conn, &self_hosted_site); (conn, db_site) } @@ -69,7 +69,7 @@ fn test_db() -> (Connection, DbSite) { /// Helper to create a test site. /// /// Uses `SiteRepository` to insert the site into the database. -pub fn create_test_site(conn: &Connection, site: &SelfHostedSite) -> DbSite { +pub fn create_test_site(conn: &mut Connection, site: &SelfHostedSite) -> DbSite { let site_repo = SiteRepository; let (db_site, _) = site_repo .upsert_self_hosted_site(conn, site) @@ -88,11 +88,11 @@ static RANDOM_TEST_SITE_COUNTER: AtomicU32 = AtomicU32::new(1); /// # Example /// /// ```rust -/// let site1 = create_random_test_site(&conn); -/// let site2 = create_random_test_site(&conn); +/// let site1 = create_random_test_site(&mut conn); +/// let site2 = create_random_test_site(&mut conn); /// // site1 and site2 will have different URLs /// ``` -pub fn create_random_test_site(conn: &Connection) -> DbSite { +pub fn create_random_test_site(conn: &mut Connection) -> DbSite { let counter = RANDOM_TEST_SITE_COUNTER.fetch_add(1, Ordering::Relaxed); let site = SelfHostedSite { url: format!("https://test-site-{}.local", counter), From 217940724ce66828bc0ca15dbc05691a05faab51 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Sat, 1 Nov 2025 23:14:16 -0400 Subject: [PATCH 20/20] Update expected migration counts in Kotlin & Swift tests --- .../kotlin/src/integrationTest/kotlin/WordPressApiCacheTest.kt | 2 +- .../Tests/wordpress-api-cache/WordPressApiCacheTests.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/native/kotlin/api/kotlin/src/integrationTest/kotlin/WordPressApiCacheTest.kt b/native/kotlin/api/kotlin/src/integrationTest/kotlin/WordPressApiCacheTest.kt index bc88e842e..7ccfe48a0 100644 --- a/native/kotlin/api/kotlin/src/integrationTest/kotlin/WordPressApiCacheTest.kt +++ b/native/kotlin/api/kotlin/src/integrationTest/kotlin/WordPressApiCacheTest.kt @@ -11,7 +11,7 @@ class WordPressApiCacheTest { @Test fun testThatMigrationsWork() = runTest { - assertEquals(5, WordPressApiCache().performMigrations()) + assertEquals(6, WordPressApiCache().performMigrations()) } @Test diff --git a/native/swift/Tests/wordpress-api-cache/WordPressApiCacheTests.swift b/native/swift/Tests/wordpress-api-cache/WordPressApiCacheTests.swift index 6b471f1c4..706635d37 100644 --- a/native/swift/Tests/wordpress-api-cache/WordPressApiCacheTests.swift +++ b/native/swift/Tests/wordpress-api-cache/WordPressApiCacheTests.swift @@ -13,7 +13,7 @@ actor Test { @Test func testMigrationsWork() async throws { let migrationsPerformed = try await self.cache.performMigrations() - #expect(migrationsPerformed == 5) + #expect(migrationsPerformed == 6) } #if !os(Linux)