From 19e1ae587f76f45855439405ea9103525c03a4ba Mon Sep 17 00:00:00 2001 From: Andrew Phillips Date: Sun, 24 Aug 2025 23:36:55 -0300 Subject: [PATCH] docs: update refactoring plan with thread safety, error handling, and layer boundaries Co-authored-by: aider (openai/andrew/openrouter/mistralai/mistral-medium-3.1) --- PLAN.md | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 75 insertions(+), 9 deletions(-) diff --git a/PLAN.md b/PLAN.md index 64c6b0d..518b5e9 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1,6 +1,6 @@ # Refactoring Plan to Reduce Code Duplication -## 1. Create Core Service Layer +## 1. Create Core Service Layer with Clear Boundaries **Files:** `src/core/` (new directory) - `src/core/item_service.rs` - Main service for item operations (synchronous) - `src/core/async_item_service.rs` - Async wrapper for API use @@ -18,7 +18,45 @@ - Provide async wrappers using `tokio::task::spawn_blocking` for API use - Use streaming for pipeline efficiency (process data in chunks) -## 2. Create Common Data Structures +**Layer Division:** +- **`db.rs`**: Low-level SQL operations (e.g., `insert_item`, `query_all_items`) +- **`item_service.rs`**: Higher-level business logic (e.g., "get item with metadata and tags," "validate item before save") +- Example: + ```rust + // db.rs (low-level) + pub fn get_item_with_tags(conn: &Connection, id: i64) -> Result<(Item, Vec)> + + // item_service.rs (high-level) + pub fn get_item_full(conn: &Connection, id: i64) -> Result { + let (item, tags) = db::get_item_with_tags(conn, id)?; + let meta = db::get_item_meta(conn, &item)?; + Ok(ItemWithMeta { item, tags, meta }) + } + ``` + +## 2. Thread Safety and Resource Management +**Reason:** Ensure services can be safely used in both synchronous and asynchronous contexts +**Implementation:** +- Document thread-safety guarantees for all core services +- Use `Arc>` or connection pooling for shared resources like database connections +- Provide examples for safe async/sync boundaries: + ```rust + pub async fn get_item_async( + state: &AppState, + id: i64, + ) -> Result { + let conn = state.db.clone(); // Arc> + tokio::task::spawn_blocking(move || { + let conn = conn.lock().unwrap(); + item_service::get_item_full(&conn, id) + }) + .await? + } + ``` +- Use `tokio::task::spawn_blocking` for CPU-bound or blocking I/O operations +- Benchmark thread pool sizes for CPU-bound tasks (e.g., compression) + +## 3. Create Common Data Structures **Files:** `src/core/types.rs` **Reason:** Standardize data structures used across modes and APIs **Implementation:** @@ -28,7 +66,35 @@ - Ensure all fields are properly documented - Use zero-copy patterns where possible (slicing instead of copying) -## 3. Refactor CLI Modes to Use Services +## 4. Unified Error Handling with Conversions +**Reason:** Standardize error types across CLI, API, and MCP interfaces +**Implementation:** +- Define a base error enum (`CoreError`) with conversions to all interface-specific error types +- Example: + ```rust + #[derive(Debug, thiserror::Error)] + pub enum CoreError { + #[error("DB error: {0}")] + Database(#[from] rusqlite::Error), + #[error("IO error: {0}")] + Io(#[from] std::io::Error), + // ... + } + + // Auto-convert to HTTP status + impl From for StatusCode { + fn from(err: CoreError) -> Self { + match err { + CoreError::Database(_) => StatusCode::INTERNAL_SERVER_ERROR, + // ... + } + } + } + ``` +- Use `#[derive(thiserror::Error)]` for easy `Display` and `Error` implementations +- Provide user-friendly error messages with error codes for programmatic handling + +## 5. Refactor CLI Modes to Use Services **Files:** - `src/modes/get.rs` - `src/modes/save.rs` @@ -46,7 +112,7 @@ - Use synchronous services directly - Implement streaming for stdin/stdout to maintain pipeline performance -## 4. Refactor REST API to Use Async Services +## 6. Refactor REST API to Use Async Services **Files:** - `src/modes/server/api/item.rs` - `src/modes/server/api/status.rs` @@ -58,7 +124,7 @@ - Use common error handling with conversions to HTTP responses - Wrap synchronous service calls in `tokio::task::spawn_blocking` -## 5. Refactor MCP Tools to Use Services +## 7. Refactor MCP Tools to Use Services **Files:** `src/modes/server/mcp/tools.rs` **Reason:** Remove duplication with REST API and CLI modes @@ -68,7 +134,7 @@ - Use synchronous services directly (MCP is typically local/short-lived) - Standardize response format to match API/CLI -## 6. Create Common Error Handling +## 8. Create Common Error Handling **Files:** `src/core/error.rs` **Reason:** Standardize error types across the application **Implementation:** @@ -84,7 +150,7 @@ - Provide user-friendly error messages - Include error codes for programmatic handling -## 7. Update Database Layer for Batch Operations +## 9. Update Database Layer for Batch Operations **Files:** `src/db.rs` **Reason:** Support efficient batch operations needed by services **Implementation:** @@ -94,7 +160,7 @@ - Optimize queries for common access patterns - Ensure all batch operations are properly documented -## 8. Add Integration Tests +## 10. Add Integration Tests **Files:** `tests/integration/` (new directory) **Reason:** Ensure refactored code maintains functionality and performance **Implementation:** @@ -105,7 +171,7 @@ - Use in-memory databases and tempfiles for isolation - Test both sync and async service implementations -## 9. Performance Optimization Guidelines +## 11. Performance Optimization Guidelines **Reason:** Ensure the refactored version doesn't slow down pipelines **Implementation:** - Use streaming for stdin/stdout processing (chunked I/O)