refactor: complete SOLID fixes + RAII guards + enhanced test assertions
CI / CI (pull_request) Failing after 21m18s
CI / CI (pull_request) Failing after 21m18s
ISP (Interface Segregation Principle): - Add JobStatusStore trait for database persistence - Implement PgJobStatusStore for PostgreSQL - Add MockJobStatusStore for unit testing - IngestWorker::with_job_store() enables dependency injection - Job status updates now via trait (testable, mockable) Resource Management (RAII): - Add PortForwardGuard struct with Drop impl - Ensures port-forward process killed even if test panics - Prevents resource leaks in integration tests Test Assertions (Verification): - Enhanced unit tests verify entity names, not just counts - Verify edges connect correct entity pairs - Verify both source and target entities exist - Batch processing verifies expected entities extracted - Entity deduplication test across multiple records All 9 issues now fixed: ✅ DRY: Removed wrapper function ✅ CRAP: Extracted save_*_with_logging() helpers ✅ OCP: Added JobStatus enum ✅ SRP: Real-time logging, no error accumulation ✅ ISP: JobStatusStore trait + mocking ✅ Logging: IngestLogContext struct ✅ RAII: PortForwardGuard for cleanup ✅ Error handling: Real-time logging at point of failure ✅ Test assertions: Verify names + connections Verification: cargo check -p mem-cli ✓
This commit is contained in:
@@ -34,6 +34,36 @@ impl std::fmt::Display for JobStatus {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Mock JobStatusStore for testing
|
||||
pub struct MockJobStatusStore {
|
||||
updates: std::sync::Arc<std::sync::Mutex<Vec<(String, JobStatus)>>>,
|
||||
}
|
||||
|
||||
impl MockJobStatusStore {
|
||||
pub fn new() -> Self {
|
||||
Self {
|
||||
updates: std::sync::Arc::new(std::sync::Mutex::new(Vec::new())),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn updates(&self) -> Vec<(String, JobStatus)> {
|
||||
self.updates.lock().unwrap().clone()
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait::async_trait]
|
||||
impl JobStatusStore for MockJobStatusStore {
|
||||
async fn update_status(&self, ingest_id: &str, status: JobStatus) -> Result<()> {
|
||||
self.updates.lock().unwrap().push((ingest_id.to_string(), status));
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Structured logging context for ingest operations — ensures consistent field names across all logs
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct IngestLogContext {
|
||||
@@ -54,6 +84,36 @@ impl IngestLogContext {
|
||||
}
|
||||
}
|
||||
|
||||
/// Job status store trait — abstracts database persistence of job status (enables mocking)
|
||||
#[async_trait::async_trait]
|
||||
pub trait JobStatusStore: Send + Sync {
|
||||
/// Update job status in storage
|
||||
async fn update_status(&self, ingest_id: &str, status: JobStatus) -> Result<()>;
|
||||
}
|
||||
|
||||
/// PostgreSQL implementation of JobStatusStore
|
||||
pub struct PgJobStatusStore {
|
||||
pool: PgPool,
|
||||
}
|
||||
|
||||
impl PgJobStatusStore {
|
||||
pub fn new(pool: PgPool) -> Self {
|
||||
Self { pool }
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait::async_trait]
|
||||
impl JobStatusStore for PgJobStatusStore {
|
||||
async fn update_status(&self, ingest_id: &str, status: JobStatus) -> Result<()> {
|
||||
sqlx::query("UPDATE ingest_jobs SET status=$1, started_at=NOW() WHERE ingest_id=$2")
|
||||
.bind(status.as_str())
|
||||
.bind(ingest_id)
|
||||
.execute(&self.pool)
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
/// Ingest worker — processes queued records through entity/fact extraction pipeline
|
||||
pub struct IngestWorker {
|
||||
@@ -61,6 +121,7 @@ pub struct IngestWorker {
|
||||
vector_store: Arc<VectorStore>,
|
||||
embeddings: Arc<EmbeddingsClient>,
|
||||
pipeline: Arc<IngestPipeline>,
|
||||
job_status_store: Arc<dyn JobStatusStore>,
|
||||
}
|
||||
|
||||
impl IngestWorker {
|
||||
@@ -68,6 +129,16 @@ impl IngestWorker {
|
||||
pub fn new(
|
||||
pool: PgPool,
|
||||
embeddings: EmbeddingsClient,
|
||||
) -> Self {
|
||||
let job_status_store = Arc::new(PgJobStatusStore::new(pool.clone()));
|
||||
Self::with_job_store(pool, embeddings, job_status_store)
|
||||
}
|
||||
|
||||
/// Create worker with custom job status store (for testing)
|
||||
pub fn with_job_store(
|
||||
pool: PgPool,
|
||||
embeddings: EmbeddingsClient,
|
||||
job_status_store: Arc<dyn JobStatusStore>,
|
||||
) -> Self {
|
||||
let vector_store = Arc::new(VectorStore::new(pool.clone()));
|
||||
|
||||
@@ -102,6 +173,7 @@ impl IngestWorker {
|
||||
vector_store,
|
||||
embeddings: Arc::new(embeddings),
|
||||
pipeline,
|
||||
job_status_store,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -128,13 +200,8 @@ impl IngestWorker {
|
||||
"Starting ingest job"
|
||||
);
|
||||
|
||||
// Update job status to processing
|
||||
if let Err(e) = sqlx::query("UPDATE ingest_jobs SET status=$1, started_at=NOW() WHERE ingest_id=$2")
|
||||
.bind(JobStatus::Processing.as_str())
|
||||
.bind(ingest_id)
|
||||
.execute(&self.pool)
|
||||
.await
|
||||
{
|
||||
// Update job status to processing (via trait, testable)
|
||||
if let Err(e) = self.job_status_store.update_status(ingest_id, JobStatus::Processing).await {
|
||||
tracing::error!(
|
||||
target: "ingest",
|
||||
error = %e,
|
||||
@@ -213,14 +280,9 @@ impl IngestWorker {
|
||||
}
|
||||
}
|
||||
|
||||
// Mark job complete
|
||||
// Mark job complete (via trait, testable)
|
||||
let final_status = JobStatus::Done;
|
||||
if let Err(e) = sqlx::query("UPDATE ingest_jobs SET status=$1, completed_at=NOW() WHERE ingest_id=$2")
|
||||
.bind(final_status.as_str())
|
||||
.bind(ingest_id)
|
||||
.execute(&self.pool)
|
||||
.await
|
||||
{
|
||||
if let Err(e) = self.job_status_store.update_status(ingest_id, final_status).await {
|
||||
tracing::error!(
|
||||
target: "ingest",
|
||||
error = %e,
|
||||
@@ -315,7 +377,8 @@ async fn save_entity_with_logging(
|
||||
}
|
||||
}
|
||||
|
||||
/// Save entity to database via raw SQL (normally would use EntityRepo trait)
|
||||
/// Save entity to database via raw SQL (normally would use EntityRepo trait)
|
||||
/// NOTE: async_trait requires manual implementation for non-trait functions
|
||||
async fn save_entity_to_db(pool: &PgPool, entity: &mem_core::entity::Entity) -> Result<()> {
|
||||
// Convert OffsetDateTime to PostgreSQL timestamp format
|
||||
let t_created_str = entity.t_created.to_string();
|
||||
|
||||
Reference in New Issue
Block a user