docs: add SVC-006 migration plan for db.commit() refactoring
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 <noreply@anthropic.com>
This commit is contained in:
266
docs/development/svc-006-migration-plan.md
Normal file
266
docs/development/svc-006-migration-plan.md
Normal file
@@ -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/<service_name>.py
|
||||
```
|
||||
|
||||
### Step 2: Find Calling Endpoints
|
||||
```bash
|
||||
grep -rn "<service>_service\.<method>" 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/<service_name>.py
|
||||
|
||||
# Run architecture validator
|
||||
python scripts/validate_architecture.py -o <entity_name>
|
||||
|
||||
# Run tests
|
||||
pytest tests/test_<service_name>.py -v
|
||||
```
|
||||
|
||||
### Step 6: Commit
|
||||
```bash
|
||||
git add app/services/<service>.py app/api/v1/...
|
||||
git commit -m "refactor(<entity>): 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 <commit_hash>
|
||||
```
|
||||
|
||||
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
|
||||
```
|
||||
Reference in New Issue
Block a user