From 804f919f12b13f2f64da61d3886a90fdcc846a13 Mon Sep 17 00:00:00 2001 From: Samir Boulahtit Date: Wed, 3 Dec 2025 22:05:00 +0100 Subject: [PATCH] docs: add SVC-006 migration plan for db.commit() refactoring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comprehensive migration plan covering: - Migration pattern (before/after examples) - Services prioritized by impact (18 services, 60 commits) - Step-by-step migration process - Testing strategy - Rollback procedures - Progress tracking commands This supports gradual migration from service-level commits to endpoint-level transaction control. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- docs/development/svc-006-migration-plan.md | 266 +++++++++++++++++++++ 1 file changed, 266 insertions(+) create mode 100644 docs/development/svc-006-migration-plan.md diff --git a/docs/development/svc-006-migration-plan.md b/docs/development/svc-006-migration-plan.md new file mode 100644 index 00000000..256cfbaa --- /dev/null +++ b/docs/development/svc-006-migration-plan.md @@ -0,0 +1,266 @@ +# SVC-006 Migration Plan: Move db.commit() from Services to Endpoints + +## Overview + +**Objective:** Migrate all `db.commit()` calls from service layer to API endpoints to follow the industry-standard transaction control pattern. + +**Rule:** SVC-006 - Services must NOT call `db.commit()` + +**Principle:** One request = one transaction, with commit controlled at the API endpoint level. + +--- + +## Benefits + +| Benefit | Description | +|---------|-------------| +| **Composability** | Multiple service calls can be composed in a single transaction | +| **Clean rollback** | If any operation fails, the entire request rolls back automatically | +| **Testability** | Services can be tested in isolation without actual commits | +| **Consistency** | All transactions follow the same pattern | + +--- + +## Migration Pattern + +### Before (Anti-pattern) +```python +# Service +def create_vendor(self, db: Session, data: VendorCreate) -> Vendor: + vendor = Vendor(**data.model_dump()) + db.add(vendor) + db.commit() # ❌ Service commits + db.refresh(vendor) + return vendor + +# Endpoint +def create_vendor_endpoint(...): + vendor = vendor_service.create_vendor(db, data) + return VendorResponse.model_validate(vendor) +``` + +### After (Correct pattern) +```python +# Service +def create_vendor(self, db: Session, data: VendorCreate) -> Vendor: + vendor = Vendor(**data.model_dump()) + db.add(vendor) + db.flush() # ✅ Get ID without committing + return vendor + +# Endpoint +def create_vendor_endpoint(...): + vendor = vendor_service.create_vendor(db, data) + db.commit() # ✅ ARCH: Commit at API level for transaction control + return VendorResponse.model_validate(vendor) +``` + +### Key Changes + +1. **Replace `db.commit()` with `db.flush()`** in services when you need the ID +2. **Remove `db.refresh()`** from services (endpoint can do this if needed) +3. **Remove `db.rollback()`** from service exception handlers (endpoint handles this) +4. **Add `db.commit()`** to the endpoint after service call(s) + +--- + +## Services to Migrate + +### Priority 1: Core Business Services (High Impact) +| Service | Commits | Complexity | Endpoints to Update | +|---------|---------|------------|---------------------| +| `admin_service.py` | 6 | Medium | Multiple admin endpoints | +| `inventory_service.py` | 9 | High | Inventory management endpoints | +| `product_service.py` | 3 | Medium | Product CRUD endpoints | +| `order_service.py` | 2 | Medium | Order processing endpoints | + +### Priority 2: Domain Services (Medium Impact) +| Service | Commits | Complexity | Endpoints to Update | +|---------|---------|------------|---------------------| +| `vendor_domain_service.py` | 4 | Medium | Domain management endpoints | +| `vendor_team_service.py` | 5 | Medium | Team management endpoints | +| `vendor_theme_service.py` | 3 | Low | Theme endpoints | +| `customer_service.py` | 4 | Medium | Customer endpoints | +| `cart_service.py` | 5 | Medium | Cart/checkout endpoints | + +### Priority 3: Supporting Services (Lower Impact) +| Service | Commits | Complexity | Endpoints to Update | +|---------|---------|------------|---------------------| +| `content_page_service.py` | 3 | Low | CMS endpoints | +| `marketplace_product_service.py` | 3 | Low | Marketplace endpoints | +| `team_service.py` | 2 | Low | Team endpoints | +| `admin_settings_service.py` | 3 | Low | Settings endpoints | +| `code_quality_service.py` | 5 | Low | Internal tooling | + +### Priority 4: Auth & Audit (Special Handling) +| Service | Commits | Notes | +|---------|---------|-------| +| `auth_service.py` | 1 | Handle carefully - auth flows | +| `admin_audit_service.py` | 1 | May need immediate commit for audit trail | +| `marketplace_import_job_service.py` | 1 | Background job - special handling | + +### Excluded +| Service | Commits | Reason | +|---------|---------|--------| +| `log_service.py` | 2 | Audit logs require immediate commits | + +--- + +## Completed Migrations + +- [x] `vendor_service.py` (6 commits → 0) - Commit: 6bd3af0 + +--- + +## Migration Steps (Per Service) + +### Step 1: Identify Commits +```bash +grep -n "db.commit()" app/services/.py +``` + +### Step 2: Find Calling Endpoints +```bash +grep -rn "_service\." app/api/ --include="*.py" +``` + +### Step 3: Update Service +For each method with `db.commit()`: + +1. **If creating entities:** + ```python + # Before + db.add(entity) + db.commit() + db.refresh(entity) + + # After + db.add(entity) + db.flush() # Get ID without committing + ``` + +2. **If updating entities:** + ```python + # Before + entity.field = value + db.commit() + db.refresh(entity) + + # After + entity.field = value + # No commit - endpoint handles it + ``` + +3. **Remove rollback from exception handlers:** + ```python + # Before + except SomeException: + db.rollback() + raise + + # After + except SomeException: + raise # Endpoint handles rollback + ``` + +### Step 4: Update Endpoints +Add `db.commit()` after service calls: +```python +result = some_service.some_method(db, data) +db.commit() # ✅ ARCH: Commit at API level for transaction control +return SomeResponse(...) +``` + +### Step 5: Validate +```bash +# Check no commits remain in service +grep -n "db.commit()" app/services/.py + +# Run architecture validator +python scripts/validate_architecture.py -o + +# Run tests +pytest tests/test_.py -v +``` + +### Step 6: Commit +```bash +git add app/services/.py app/api/v1/... +git commit -m "refactor(): migrate db.commit() from service to endpoints (SVC-006)" +``` + +--- + +## Testing Strategy + +### Unit Tests +- Services should be tested with `db.flush()` (no actual commit) +- Mock the session or use a test transaction that rolls back + +### Integration Tests +- Test full request flow including commit +- Verify data persists after endpoint call +- Verify rollback on exception + +### Manual Testing +After each migration: +1. Test create operations - verify data persists +2. Test update operations - verify changes persist +3. Test error scenarios - verify no partial commits + +--- + +## Rollback Strategy + +If issues are found after migration: + +1. **Revert the commit:** + ```bash + git revert + ``` + +2. **Or fix forward:** + - Add `db.commit()` back to service temporarily + - Investigate the endpoint that's missing commit + - Fix and re-deploy + +--- + +## Progress Tracking + +Run this command to check remaining violations: +```bash +python scripts/validate_architecture.py 2>&1 | grep "SVC-006" | wc -l +``` + +Current status: **60 remaining** (down from 66) + +--- + +## Timeline Recommendation + +This migration can be done gradually: + +1. **Immediate:** Fix as you touch each service for other changes +2. **Batch:** Dedicate time to migrate 2-3 services per session +3. **Priority:** Focus on Priority 1 services first for maximum impact + +The SVC-006 rule is set to **warning** (not error), so CI won't fail. This allows gradual migration without blocking deployments. + +--- + +## Commands Reference + +```bash +# Count remaining violations +python scripts/validate_architecture.py 2>&1 | grep "SVC-006" | wc -l + +# List services with commits (sorted by count) +grep -c "db.commit()" app/services/*.py | grep -v ":0$" | sort -t: -k2 -n + +# Validate specific entity +python scripts/validate_architecture.py -o vendor + +# Validate specific file +python scripts/validate_architecture.py -f app/services/admin_service.py +```