做完了一部分,备份一下,防止以外删除
This commit is contained in:
218
specs/001-fiber-middleware-integration/optional-improvements.md
Normal file
218
specs/001-fiber-middleware-integration/optional-improvements.md
Normal file
@@ -0,0 +1,218 @@
|
||||
# Optional Improvements for 001-fiber-middleware-integration
|
||||
|
||||
**Created**: 2025-11-11
|
||||
**Status**: Deferred - Address after core implementation
|
||||
**Source**: `/speckit.analyze` findings (Medium/Low priority)
|
||||
|
||||
---
|
||||
|
||||
## Medium Priority Improvements
|
||||
|
||||
### A2: Log Retention Policy Clarity
|
||||
- **Issue**: FR-006 mentions "configured retention policy" but doesn't specify units/format
|
||||
- **Current Impact**: Implementation will need to infer format
|
||||
- **Recommendation**: Add to spec.md FR-006:
|
||||
```
|
||||
Retention policy specified in days (integer), e.g., 30 for app logs, 90 for access logs.
|
||||
Implemented via Lumberjack MaxAge parameter.
|
||||
```
|
||||
- **Effort**: 10 minutes (documentation only)
|
||||
|
||||
---
|
||||
|
||||
### A3: Rate Limiting Default Values
|
||||
- **Issue**: FR-018a says "configurable requests per time window" but no default values
|
||||
- **Current Impact**: Developers must guess initial values
|
||||
- **Recommendation**: Add to spec.md FR-018a:
|
||||
```
|
||||
Default: 100 requests per minute per IP
|
||||
Supported time units: second (s), minute (m), hour (h)
|
||||
Example config: max=100, window=1m
|
||||
```
|
||||
- **Effort**: 15 minutes (spec update + config.yaml example)
|
||||
|
||||
---
|
||||
|
||||
### A5: Phase 0 Research Status
|
||||
- **Issue**: plan.md Phase 0 lists "Best Redis client" as unknown but Technical Context already decided
|
||||
- **Current Impact**: Confusion about whether research is needed
|
||||
- **Recommendation**: Update plan.md Phase 0:
|
||||
- Remove "Best Redis client" from unknowns (already decided: go-redis/redis/v8)
|
||||
- Or clarify: "Validate go-redis/redis/v8 choice (performance benchmarks, connection pool tuning)"
|
||||
- **Effort**: 5 minutes (documentation clarity)
|
||||
|
||||
---
|
||||
|
||||
### A6: Config Validation Rules Location
|
||||
- **Issue**: T009 says "config loading with validation" but validation rules not documented
|
||||
- **Current Impact**: Developer must design validation rules during implementation
|
||||
- **Recommendation**: Create section in data-model.md or spec.md:
|
||||
```markdown
|
||||
## Configuration Validation Rules
|
||||
- server.port: 1024-65535 (int)
|
||||
- server.host: non-empty string
|
||||
- redis.addr: host:port format
|
||||
- logging.max_size: 1-1000 (MB)
|
||||
- logging.max_age: 1-365 (days)
|
||||
```
|
||||
- **Effort**: 30 minutes (requires design decisions)
|
||||
|
||||
---
|
||||
|
||||
### U2: Access Log Format Specification
|
||||
- **Issue**: FR-011 mentions logging HTTP requests but doesn't specify JSON schema for access.log
|
||||
- **Current Impact**: Access log structure determined during implementation
|
||||
- **Recommendation**: Add to spec.md or data-model.md:
|
||||
```json
|
||||
{
|
||||
"timestamp": "2025-11-10T15:30:45Z",
|
||||
"level": "info",
|
||||
"request_id": "550e8400-e29b-...",
|
||||
"method": "GET",
|
||||
"path": "/api/v1/users",
|
||||
"status": 200,
|
||||
"duration_ms": 45,
|
||||
"ip": "192.168.1.100",
|
||||
"user_agent": "Mozilla/5.0...",
|
||||
"user_id": "12345" // if authenticated
|
||||
}
|
||||
```
|
||||
- **Effort**: 20 minutes (design + documentation)
|
||||
|
||||
---
|
||||
|
||||
### U3: Panic Response Details
|
||||
- **Issue**: FR-014 says "HTTP 500 with unified error response" but unclear about stack trace handling
|
||||
- **Current Impact**: Security concern - should stack traces be in production responses?
|
||||
- **Recommendation**: Clarify in spec.md FR-014:
|
||||
```
|
||||
Development/Staging: Include sanitized error message in response.msg, full stack trace in logs only
|
||||
Production: Generic error message "Internal server error", full details in logs only
|
||||
Response format: {"code": 1000, "data": null, "msg": "Internal server error"}
|
||||
```
|
||||
- **Effort**: 15 minutes (security consideration + spec update)
|
||||
|
||||
---
|
||||
|
||||
### U4: Rate Limiter Documentation Format
|
||||
- **Issue**: FR-020 mentions "documentation" but unclear where (quickstart.md? inline comments? separate docs/)
|
||||
- **Current Impact**: Documentation may be incomplete or scattered
|
||||
- **Recommendation**: Specify in spec.md FR-020:
|
||||
```
|
||||
Documentation location: quickstart.md section "Enabling Rate Limiting"
|
||||
Must include: Configuration parameters, per-endpoint setup, testing examples
|
||||
Code example: Uncomment middleware, adjust max/window, test with curl loop
|
||||
```
|
||||
- **Effort**: 10 minutes (clarify requirements)
|
||||
|
||||
---
|
||||
|
||||
### U5: Non-Writable Log Directory Behavior
|
||||
- **Issue**: Edge case says "fail to start with clear error" but no HTTP status code if started
|
||||
- **Current Impact**: Unclear behavior if directory becomes non-writable at runtime
|
||||
- **Recommendation**: Clarify in spec.md Edge Cases:
|
||||
```
|
||||
Startup: Fail immediately with exit code 1 and error message before listening on port
|
||||
Runtime: If directory becomes non-writable, log error to stderr and return 503 on health check
|
||||
```
|
||||
- **Effort**: 15 minutes (design decision + spec update)
|
||||
|
||||
---
|
||||
|
||||
### U6: Performance Test Task Missing
|
||||
- **Issue**: plan.md mentions "1000+ req/s capacity" but no performance test task in tasks.md
|
||||
- **Current Impact**: Performance goal not validated
|
||||
- **Recommendation**: Add task to tasks.md Phase 10:
|
||||
```
|
||||
- [ ] T117a [P] Load test with 1000 req/s for 60s, verify P95 < 200ms (use hey or wrk)
|
||||
```
|
||||
- **Effort**: 1 hour (implementation + infrastructure setup)
|
||||
|
||||
---
|
||||
|
||||
### I1: Logger Instances Documentation
|
||||
- **Issue**: spec.md mentions Zap but tasks reference appLogger/accessLogger instances
|
||||
- **Current Impact**: Spec doesn't explicitly require two logger instances
|
||||
- **Recommendation**: Add to spec.md FR-004 or Key Entities:
|
||||
```
|
||||
System maintains two independent Zap logger instances:
|
||||
- appLogger: For application-level logs (business logic, errors, debug)
|
||||
- accessLogger: For HTTP access logs (request/response details)
|
||||
Each instance has separate Lumberjack rotation configuration.
|
||||
```
|
||||
- **Effort**: 10 minutes (documentation clarity)
|
||||
|
||||
---
|
||||
|
||||
### I3: Fail-Closed Timing Validation
|
||||
- **Issue**: spec.md FR-016b says "immediately" but T063 doesn't validate timing (< 100ms)
|
||||
- **Current Impact**: "Immediate" is subjective, no performance assertion
|
||||
- **Recommendation**: Update tasks.md T063:
|
||||
```
|
||||
- [ ] T063 [P] [US6] Unit test for Redis unavailable (fail closed with < 100ms response time)
|
||||
```
|
||||
Or clarify spec.md: "immediately = same request cycle, no retry delays"
|
||||
- **Effort**: 5 minutes (clarify requirements OR 30 minutes add timing assertion)
|
||||
|
||||
---
|
||||
|
||||
## Low Priority Improvements
|
||||
|
||||
### D1: Response Format Duplication
|
||||
- **Issue**: FR-007 and US3 both define unified response format
|
||||
- **Current Impact**: Redundancy, potential inconsistency if one updated
|
||||
- **Recommendation**: Keep FR-007 as normative, update US3 acceptance criteria:
|
||||
```
|
||||
Change: "the response contains `{...}`"
|
||||
To: "the response follows unified format defined in FR-007"
|
||||
```
|
||||
- **Effort**: 5 minutes (reduce duplication)
|
||||
|
||||
---
|
||||
|
||||
### D2: Code Quality Task Consolidation
|
||||
- **Issue**: T096-T099 are four separate tasks that could run in one script
|
||||
- **Current Impact**: Overhead running tasks sequentially
|
||||
- **Recommendation**: Consider combining to T096-combined:
|
||||
```
|
||||
- [ ] T096 [P] Run code quality checks: gofmt -l ., go vet ./..., golangci-lint run, check doc comments
|
||||
```
|
||||
Or keep separate for granular progress tracking (current approach is also valid)
|
||||
- **Effort**: 15 minutes (script creation) OR keep as-is (no change needed)
|
||||
|
||||
---
|
||||
|
||||
### I4: Task Count Verification
|
||||
- **Issue**: plan.md says 126 tasks, tasks.md has T001-T126 (now T127 with T012a)
|
||||
- **Current Impact**: None - counts match after update
|
||||
- **Recommendation**: No action needed, already addressed in C1 fix
|
||||
- **Effort**: 0 minutes (already resolved)
|
||||
|
||||
---
|
||||
|
||||
## Priority Recommendation
|
||||
|
||||
**Before Implementation**:
|
||||
- None (all HIGH priority issues already fixed)
|
||||
|
||||
**During Implementation** (if time permits):
|
||||
- A6: Config validation rules (needed for T009 implementation)
|
||||
- U2: Access log format (needed for T050 implementation)
|
||||
- U3: Panic response details (security consideration)
|
||||
|
||||
**After Implementation** (polish phase):
|
||||
- A2, A3: Add default values to config.yaml and documentation
|
||||
- U4, U5, I1: Documentation improvements
|
||||
- U6: Performance testing (if infrastructure available)
|
||||
- D1, D2, I3: Nice-to-have optimizations
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
- **Total Optional Improvements**: 14 items
|
||||
- **Medium Priority**: 9 items (mostly documentation/specification clarity)
|
||||
- **Low Priority**: 5 items (minor optimizations, already acceptable as-is)
|
||||
- **Estimated Total Effort**: ~4 hours for all medium priority items
|
||||
- **Recommended Approach**: Address medium priority items during implementation when context is fresh
|
||||
|
||||
Reference in New Issue
Block a user