docs: merge ARCHITECTURE_REFACTORING into memory-wiki-graph-rag-optimization.md
Integrated SOLID + DRY optimizations as new section: - Scoring pipeline (DocumentScorer trait, ScoringPipeline orchestrator) - Policy provider (PolicyProvider trait, pluggable Vault/Postgres/Redis) - RBAC decision engine (AccessChecker composition, short-circuit eval) - Test fixtures (OidcClaimsBuilder, AccessPolicyBuilder) Implementation priority: 1. ScoringPipeline (Phase 3) 2. PolicyProvider trait (Phase 7) 3. AccessChecker composition (Phase 7) 4. Test fixtures (All phases) Unified doc now has: architecture + concrete implementation + SOLID refactoring.
This commit is contained in:
@@ -1290,6 +1290,271 @@ cargo bench --bench wiki_graph_retrieval
|
||||
|
||||
|
||||
|
||||
---
|
||||
|
||||
## Architecture Refactoring: SOLID + DRY Optimization
|
||||
|
||||
### Problem Analysis
|
||||
|
||||
Without refactoring, the design has antipatterns:
|
||||
|
||||
```
|
||||
Current (Tightly Coupled):
|
||||
TfIdfIndex ─┬─ score_chunk() [duplicated logic across GlobalTfIdfIndex,
|
||||
│ ProjectTfIdfIndex, ChunkMetadataIndex]
|
||||
├─ cache management [responsibility mixing]
|
||||
└─ build_from_vault() [coupled to Vault directly]
|
||||
|
||||
RbacEngine ─┬─ load_policy() [duplicated in multiple methods]
|
||||
├─ check_access() [fat method, 300+ lines, multiple concerns]
|
||||
└─ audit_log() [mixed responsibility]
|
||||
|
||||
WikiLinkParser ─ resolve_path() called in multiple places
|
||||
(logic varies, not DRY)
|
||||
```
|
||||
|
||||
### Solution: Trait-Based Architecture
|
||||
|
||||
#### 1. Scoring Pipeline (Single DocumentScorer trait)
|
||||
|
||||
**Problem:** TF-IDF scoring logic duplicated in GlobalTfIdfIndex, ProjectTfIdfIndex, SemanticScorer, ChunkMetadataIndex.
|
||||
|
||||
**Solution:**
|
||||
|
||||
```rust
|
||||
/// Single interface: one scorer, one job
|
||||
pub trait DocumentScorer: Send + Sync {
|
||||
async fn score(&self, query: &str, doc_id: &str) -> Result<f32>;
|
||||
fn name(&self) -> &str; // for debugging/metrics
|
||||
}
|
||||
|
||||
// All scoring variants implement the same trait
|
||||
pub struct GlobalTfIdfScorer { vocabulary: Arc<...> }
|
||||
impl DocumentScorer for GlobalTfIdfScorer { ... }
|
||||
|
||||
pub struct ProjectTfIdfScorer { project: String, vocabulary: Arc<...> }
|
||||
impl DocumentScorer for ProjectTfIdfScorer { ... }
|
||||
|
||||
pub struct SemanticScorer { embeddings: Arc<...>, pgvector: Arc<...> }
|
||||
impl DocumentScorer for SemanticScorer { ... }
|
||||
|
||||
pub struct MetadataBoostingScorer {
|
||||
base_scorer: Arc<dyn DocumentScorer>, // Composition, not inheritance
|
||||
metadata: Arc<ChunkMetadataCache>,
|
||||
boost_factor: f32,
|
||||
}
|
||||
impl DocumentScorer for MetadataBoostingScorer { ... }
|
||||
```
|
||||
|
||||
**Multi-Scorer Orchestrator:**
|
||||
|
||||
```rust
|
||||
pub struct ScoringPipeline {
|
||||
scorers: Vec<(String, f32, Arc<dyn DocumentScorer>)>, // name, weight, scorer
|
||||
}
|
||||
|
||||
impl ScoringPipeline {
|
||||
pub fn new() -> Self { ... }
|
||||
|
||||
pub fn with_scorer(
|
||||
mut self,
|
||||
name: &str,
|
||||
weight: f32,
|
||||
scorer: Arc<dyn DocumentScorer>,
|
||||
) -> Self {
|
||||
self.scorers.push((name.to_string(), weight, scorer));
|
||||
self
|
||||
}
|
||||
|
||||
/// Execute all scorers in parallel, fuse with RRF
|
||||
pub async fn score(&self, query: &str, doc_id: &str) -> Result<f32> {
|
||||
let scores = futures::stream::iter(&self.scorers)
|
||||
.then(|(_, _, scorer)| async move { scorer.score(query, doc_id).await })
|
||||
.collect::<Result<Vec<_>>>()
|
||||
.await?;
|
||||
|
||||
// RRF: weighted sum of normalized scores
|
||||
let weighted_sum: f32 = self.scorers
|
||||
.iter()
|
||||
.zip(scores)
|
||||
.map(|((_, weight, _), score)| weight * score)
|
||||
.sum();
|
||||
|
||||
Ok(weighted_sum / self.scorers.iter().map(|(_, w, _)| w).sum::<f32>())
|
||||
}
|
||||
}
|
||||
|
||||
// Usage:
|
||||
let pipeline = ScoringPipeline::new()
|
||||
.with_scorer("global-tfidf", 0.1, Arc::new(global_tfidf))
|
||||
.with_scorer("project-tfidf", 0.3, Arc::new(project_tfidf))
|
||||
.with_scorer("semantic", 0.6, Arc::new(semantic));
|
||||
|
||||
let final_score = pipeline.score(query, doc_id).await?;
|
||||
```
|
||||
|
||||
**Benefits:** ✅ DRY | ✅ S | ✅ O | ✅ L | ✅ D | ✅ Testable
|
||||
|
||||
#### 2. Policy Provider (Pluggable backend)
|
||||
|
||||
**Problem:** RbacEngine tightly coupled to Vault. To support Postgres or Redis requires modifying multiple methods.
|
||||
|
||||
**Solution:**
|
||||
|
||||
```rust
|
||||
#[async_trait]
|
||||
pub trait PolicyProvider: Send + Sync {
|
||||
async fn get_policy(&self, resource_type: &str, resource_name: &str) -> Result<AccessPolicy>;
|
||||
async fn invalidate_cache(&self, resource_type: &str, name: &str) -> Result<()>;
|
||||
}
|
||||
|
||||
// Vault implementation
|
||||
pub struct VaultPolicyProvider {
|
||||
vault_root: PathBuf,
|
||||
cache: Arc<RwLock<LruCache<String, AccessPolicy>>>,
|
||||
}
|
||||
|
||||
// Alternative: Postgres backend
|
||||
pub struct DatabasePolicyProvider {
|
||||
db: Arc<PostgresPool>,
|
||||
}
|
||||
|
||||
// Decorator: Redis cache layer
|
||||
pub struct CachedPolicyProvider {
|
||||
inner: Arc<dyn PolicyProvider>,
|
||||
redis: Arc<redis::aio::Connection>,
|
||||
ttl_secs: u64,
|
||||
}
|
||||
|
||||
// Usage (all identical interface):
|
||||
let provider: Arc<dyn PolicyProvider> = match env {
|
||||
"vault" => Arc::new(VaultPolicyProvider::new(vault_root)),
|
||||
"postgres" => Arc::new(DatabasePolicyProvider::new(db)),
|
||||
};
|
||||
|
||||
let policy = provider.get_policy("project", "poimen").await?;
|
||||
```
|
||||
|
||||
**Benefits:** ✅ O | ✅ D | ✅ Composition | ✅ Testable
|
||||
|
||||
#### 3. RBAC Decision Engine (Composition of checkers)
|
||||
|
||||
**Problem:** RbacEngine.check_access() is 300+ lines mixing access level, role, permission, and audit concerns.
|
||||
|
||||
**Solution:**
|
||||
|
||||
```rust
|
||||
#[async_trait]
|
||||
pub trait AccessChecker: Send + Sync {
|
||||
async fn check(&self, claims: &OidcClaims, policy: &AccessPolicy) -> Result<bool>;
|
||||
fn description(&self) -> &str;
|
||||
}
|
||||
|
||||
// Single-purpose checkers
|
||||
pub struct AccessLevelChecker; // public | group | private
|
||||
pub struct RoleChecker; // verify required_role
|
||||
pub struct PermissionChecker; // verify required_permission
|
||||
|
||||
impl AccessChecker for AccessLevelChecker { ... }
|
||||
impl AccessChecker for RoleChecker { ... }
|
||||
impl AccessChecker for PermissionChecker { ... }
|
||||
|
||||
/// Orchestrator: compose all checkers (short-circuit evaluation)
|
||||
pub struct AccessDecisionEngine {
|
||||
checkers: Vec<Arc<dyn AccessChecker>>,
|
||||
policy_provider: Arc<dyn PolicyProvider>,
|
||||
audit: Arc<dyn AuditLogger>,
|
||||
}
|
||||
|
||||
impl AccessDecisionEngine {
|
||||
pub async fn check_access(
|
||||
&self,
|
||||
claims: &OidcClaims,
|
||||
resource_type: &str,
|
||||
resource_name: &str,
|
||||
) -> Result<bool> {
|
||||
let policy = self.policy_provider.get_policy(resource_type, resource_name).await?;
|
||||
|
||||
// Evaluate all checkers (short-circuit on failure)
|
||||
for checker in &self.checkers {
|
||||
if !checker.check(claims, &policy).await? {
|
||||
self.audit.log_decision(deny(checker.description())).await?;
|
||||
return Ok(false);
|
||||
}
|
||||
}
|
||||
|
||||
self.audit.log_decision(allow()).await?;
|
||||
Ok(true)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Benefits:** ✅ S | ✅ I | ✅ D | ✅ Easy to extend | ✅ Easy to test
|
||||
|
||||
#### 4. Test Fixtures (Reusable builders)
|
||||
|
||||
**Problem:** Test setup code repeated (creating OidcClaims, AccessPolicy, etc.).
|
||||
|
||||
**Solution:**
|
||||
|
||||
```rust
|
||||
// tests/fixtures/builders.rs
|
||||
pub struct OidcClaimsBuilder {
|
||||
sub: String,
|
||||
groups: Vec<String>,
|
||||
roles: Vec<String>,
|
||||
permissions: Vec<String>,
|
||||
}
|
||||
|
||||
impl OidcClaimsBuilder {
|
||||
pub fn new(sub: &str) -> Self { ... }
|
||||
pub fn group(mut self, group: &str) -> Self { ... }
|
||||
pub fn role(mut self, role: &str) -> Self { ... }
|
||||
pub fn permission(mut self, perm: &str) -> Self { ... }
|
||||
pub fn build(self) -> OidcClaims { ... }
|
||||
}
|
||||
|
||||
pub struct AccessPolicyBuilder { ... }
|
||||
|
||||
// Usage in tests:
|
||||
#[tokio::test]
|
||||
async fn test_rbac_project_access_group() {
|
||||
let claims = OidcClaimsBuilder::new("charlie")
|
||||
.group("platform-team")
|
||||
.permission("memory:read")
|
||||
.build();
|
||||
|
||||
let policy = AccessPolicyBuilder::public()
|
||||
.group(vec!["platform-team"])
|
||||
.build();
|
||||
|
||||
let engine = AccessDecisionEngine::new(provider, audit);
|
||||
let allowed = engine.check_access(&claims, "project", "poimen").await?;
|
||||
assert!(allowed);
|
||||
}
|
||||
```
|
||||
|
||||
**Benefits:** ✅ DRY | ✅ Readable | ✅ Maintainable | ✅ Flexible
|
||||
|
||||
### Implementation Priority
|
||||
|
||||
1. **ScoringPipeline** (Phase 3) — unblocks all TF-IDF work
|
||||
2. **PolicyProvider trait** (Phase 7) — enables Vault/Postgres/Redis swap
|
||||
3. **AccessChecker composition** (Phase 7) — splits fat RBAC method
|
||||
4. **Test fixtures** (All phases) — immediate DRY wins
|
||||
|
||||
### Summary: SOLID + DRY Improvements
|
||||
|
||||
| Aspect | Before | After |
|
||||
|---|---|---|
|
||||
| **Code duplication** | TF-IDF logic in 3+ places | 1 trait, N implementations |
|
||||
| **New scorer** | Modify TfIdfIndex | New struct + impl DocumentScorer |
|
||||
| **Policy source** | Vault-only | Swap PolicyProvider trait |
|
||||
| **RBAC checks** | 1 fat method (300 lines) | 3 single-purpose checkers |
|
||||
| **Audit logging** | Hardcoded to Postgres | Pluggable AuditLogger trait |
|
||||
| **Test setup** | Repeated code | Reusable builders |
|
||||
| **Testability** | Hard to mock | Mock any trait |
|
||||
|
||||
---
|
||||
|
||||
## Expected Outcomes
|
||||
|
||||
Reference in New Issue
Block a user