chore: reorder refactoring plan for logical flow
Co-authored-by: aider (openai/andrew/openrouter/mistralai/mistral-medium-3.1) <aider@aider.chat>
This commit is contained in:
Andrew Phillips
211
PLAN.md
211
PLAN.md
@@ -1,13 +1,74 @@
|
||||
# Refactoring Plan to Reduce Code Duplication
|
||||
|
||||
## 1. Create Core Service Layer with Clear Boundaries
|
||||
## Implementation Order:
|
||||
1. Create core module structure and error types
|
||||
2. Create common data structures
|
||||
3. Update database layer for batch operations
|
||||
4. Create core services with clear boundaries (synchronous)
|
||||
5. Add async wrappers for API use
|
||||
6. Refactor CLI modes to use services
|
||||
7. Refactor REST API to use async services
|
||||
8. Refactor MCP tools to use services
|
||||
9. Create unified error handling
|
||||
10. Add integration tests
|
||||
11. Add performance optimization guidelines
|
||||
|
||||
## 1. Create Core Module Structure and Error Types
|
||||
**Files:**
|
||||
- Add: `src/core/error.rs`
|
||||
- Add: `src/core/mod.rs`
|
||||
|
||||
**Functions:**
|
||||
- Add: `CoreError` enum with comprehensive variants
|
||||
- Add: Conversion traits for all error types
|
||||
|
||||
**Reason:** Establish foundation for consistent error handling
|
||||
**Implementation:**
|
||||
- Define base error enum with conversions to all interface-specific error types
|
||||
- Use `#[derive(thiserror::Error)]` for easy `Display` and `Error` implementations
|
||||
- Provide user-friendly error messages with error codes
|
||||
|
||||
## 2. Create Common Data Structures
|
||||
**Files:**
|
||||
- Add: `src/core/types.rs`
|
||||
|
||||
**Functions:**
|
||||
- Add: `ItemWithMeta` struct
|
||||
- Add: `ItemWithContent` struct
|
||||
- Add: `From<db::Item>` for `ItemWithMeta`
|
||||
- Add: `From<ItemWithMeta>` for `ItemInfo` (API response)
|
||||
- Add: Serialization/deserialization implementations
|
||||
|
||||
**Reason:** Standardize data structures used across modes and APIs
|
||||
**Implementation:**
|
||||
- Define structs for common data types
|
||||
- Include conversion functions from database types
|
||||
- Add serialization/deserialization support for JSON/YAML
|
||||
- Ensure all fields are properly documented
|
||||
|
||||
## 3. Update Database Layer for Batch Operations
|
||||
**Files:**
|
||||
- Change: `src/db.rs`
|
||||
|
||||
**Functions:**
|
||||
- Add: `get_items_with_meta_batch`
|
||||
- Add: `get_items_with_tags_batch`
|
||||
- Add: `insert_item_with_meta_transaction`
|
||||
- Add: `delete_item_with_meta_transaction`
|
||||
- Change: Optimize existing queries for batch operations
|
||||
|
||||
**Reason:** Support efficient batch operations needed by services
|
||||
**Implementation:**
|
||||
- Add functions to get multiple items with their metadata and tags
|
||||
- Add batch insertion/updates for tags and metadata
|
||||
- Add transaction support for atomic operations
|
||||
- Optimize queries for common access patterns
|
||||
|
||||
## 4. Create Core Service Layer with Clear Boundaries
|
||||
**Files:**
|
||||
- Add: `src/core/item_service.rs`
|
||||
- Add: `src/core/async_item_service.rs`
|
||||
- Add: `src/core/compression_service.rs`
|
||||
- Add: `src/core/meta_service.rs`
|
||||
- Add: `src/core/mod.rs`
|
||||
- Add: `src/core/error.rs`
|
||||
|
||||
**Functions:**
|
||||
- Add: `get_item_full` in `item_service.rs`
|
||||
@@ -16,7 +77,6 @@
|
||||
- Add: `delete_item` in `item_service.rs`
|
||||
- Add: `get_compressed_content` in `compression_service.rs`
|
||||
- Add: `process_metadata` in `meta_service.rs`
|
||||
- Add: Async wrappers for all core functions in `async_item_service.rs`
|
||||
|
||||
**Reason:** Extract common business logic from modes and APIs into reusable services
|
||||
**Implementation:**
|
||||
@@ -24,116 +84,30 @@
|
||||
- Services should return structured data, not format output
|
||||
- Handle compression, metadata, and database operations
|
||||
- Keep core services **synchronous** for CLI performance
|
||||
- Provide async wrappers using `tokio::task::spawn_blocking` for API use
|
||||
- Use streaming for pipeline efficiency (process data in chunks)
|
||||
|
||||
**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<Tag>)>
|
||||
|
||||
// item_service.rs (high-level)
|
||||
pub fn get_item_full(conn: &Connection, id: i64) -> Result<ItemWithMeta> {
|
||||
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
|
||||
## 5. Add Async Wrappers for API Use
|
||||
**Files:**
|
||||
- Change: `src/modes/server/api/item.rs`
|
||||
- Change: `src/modes/server/api/status.rs`
|
||||
- Change: `src/modes/server/mcp/tools.rs`
|
||||
- Add: `src/core/async_item_service.rs`
|
||||
|
||||
**Functions:**
|
||||
- Add: `get_item_async` in `async_item_service.rs`
|
||||
- Add: `save_item_async` in `async_item_service.rs`
|
||||
- Change: All API handlers to use async services
|
||||
- Add: `list_items_async` in `async_item_service.rs`
|
||||
- Add: `delete_item_async` in `async_item_service.rs`
|
||||
|
||||
**Reason:** Ensure services can be safely used in both synchronous and asynchronous contexts
|
||||
**Reason:** Ensure services can be safely used in asynchronous contexts
|
||||
**Implementation:**
|
||||
- Document thread-safety guarantees for all core services
|
||||
- Use `Arc<Mutex<T>>` 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<ItemWithMeta, CoreError> {
|
||||
let conn = state.db.clone(); // Arc<Mutex<Connection>>
|
||||
tokio::task::spawn_blocking(move || {
|
||||
let conn = conn.lock().unwrap();
|
||||
item_service::get_item_full(&conn, id)
|
||||
})
|
||||
.await?
|
||||
}
|
||||
```
|
||||
- Use `Arc<Mutex<T>>` or connection pooling for shared resources
|
||||
- Provide examples for safe async/sync boundaries
|
||||
- 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:**
|
||||
- Add: `src/core/types.rs`
|
||||
|
||||
**Functions:**
|
||||
- Add: `From<db::Item>` for `ItemWithMeta`
|
||||
- Add: `From<ItemWithMeta>` for `ItemInfo` (API response)
|
||||
- Add: Serialization/deserialization implementations
|
||||
|
||||
**Reason:** Standardize data structures used across modes and APIs
|
||||
**Implementation:**
|
||||
- Define structs for `Item`, `ItemWithContent`, `ItemWithMeta`, `Response<T>`
|
||||
- Include conversion functions from database types (`From<db::Item>`)
|
||||
- Add serialization/deserialization support for JSON/YAML
|
||||
- Ensure all fields are properly documented
|
||||
- Use zero-copy patterns where possible (slicing instead of copying)
|
||||
|
||||
## 4. Unified Error Handling with Conversions
|
||||
**Files:**
|
||||
- Add: `src/core/error.rs`
|
||||
- Change: `src/modes/server/api/item.rs`
|
||||
- Change: `src/modes/server/mcp/tools.rs`
|
||||
- Change: All mode files (`get.rs`, `save.rs`, etc.)
|
||||
|
||||
**Functions:**
|
||||
- Add: `CoreError` enum with comprehensive variants
|
||||
- Add: `From<CoreError>` for `StatusCode`
|
||||
- Add: `From<CoreError>` for `ToolError`
|
||||
- Add: `From<CoreError>` for `anyhow::Error`
|
||||
- Change: All error handling to use `CoreError`
|
||||
|
||||
**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<CoreError> 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
|
||||
## 6. Refactor CLI Modes to Use Services
|
||||
**Files:**
|
||||
- Change: `src/modes/get.rs`
|
||||
- Change: `src/modes/save.rs`
|
||||
@@ -160,7 +134,7 @@
|
||||
- Use synchronous services directly
|
||||
- Implement streaming for stdin/stdout to maintain pipeline performance
|
||||
|
||||
## 6. Refactor REST API to Use Async Services
|
||||
## 7. Refactor REST API to Use Async Services
|
||||
**Files:**
|
||||
- Change: `src/modes/server/api/item.rs`
|
||||
- Change: `src/modes/server/api/status.rs`
|
||||
@@ -181,7 +155,7 @@
|
||||
- Use common error handling with conversions to HTTP responses
|
||||
- Wrap synchronous service calls in `tokio::task::spawn_blocking`
|
||||
|
||||
## 7. Refactor MCP Tools to Use Services
|
||||
## 8. Refactor MCP Tools to Use Services
|
||||
**Files:**
|
||||
- Change: `src/modes/server/mcp/tools.rs`
|
||||
|
||||
@@ -199,13 +173,11 @@
|
||||
- Use synchronous services directly (MCP is typically local/short-lived)
|
||||
- Standardize response format to match API/CLI
|
||||
|
||||
## 8. Create Common Error Handling
|
||||
## 9. Create Unified Error Handling
|
||||
**Files:**
|
||||
- Add: `src/core/error.rs`
|
||||
- Change: All files that handle errors
|
||||
|
||||
**Functions:**
|
||||
- Add: Comprehensive error handling in `core/error.rs`
|
||||
- Add: Conversion traits for all error types
|
||||
- Change: All error handling to use new error system
|
||||
|
||||
@@ -223,25 +195,6 @@
|
||||
- Provide user-friendly error messages
|
||||
- Include error codes for programmatic handling
|
||||
|
||||
## 9. Update Database Layer for Batch Operations
|
||||
**Files:**
|
||||
- Change: `src/db.rs`
|
||||
|
||||
**Functions:**
|
||||
- Add: `get_items_with_meta_batch`
|
||||
- Add: `get_items_with_tags_batch`
|
||||
- Add: `insert_item_with_meta_transaction`
|
||||
- Add: `delete_item_with_meta_transaction`
|
||||
- Change: Optimize existing queries for batch operations
|
||||
|
||||
**Reason:** Support efficient batch operations needed by services
|
||||
**Implementation:**
|
||||
- Add functions to get multiple items with their metadata and tags
|
||||
- Add batch insertion/updates for tags and metadata
|
||||
- Add transaction support for atomic operations
|
||||
- Optimize queries for common access patterns
|
||||
- Ensure all batch operations are properly documented
|
||||
|
||||
## 10. Add Integration Tests
|
||||
**Files:**
|
||||
- Add: `tests/integration/core_tests.rs`
|
||||
@@ -284,18 +237,6 @@
|
||||
- Benchmark critical operations before/after refactoring
|
||||
- Document performance characteristics and tradeoffs
|
||||
|
||||
## Implementation Order:
|
||||
1. Create core module structure and error types (`core/error.rs`, `core/types.rs`)
|
||||
2. Implement core services with basic functionality (sync first)
|
||||
3. Add async wrappers for API use
|
||||
4. Refactor one mode (e.g., `get`) to use services and validate approach
|
||||
5. Refactor corresponding API endpoints to use async services
|
||||
6. Repeat for other modes and APIs
|
||||
7. Refactor MCP tools to use services
|
||||
8. Add comprehensive tests and benchmarks
|
||||
9. Clean up removed code from original files
|
||||
10. Document performance characteristics and tradeoffs
|
||||
|
||||
## Benefits:
|
||||
- Reduced code duplication between CLI, API, and MCP
|
||||
- Easier maintenance with clear separation of concerns
|
||||
|
||||
Reference in New Issue
Block a user