WIP: Caching support #976
Draft
WIP: Caching support #976
+3,355
−48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implements bidirectional mapping between `AnyPostWithEditContext` and SQLite for the mobile cache layer. Key features: - Helper functions for common conversions (ID types, enums, JSON, dates) - Type-safe column access via `ColumnIndex` trait and `RowExt` extension - Inline struct construction minimizes intermediate variables - Type inference using `From` trait reduces boilerplate Changes: - Add `mappings` module with `TryFromDbRow` and `InsertIntoDb` traits - Add `helpers` module with reusable conversion functions - Implement post mappings in `mappings/posts.rs` - Update schema to match REST API field names - Add `serde` and `serde_json` dependencies
Implements comprehensive round-trip testing to validate that `AnyPostWithEditContext` can be correctly serialized to and deserialized from the SQLite database. Changes: - Add test fixtures for creating minimal and fully-populated posts - Add `setup_test_db()` helper that applies all migrations - Add round-trip tests covering all field types and edge cases - Test optional field handling, enum variants, and empty collections
Implements a flexible repository pattern to reduce boilerplate for database CRUD operations while allowing type-specific customization. Changes: - Add `QueryExecutor` trait to abstract database query execution - Add `DbEntity` trait for types that can be persisted to database - Add `Repository` trait with default implementations for `insert` and `insert_batch` - Implement `PostRepository` with custom query methods (`select_by_post_id`, `select_by_author`, `select_by_status`, `delete_by_post_id`, `update_by_post_id`, `count`) - Update existing unit tests to use `PostRepository` instead of raw SQL queries - Add comprehensive tests for all repository methods
Implements SQLite's INSERT ... ON CONFLICT ... DO UPDATE syntax to handle both insert and update operations in a single SQL statement. This ensures database observers receive a single INSERT or UPDATE action, not DELETE + INSERT. Changes: - Add `upsert()` method that inserts or updates posts by `post_id` - Uses the UNIQUE INDEX on `id` column to detect conflicts - Writes SQL field list only once (shared between INSERT and UPDATE) - Preserves rowid when updating (same row, not delete+insert) - Add comprehensive tests for both insert and update scenarios
Update the post database schema and repository to support multi-site functionality by adding a site_id field. The combination of (site_id, id) is now unique instead of just id, allowing posts from different sites to have the same WordPress post ID without conflicts. Changes: - Add SiteId newtype wrapper in lib.rs - Update database migration to include site_id column - Change unique index from id to (site_id, id) - Update DbAnyPostWithEditContext to include site_id field - Modify InsertIntoDb trait to require site_id parameter - Update all Repository methods to accept site_id - Update PostRepository query methods to filter by site_id - Update upsert to use ON CONFLICT(site_id, id) - Update all tests to pass site_id parameter
Reorganize migrations to add a sites table as the foundation for multi-site support. Posts now reference the sites table via a foreign key constraint. Changes: - Create 0001-create-sites-table.sql with scaffold sites table - Rename posts migration from 0001 to 0002 - Delete users migration (not needed yet) - Add foreign key constraint from posts.site_id to sites.id - Add index on posts.site_id for query performance - Update MIGRATION_QUERIES array to reference new migrations - Update setup_test_db to insert test site for FK constraint
Document the repository pattern implementation with comprehensive coverage of design decisions, multi-site architecture, and usage examples. The documentation includes: - Core components (QueryExecutor, DbEntity, Repository traits) - Design decisions and rationale - Multi-site architecture with foreign key constraints - UPSERT implementation using composite keys - Complete usage examples with site_id scoping - File organization and migration structure
Replace i64 row IDs with strongly-typed RowId (u64) and refactor SiteId to DbSite to prevent confusion with WordPress.com site IDs. SQLite autoincrement guarantees non-negative values, making u64 appropriate. The DbSite type is explicitly database-internal (Db prefix) and prevents mixing up cache database row IDs with WordPress domain identifiers. Repository methods now require &DbSite, forcing callers to fetch valid sites before querying, which prevents invalid ID construction. Changes: - Add `RowId` type wrapping u64 with `ToSql`/`FromSql` traits for seamless rusqlite integration - Replace `SiteId` with `DbSite` struct containing `row_id: RowId` field - Update all repository methods to take `&DbSite` instead of `SiteId` - Update `DbAnyPostWithEditContext` to use `RowId` and `DbSite` types - Add comprehensive documentation explaining DbSite design rationale - Update REPOSITORY_PATTERN_DESIGN.md with multi-site architecture details
…saction safety This commit refactors the database abstraction layer to prevent nested transactions at compile time and ensure consistent use of the QueryExecutor abstraction throughout the codebase. Key changes: **Trait Separation:** - Split `QueryExecutor` into two traits: `QueryExecutor` and `TransactionManager` - `QueryExecutor` provides core query operations: `prepare()`, `execute()`, `last_insert_rowid()` - `TransactionManager` extends `QueryExecutor` and adds `transaction()` method - `Connection` implements both traits (can execute queries and create transactions) - `Transaction` implements only `QueryExecutor` (prevents nested transactions at compile time) **Type Safety Improvements:** - Updated `InsertIntoDb` trait to accept `&impl QueryExecutor` instead of `&Connection` - Changed return type from `i64` to `RowId` for consistency with type wrappers - Repository methods now use `&impl QueryExecutor` for queries - `insert_batch()` requires `&mut impl TransactionManager` making transaction requirement explicit **Documentation Updates:** - Added "Decision 8: Split QueryExecutor and TransactionManager Traits" section - Updated all trait signatures to reflect QueryExecutor/TransactionManager split - Updated all code examples to use `RowId` instead of `i64` - Updated method signatures in concrete repository examples - Clarified that Transaction implements QueryExecutor (compile-time nested transaction prevention) **Benefits:** - Compile-time enforcement prevents nested transactions (no runtime panics) - Clear separation of concerns between query execution and transaction management - Repository method signatures explicitly show when transactions are needed - Consistent abstraction layer throughout codebase (no direct Connection usage) - Type-safe return values using `RowId` wrapper instead of raw `i64`
Rename the site_id column to db_site_id in the posts table and all related queries to maintain consistency with database-internal naming conventions. Changes: - Updated migration: site_id → db_site_id column and index names - Updated all SQL queries in PostRepository - Updated INSERT query in InsertIntoDb implementation - Updated documentation examples in REPOSITORY_PATTERN_DESIGN.md
Add comprehensive documentation for Decision 9: Term Relationships Normalization, including database schema, type design, repository patterns, and usage examples. Also add a prompt file for a future session to reorganize the growing documentation into a more maintainable structure with separate files in a docs/ folder. Changes: - Add Decision 9: Term Relationships Normalization to design doc - Document sync-based approach for observer compatibility - Add upsert_with_terms usage examples - Clarify referential integrity (FK on db_site_id, not object_id) - Update file organization to include term_relationships files - Add REORGANIZE_DOCS_PROMPT.md for future documentation refactoring
Store post categories and tags in a normalized `term_relationships` table instead of JSON arrays to enable efficient querying and improve data integrity. This implementation uses a sync-based approach that only generates database events for actual changes (inserts/deletes), making it observer-friendly. The design is extensible to support any taxonomy type and object type. Changes: - Add migration 0003 creating term_relationships table with indexes - Create DbTermRelationship type with TaxonomyType and TermId from wp_api - Implement TermRelationshipRepository with sync_terms_for_object method - Update PostRepository to populate terms from term_relationships on read - Add upsert_with_terms for atomic post and term operations - Update delete_by_post_id to cascade term relationship cleanup - Fix upsert to query rowid after UPDATE (last_insert_rowid doesn't change) - Add 13 comprehensive tests for term relationship operations
Add `last_fetched_at` timestamp to cached posts for tracking data staleness. This enables cache refresh logic and displaying "Last updated X ago" in offline UX scenarios like pull-to-refresh. SQLite automatically manages timestamps using DEFAULT on INSERT and explicit setting on UPDATE, requiring zero boilerplate from developers. Changes: - Add `last_fetched_at` column to posts_edit_context table with UTC timestamp default - Add `last_fetched_at` field to DbAnyPostWithEditContext type - Update upsert SQL to set timestamp on UPDATE via strftime - Add column enum and mapping for last_fetched_at in posts mappings - Add helper function assert_recent_timestamp for timestamp validation in tests - Add test_insert_sets_last_fetched_at to verify INSERT behavior - Add test_upsert_updates_last_fetched_at_on_update to verify UPDATE behavior - Document Decision 10 in REPOSITORY_PATTERN_DESIGN.md with rationale and examples
Split monolithic REPOSITORY_PATTERN_DESIGN.md into well-organized documentation structure for better maintainability and navigation. The new structure provides: - Clear navigation via docs/README.md - Focused architecture documents - Individual design decision files (10 decisions) - Complete repository API documentation - Practical usage examples and migration guide Changes: - Create docs/ directory structure with architecture/, design-decisions/, and repositories/ subdirectories - Create docs/README.md with comprehensive navigation and quick start - Create docs/architecture/core-traits.md for QueryExecutor, TransactionManager, Repository traits - Create docs/architecture/database-schema.md for complete schema documentation - Create docs/architecture/type-system.md for RowId, DbSite, and wrapper types - Create 10 focused design decision documents (01-executor-passing.md through 10-cache-freshness.md) - Create docs/repositories/post-repository.md with full PostRepository API - Create docs/repositories/term-relationship-repository.md with TermRelationshipRepository API - Create docs/usage-examples.md with comprehensive code examples - Create docs/migration-guide.md for adding new entities - Remove REPOSITORY_PATTERN_DESIGN.md (content fully migrated) - Remove REORGANIZE_DOCS_PROMPT.md (task completed)
Implement transaction rollback, constraint, and multi-site isolation tests. This commit adds 19 new tests across four test modules to verify critical database behavior and data integrity guarantees. The test suite now includes comprehensive coverage of transaction safety, constraint enforcement, and multi-site isolation. Changes: - Add rstest framework for fixture-based testing - Create `PostBuilder` with thread-safe auto-incrementing IDs - Add rstest fixtures (`test_db`, `test_site`) to reduce boilerplate - Implement transaction rollback tests (4 tests) - Verify constraint violation rollback - Verify foreign key violation rollback - Verify term relationship consistency - Verify happy path batch inserts - Implement constraint enforcement tests (7 tests) - Duplicate post ID detection - Foreign key constraint validation - Error handling for non-existent entities - Implement multi-site isolation tests (8 tests total) - Verify posts isolated between sites - Verify same post ID allowed in different sites - Verify operations respect site boundaries - Verify term relationships isolated by site
Standardize all test infrastructure to use rstest fixtures for consistency
and better test ergonomics. The previous setup had two separate approaches:
`setup_test_db()` function in `unit_test_common.rs` and rstest fixtures in
`test_helpers.rs`. This consolidation eliminates the redundancy.
Changes:
- Convert all tests in `repository/posts.rs` to use rstest fixtures (17 tests)
- Convert all tests in `repository/term_relationships.rs` to rstest fixtures (7 tests)
- Convert all tests in `mappings/posts.rs` to rstest fixtures (6 tests)
- Replace `#[test]` with `#[rstest]` throughout test suite
- Update imports from `unit_test_common::setup_test_db` to `test_helpers::{test_db, test_site}`
- Replace function parameters from manual setup to rstest fixtures
- Delete `unit_test_common.rs` module
- Remove `unit_test_common` from `lib.rs` module declarations
Simplified the post repository API by consolidating three separate insertion code paths into a single atomic `upsert()` method that always handles term relationships correctly. Changes: - Remove JSON `categories` and `tags` columns from posts table migration - Update column indices in post mappings after removing JSON columns - Remove `insert()` and `insert_batch()` from Repository trait - Integrate term relationships sync directly into `upsert()` method - Delete redundant `upsert_with_terms()` method - Add `upsert_batch()` that calls `upsert()` for each post - Update all tests to use consolidated `upsert()` method - Fix test expectations for UPSERT behavior (updates instead of errors) The new `upsert()` method: - Uses SQLite's INSERT...ON CONFLICT...DO UPDATE for atomic operations - Syncs term relationships in the same transaction - Ensures database observers see single INSERT/UPDATE events - Makes it impossible to accidentally insert posts without handling terms All 50 tests passing.
The InsertIntoDb trait was a leaky abstraction that couldn't handle cross-repository concerns. Posts require term relationships to be synced via TermRelationshipRepository, making it impossible to implement proper insertion through a trait on the entity type itself. The only correct way to insert/update posts is through PostRepository::upsert(), which coordinates with the term repository in a single transaction. Changes: - Remove InsertIntoDb trait definition from mappings module - Remove InsertIntoDb implementation for AnyPostWithEditContext - Remove InsertIntoDb trait bound from DbEntity - Remove unused helper imports (bool_to_integer, serialize_value_to_json) - Clean up test code that only existed to satisfy the trait bound This simplifies the codebase by removing ~95 lines of dead code.
The Repository trait was premature abstraction. PostRepository is currently the only repository, and future repositories (users, media, comments) will likely have different requirements: - Posts need term relationship coordination - Other entities won't have categories/tags - Different entities will need different query methods Rather than forcing a common interface that may not fit all use cases, each repository will define its own API based on its specific needs. The trait can be reintroduced later if a clear pattern emerges across multiple repositories. Changes: - Remove Repository trait definition and documentation - Remove impl Repository for PostRepository - Remove TestRepository test scaffolding - Update PostRepository doc comment The DbEntity marker trait remains as it's genuinely generic (just TABLE_NAME constant).
Remove outdated references to removed traits and methods: - Remove all references to InsertIntoDb trait (removed in recent commits) - Remove all references to Repository trait (removed as premature abstraction) - Update insert() and insert_batch() references to upsert() and upsert_batch() - Update upsert_with_terms() references to upsert() (terms now handled automatically) - Update method signatures to reflect new TransactionManager requirement Updated files: - docs/architecture/core-traits.md: Removed Repository trait section - docs/repositories/post-repository.md: Updated all method signatures and examples - docs/repositories/term-relationship-repository.md: Removed Repository trait note - docs/usage-examples.md: Updated all code examples to use upsert() - docs/migration-guide.md: Removed InsertIntoDb implementation steps - docs/design-decisions/*.md: Updated all code examples and added historical note All examples now accurately reflect the current implementation where: - PostRepository::upsert() handles term synchronization automatically - Each repository defines its own API (no shared Repository trait) - All operations use TransactionManager for atomic commits
Implement infallible string conversions for TaxonomyType in wp_api, leveraging the Custom(String) variant as a fallback. This provides a cleaner, more reusable API than serde_json for simple string parsing. Implementation uses delegation pattern to avoid code duplication: - From<&str> contains the actual conversion logic - From<String> delegates to From<&str> via s.as_str() Changes: - Add From<&str> for TaxonomyType in wp_api/src/taxonomies.rs - Add From<String> for TaxonomyType (delegates to From<&str>) - Update term_relationships mapping to use .into() instead of serde_json Benefits: - Cleaner code: row.get_column(...)?.into() vs serde_json boilerplate - Reusable: From implementations available everywhere, not just in mapping layer - Infallible: No error handling needed thanks to Custom variant fallback - No code duplication: From<String> delegates to From<&str> - No unnecessary allocations from serde_json path All existing tests pass.
Clean up test infrastructure by removing unused fixtures and duplicate test coverage. Changes: - Remove unused `second_site` fixture from test_helpers - Remove redundant `test_site_isolation_basic_verification` test - Remove emoji from comment in term_relationships.rs
Consolidate test fixture usage by introducing a comprehensive `TestContext` that bundles all common test dependencies (database connection, site, and repositories). This reduces boilerplate, improves maintainability, and prepares the codebase for future service layer patterns. Changes: - Add `term_repo` fixture and `TermRelationshipRepository` to `TestContext` - Rename `TestContext.repo` to `post_repo` for clarity - Update all test functions to use `TestContext` fixture instead of manually instantiating repositories - Update 49 test functions across 6 test files (posts.rs, posts_constraint_tests.rs, posts_multi_site_tests.rs, posts_transaction_tests.rs, term_relationships.rs, term_relationships_multi_site_tests.rs) - Remove repetitive `let repo = Repository;` lines from all tests
Finish migrating remaining tests to use `TestContext` and streamline the fixture implementation by removing intermediate fixtures. Changes: - Update 7 mapping tests in `posts.rs` to use `TestContext` - Remove unused individual fixtures (`test_site`, `post_repo`, `term_repo`) - Simplify `test_ctx` fixture to construct all dependencies inline - Remove unused imports from mapping tests
Consolidate test infrastructure by renaming the test helpers module to match the existing test_fixtures module naming convention, providing a clearer and more consistent structure. Changes: - Rename test_helpers.rs to test_fixtures.rs - Update module declaration in lib.rs - Update all test imports to use test_fixtures instead of test_helpers - Remove test_fixtures/mod.rs directory
Remove select_by_author and select_by_status methods along with their tests to avoid premature commitment to APIs that may not be needed. These can be added back when actual use cases emerge, preventing unnecessary maintenance burden during early development. Changes: - Remove select_by_author method and test - Remove select_by_status method and test - Remove unused UserId import from tests
Remove test_round_trip_with_optional_fields_none and test_round_trip_with_sticky_boolean_variants as they duplicate coverage already provided by existing tests. The first test explicitly sets fields to None that are already None in create_minimal_post(). The second test exhaustively checks all Option<bool> variants when standard SQLite boolean handling is already validated by existing tests. Changes: - Remove test_round_trip_with_optional_fields_none test - Remove test_round_trip_with_sticky_boolean_variants test
Replace the DbEntity trait with const TABLE_NAME in each repository. This is simpler and achieves the same goal without adding ceremony that doesn't provide compile-time guarantees. Changes: - Remove DbEntity trait from repository/mod.rs - Remove DbEntity implementation from AnyPostWithEditContext - Add const TABLE_NAME to PostRepository and TermRelationshipRepository - Replace all hardcoded table names with Self::TABLE_NAME in SQL queries
oguzkocer
force-pushed
the
wp_mobile_cache-mapping
branch
from
4dc604f to
fe6c03e
Compare
Change sync_terms_for_object to require a Transaction parameter instead of generic QueryExecutor to guarantee atomicity at compile-time. This prevents accidental usage without a transaction which could lead to inconsistent state if any step fails. The method performs multiple operations (SELECT, DELETE, INSERT) that must be atomic, so requiring Transaction prevents bugs where the method is called with a bare Connection. Changes: - Change sync_terms_for_object parameter from &impl QueryExecutor to &rusqlite::Transaction - Update all test calls to create and commit transactions
Refactor post entity construction to guarantee terms are always loaded from the database at compile time. This prevents bugs where terms might be accidentally omitted, and adds efficient batch loading to avoid N+1 query problems. Changes: - Add `PostTerms` struct to encapsulate categories and tags - Add `get_post_terms()` for single post term loading - Add `get_post_terms_batch()` for efficient batch loading of multiple posts - Change `DbAnyPostWithEditContext` to use `from_row_with_terms(row, terms)` requiring `PostTerms` parameter - Update all `PostRepository` select methods to load terms before constructing posts - Remove `TryFromDbRow` trait (no longer needed) - Remove `populate_terms()` mutation method (replaced by explicit loading) - Mark `mappings/term_relationships.rs` as unused
Replace domain-specific `PostTerms` with generic `DbTermRelationship` entities to improve separation of concerns and repository reusability. Changes: - Add `RowId::to_sql_list()` helper to create comma-separated ID lists for SQL IN clauses - Restore `DbTermRelationship::from_row()` mapping in `mappings/term_relationships.rs` - Replace `get_post_terms()` and `get_post_terms_batch()` with generic `get_terms_for_objects()` - Repository layer now returns raw `DbTermRelationship` entities - Move domain logic (extracting categories/tags) to mapping layer in `from_row_with_terms()` - Use functional programming (fold) for cleaner term extraction - Update `PostRepository` to use `get_terms_for_objects()` - Interpolate object_ids into SQL IN clause (safe since they're internal IDs) - Keep db_site_id as parameterized query for consistency - Repository is now reusable for other object types (pages, nav items, etc.)
…rowids The `term_relationships` table was incorrectly storing SQLite rowids as `object_id` values instead of WordPress object IDs (e.g., post.id). This caused term relationships to be lost when querying by WordPress ID. All term relationship operations now consistently use WordPress object IDs throughout the codebase, ensuring terms are properly associated with their WordPress objects regardless of how the object is queried. Changes: - Update `TermRelationshipRepository` methods to use `i64` for `object_id` instead of `RowId` - Fix `PostRepository.upsert()` to sync terms using `post.id.0` instead of `post_rowid` - Fix `PostRepository` select methods to query WordPress post IDs for term loading - Fix `PostRepository.delete_by_post_id()` to delete terms using WordPress post ID - Update all tests to use WordPress post IDs instead of SQLite rowids - Remove unused `RowId` import from `term_relationships.rs`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is currently work in progress. I've worked on this with Claude and although the general design is in place and I've reviewed some of the code and polished it, there are some parts I'd like to go over. I'm going to use this PR to review it myself in an organized way and update the description once it's ready for others to review.