refactor: command handlers refactoring (Issue #61)#62
Merged
Conversation
- Create common/failure_context module with build_base_failure_context helper - Helper centralizes timing calculation, trace ID generation, and base context building - Update provision, configure, and destroy handlers to use the common helper - Eliminate ~60 lines of duplicated code across three handlers - Add 4 unit tests covering timing, trace ID uniqueness, zero duration, and error summary - Simplify return type to just BaseFailureContext (removed unused tuple values) - All handlers maintain their handler-specific trace file generation logic - Zero behavior changes - only code organization improvements Relates to command-handlers-refactoring.md Proposal #0
Create TypedEnvironmentRepository wrapper to eliminate duplicated persistence logic across command handlers. ## Changes **Domain Layer (src/domain/environment/repository.rs)**: - Add TypedEnvironmentRepository wrapper struct - Implement state-specific save methods using macro (15 states) - Add debug logging to all persistence operations - Provide inner() accessor for load/delete/list operations **Application Layer - Command Handlers**: - Update ProvisionCommandHandler to use TypedEnvironmentRepository - Update ConfigureCommandHandler to use TypedEnvironmentRepository - Update DestroyCommandHandler to use TypedEnvironmentRepository - Replace 9 instances of `.clone().into_any()` with typed save methods **Tests**: - Fix Arc strong_count test to use repository.inner() - All 991 unit tests passing - All E2E tests passing ## Implementation Details Originally planned a simple helper function but discovered ergonomic issues requiring verbose turbofish syntax. Instead implemented a wrapper repository following adapter pattern. **Key Technical Decisions**: 1. Domain vs Application Layer: Moved to domain layer (repository concerns) 2. Macro vs Manual: Used macro to generate 15 state-specific methods 3. Wrapper vs Trait: Wrapper pattern simpler and more maintainable 4. Logging Location: Repository methods (not application layer) **API Improvement**: - Before: `self.repository.save(&environment.clone().into_any())?` - After: `self.repository.save_provisioning(&environment)?` ## Benefits - ✅ Eliminated 27 lines of duplicated conversion code (9 calls × 3 lines) - ✅ Type-safe API with compiler-enforced state correctness - ✅ Clean syntax without turbofish type annotations - ✅ Built-in logging for all persistence operations - ✅ Better DDD alignment (conversion is repository responsibility) - ✅ Easier to extend with typed load methods in future ## Refactoring Progress - Completed: 2/7 proposals (Proposal #0 and #1) - Phase 0 (Quick Wins): 66% complete (2/3) See docs/refactors/plans/command-handlers-refactoring.md for details.
…stroyed Problem: - TestContext::drop was always attempting cleanup via emergency_destroy - This caused spurious warnings even when DestroyCommand already cleaned up - Error: 'No such file or directory (os error 2)' appeared at test end - Cleanup should be smart enough to detect when infrastructure already gone Solution: - Check environment state before attempting cleanup in Drop - Skip cleanup if environment already in Destroyed state - Use matches! macro to check AnyEnvironmentState::Destroyed variant - Prevents unnecessary cleanup attempts and spurious error messages Changes: - src/testing/e2e/context.rs: Added state check in Drop implementation * Early return if environment is already Destroyed * Eliminates unnecessary emergency_destroy calls * No warnings when DestroyCommand completes successfully - project-words.txt: Added 'turbofish' to dictionary * Technical Rust term used in Proposal #1 documentation * Fixes spell check failures Benefits: - Cleaner test output without spurious warnings - More intelligent cleanup strategy - Better user experience - no confusing error messages - Aligns with principle: tests should be smart about their cleanup Testing: - All 991 unit tests pass - E2E tests complete without cleanup warnings - Verified state transitions work correctly
- Standardized field naming: all handlers now use 'environment' field (not 'environment_name') - Simplified create handler logging: removed verbose step-by-step logs, now logs only start/completion like other handlers - Added comprehensive 'Command Handler Logging Patterns' section to docs/contributing/logging-guide.md - Documented standard pattern with examples for provision, configure, destroy, and create handlers - Documented anti-patterns to avoid and key principles for consistent logging - Updated refactoring plan to mark Proposal #5 as completed (Phase 2 now 50% complete)
- Reordered methods in destroy handler to follow module organization conventions - Moved pub(crate) methods (should_destroy_infrastructure, cleanup_state_files) before private methods - All handlers now follow consistent ordering: public API, pub(crate) helpers, private methods - Updated refactoring plan to mark Proposal #6 as completed - All 7 active proposals now complete (Phase 0: 100%, Phase 1: 100%, Phase 2: 100%)
Member
Author
|
ACK 29c3d8d |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Overview
This PR completes a comprehensive refactoring of the command handlers codebase to improve maintainability, reduce code duplication, and standardize patterns across all handlers.
Closes #61
📋 Changes Summary
✅ Completed Proposals (7/7)
Phase 0 - Quick Wins (High Impact, Low Effort)
build_failure_contextlogic into shared helperTypedEnvironmentRepositorywrapper - Created wrapper for type-safe environment state persistenceStepResulttype alias - Simplified step execution return types.help()methods to all error types for detailed troubleshootingPhase 1 - Structural Improvements (High Impact, Medium Effort)
pub(crate)test exposure - Made internal methods private, deleted tests that tested implementation detailsPhase 2 - Consistency & Polish (Medium Impact, Low Effort)
🔧 Key Improvements
Code Quality
TypedEnvironmentRepositoryDeveloper Experience
.help()methodsDocumentation
🧪 Testing
All quality gates passed:
Test Coverage
📊 Impact
Before
build_failure_contextacross 3 handlers (~90% identical code)pub(crate)for testingAfter
build_failure_contexthelperenvironmentnotenvironment_name)🔄 Migration Notes
No breaking changes - this is a pure refactoring. All public APIs remain unchanged.
✅ Pre-commit Verification
Result: All checks passed (4m 48s)
📝 Commits
refactor: [#61] extract common failure context builder (Proposal #0)refactor: [#61] add TypedEnvironmentRepository wrapper (Proposal #1)refactor: [#61] add StepResult type alias (Proposal #2)refactor: [#61] add error help methods (Proposal #3)refactor: [#61] remove pub(crate) test exposure (Proposal #4)refactor: [#61] standardize logging patterns (Proposal #5)refactor: [#61] standardize method ordering (Proposal #6)🎉 Summary
This refactoring successfully:
Ready for review and merge! 🚀