|
| 1 | +# Decision: Command State Return Pattern |
| 2 | + |
| 3 | +## Status |
| 4 | + |
| 5 | +Accepted |
| 6 | + |
| 7 | +## Date |
| 8 | + |
| 9 | +2025-10-03 |
| 10 | + |
| 11 | +## Context |
| 12 | + |
| 13 | +In Phase 5 of the environment state management feature, we need to integrate type-safe state transitions into our command handlers (`ProvisionCommand` and `ConfigureCommand`). This raises a fundamental architectural question: **Should commands return the transformed `Environment<S>` state, or should they operate as pure command handlers that only persist state via the repository?** |
| 14 | + |
| 15 | +### The Problem |
| 16 | + |
| 17 | +We have two competing patterns: |
| 18 | + |
| 19 | +1. **Typed State Returns**: Commands accept `Environment<S>` and return `Environment<T>` |
| 20 | + |
| 21 | + - Example: `ProvisionCommand::execute(Environment<Created>) -> Result<Environment<Provisioned>, Error>` |
| 22 | + |
| 23 | +2. **Pure Command Handler**: Commands accept `Environment<S>`, persist state internally, return void |
| 24 | + - Example: `ProvisionCommand::execute(Environment<Created>) -> Result<(), Error>` |
| 25 | + |
| 26 | +Both patterns can work, but they have different implications for type safety, data flow, and future extensibility. |
| 27 | + |
| 28 | +### Why This Matters |
| 29 | + |
| 30 | +- We've invested 4 phases of work building a sophisticated type-state pattern for compile-time state validation |
| 31 | +- Commands orchestrate complex multi-step workflows with clear state progressions |
| 32 | +- We want to enable future command chaining and orchestration with compile-time guarantees |
| 33 | +- The repository layer exists for persistence, not as the primary data flow mechanism |
| 34 | + |
| 35 | +## Decision |
| 36 | + |
| 37 | +**We will use typed state returns**: Commands accept and return strongly-typed `Environment<S>` states. |
| 38 | + |
| 39 | +```rust |
| 40 | +impl ProvisionCommand { |
| 41 | + pub async fn execute( |
| 42 | + &self, |
| 43 | + environment: Environment<Created>, |
| 44 | + ) -> Result<Environment<Provisioned>, ProvisionCommandError> { |
| 45 | + // Transition to intermediate state |
| 46 | + let environment = environment.start_provisioning(); |
| 47 | + self.persist_state(&environment)?; // Persistence is secondary |
| 48 | + |
| 49 | + // Execute provisioning steps... |
| 50 | + let provisioned = self.execute_steps(&environment).await?; |
| 51 | + |
| 52 | + // Persist final state |
| 53 | + self.persist_state(&provisioned)?; |
| 54 | + |
| 55 | + // Return transformed state |
| 56 | + Ok(provisioned) |
| 57 | + } |
| 58 | +} |
| 59 | +``` |
| 60 | + |
| 61 | +### Key Principles |
| 62 | + |
| 63 | +1. **Commands are state transformations**: `Environment<S>` → `Environment<T>` |
| 64 | +2. **Repository is for persistence**: Save/load state, but not primary data flow |
| 65 | +3. **Type safety is paramount**: Leverage compile-time guarantees from type-state pattern |
| 66 | +4. **Data flow is explicit**: Input → transform → output (no hidden state) |
| 67 | + |
| 68 | +## Consequences |
| 69 | + |
| 70 | +### Positive |
| 71 | + |
| 72 | +✅ **Compile-Time Safety**: Invalid state transitions are prevented at compile time |
| 73 | + |
| 74 | +- Cannot call `ConfigureCommand` on an `Environment<Created>` (not yet provisioned) |
| 75 | +- Cannot call `ProvisionCommand` on an already `Environment<Provisioned>` instance |
| 76 | +- Impossible to forget a state transition |
| 77 | + |
| 78 | +✅ **Clear Data Flow**: Easy to understand what's happening |
| 79 | + |
| 80 | +```rust |
| 81 | +let created = Environment::new(...); |
| 82 | +let provisioned = provision_command.execute(created).await?; |
| 83 | +let configured = configure_command.execute(provisioned).await?; |
| 84 | +``` |
| 85 | + |
| 86 | +✅ **No Repeated Parsing**: Avoid pattern matching on `AnyEnvironmentState` |
| 87 | + |
| 88 | +- Without typed returns: Load from repository → match on `AnyEnvironmentState` → extract typed state |
| 89 | +- With typed returns: State already typed, no parsing needed |
| 90 | + |
| 91 | +✅ **Future Orchestration**: Enables fluent command chaining |
| 92 | + |
| 93 | +```rust |
| 94 | +// Future possibility: |
| 95 | +let workflow = Workflow::new() |
| 96 | + .then(provision_command) |
| 97 | + .then(configure_command) |
| 98 | + .then(deploy_command); |
| 99 | + |
| 100 | +let final_state = workflow.execute(created).await?; |
| 101 | +``` |
| 102 | + |
| 103 | +✅ **Type-State Pattern Reaches Full Potential**: Commands leverage all the work from Phases 1-4 |
| 104 | + |
| 105 | +### Negative |
| 106 | + |
| 107 | +⚠️ **Deviates from Pure CQS**: Commands traditionally shouldn't return values in strict Command/Query Separation |
| 108 | + |
| 109 | +- However, CQRS patterns allow commands to return acknowledgments/identifiers |
| 110 | +- State transformation is a valid command output in functional paradigms |
| 111 | + |
| 112 | +⚠️ **Commands Return Values**: Not traditional "fire and forget" command handlers |
| 113 | + |
| 114 | +- However, this is intentional - we want the transformed state for chaining |
| 115 | + |
| 116 | +### Neutral |
| 117 | + |
| 118 | +ℹ️ **Repository is Secondary**: State persistence happens alongside transformation |
| 119 | + |
| 120 | +- This is by design - persistence is a cross-cutting concern, not the primary data flow |
| 121 | +- Failed persistence is logged but doesn't fail the command (state is still valid in memory) |
| 122 | + |
| 123 | +## Alternatives Considered |
| 124 | + |
| 125 | +### Alternative 1: Pure Command Handler Pattern |
| 126 | + |
| 127 | +```rust |
| 128 | +impl ProvisionCommand { |
| 129 | + pub async fn execute( |
| 130 | + &self, |
| 131 | + environment: Environment<Created>, |
| 132 | + ) -> Result<(), ProvisionCommandError> { |
| 133 | + let environment = environment.start_provisioning(); |
| 134 | + self.repository.save(&environment.into_any())?; |
| 135 | + |
| 136 | + // Execute steps... |
| 137 | + |
| 138 | + let provisioned = environment.complete_provisioning(ip); |
| 139 | + self.repository.save(&provisioned.into_any())?; |
| 140 | + |
| 141 | + // No return - caller must load from repository |
| 142 | + } |
| 143 | +} |
| 144 | + |
| 145 | +// Caller must load state |
| 146 | +provision_command.execute(created).await?; |
| 147 | +let state = repository.load(&env_name)?.expect("Must exist"); |
| 148 | +let provisioned = match state { |
| 149 | + AnyEnvironmentState::Provisioned(env) => env, |
| 150 | + _ => return Err("Wrong state!"), // Runtime error! |
| 151 | +}; |
| 152 | +``` |
| 153 | + |
| 154 | +**Why Rejected**: |
| 155 | + |
| 156 | +- ❌ Loses compile-time type safety (runtime pattern matching required) |
| 157 | +- ❌ Awkward data flow (caller must reload what command just created) |
| 158 | +- ❌ Repository becomes central to data flow (not just persistence) |
| 159 | +- ❌ Makes command chaining difficult |
| 160 | +- ❌ Doesn't leverage the type-state pattern we built in Phases 1-4 |
| 161 | + |
| 162 | +### Alternative 2: Hybrid - Store Environment in Command |
| 163 | + |
| 164 | +```rust |
| 165 | +pub struct ProvisionCommand { |
| 166 | + environment: RefCell<Option<Environment<Provisioning>>>, |
| 167 | + // ... |
| 168 | +} |
| 169 | + |
| 170 | +impl ProvisionCommand { |
| 171 | + pub async fn execute(&self, environment: Environment<Created>) -> Result<(), Error> { |
| 172 | + let provisioning = environment.start_provisioning(); |
| 173 | + *self.environment.borrow_mut() = Some(provisioning); |
| 174 | + |
| 175 | + // Execute... |
| 176 | + |
| 177 | + let provisioned = self.environment.borrow().as_ref().unwrap().complete_provisioning(ip); |
| 178 | + self.repository.save(&provisioned.into_any())?; |
| 179 | + } |
| 180 | + |
| 181 | + pub fn get_result(&self) -> Environment<Provisioned> { |
| 182 | + // Complex extraction logic... |
| 183 | + } |
| 184 | +} |
| 185 | +``` |
| 186 | + |
| 187 | +**Why Rejected**: |
| 188 | + |
| 189 | +- ❌ Interior mutability complexity (`RefCell`, borrowing rules) |
| 190 | +- ❌ Unclear ownership semantics |
| 191 | +- ❌ Still requires separate getter method |
| 192 | +- ❌ Makes command non-`Send` (problematic for async) |
| 193 | +- ❌ More complex than straightforward transformation |
| 194 | + |
| 195 | +### Alternative 3: Builder Pattern with Fluent API |
| 196 | + |
| 197 | +```rust |
| 198 | +provision_command |
| 199 | + .with_environment(created) |
| 200 | + .execute() |
| 201 | + .await? |
| 202 | + .get_provisioned_environment(); |
| 203 | +``` |
| 204 | + |
| 205 | +**Why Rejected**: |
| 206 | + |
| 207 | +- ❌ More complex API than direct transformation |
| 208 | +- ❌ Still needs to return state somehow |
| 209 | +- ❌ Doesn't solve the fundamental return question |
| 210 | + |
| 211 | +## Related Decisions |
| 212 | + |
| 213 | +- [Type Erasure for Environment States](./type-erasure-for-environment-states.md) - How we handle serialization while maintaining type safety |
| 214 | +- [Actionable Error Messages](./actionable-error-messages.md) - Error handling approach for commands |
| 215 | +- Phase 1-4 implementation of type-state pattern in `Environment<S>` |
| 216 | + |
| 217 | +## References |
| 218 | + |
| 219 | +- **Type-State Pattern in Rust**: <https://cliffle.com/blog/rust-typestate/> |
| 220 | + |
| 221 | + - Demonstrates how to use Rust's type system for state machines |
| 222 | + - Our pattern follows this approach for environment lifecycle |
| 223 | + |
| 224 | +- **CQRS Flexibility**: <https://martinfowler.com/bliki/CQRS.html> |
| 225 | + |
| 226 | + - While pure CQS says commands return void, CQRS patterns often return acknowledgments |
| 227 | + - Command can return identifiers or confirmation objects |
| 228 | + |
| 229 | +- **Functional Programming Perspective**: |
| 230 | + |
| 231 | + - State transitions as pure transformations: `S -> T` |
| 232 | + - Commands as functions that transform state |
| 233 | + - Side effects (persistence) are secondary concerns |
| 234 | + |
| 235 | +- **Rust Ownership Model**: |
| 236 | + |
| 237 | + - Returning transformed data is idiomatic in Rust |
| 238 | + - Ownership transfer makes data flow explicit |
| 239 | + - No implicit state mutations |
| 240 | + |
| 241 | +- **Phase 1-4 Implementation**: |
| 242 | + - `docs/features/environment-state-management/feature-description.md` |
| 243 | + - `src/domain/environment/mod.rs` - Type-state implementation |
| 244 | + - `src/infrastructure/persistence/` - Repository layer |
| 245 | + |
| 246 | +## Implementation Notes |
| 247 | + |
| 248 | +### Persistence Error Handling |
| 249 | + |
| 250 | +Persistence failures are logged but don't fail the command: |
| 251 | + |
| 252 | +```rust |
| 253 | +if let Err(e) = self.persist_state(&environment) { |
| 254 | + warn!( |
| 255 | + "Failed to persist state: {}. Command execution continues.", |
| 256 | + e |
| 257 | + ); |
| 258 | +} |
| 259 | +``` |
| 260 | + |
| 261 | +**Rationale**: The in-memory state transformation is valid even if persistence fails. We log for observability but don't block the workflow. |
| 262 | + |
| 263 | +### Command Chaining Pattern |
| 264 | + |
| 265 | +This decision enables future orchestration: |
| 266 | + |
| 267 | +```rust |
| 268 | +// Phase 5 Subtasks 3-4: Individual commands return typed states |
| 269 | +let provisioned = provision_cmd.execute(created).await?; |
| 270 | +let configured = configure_cmd.execute(provisioned).await?; |
| 271 | + |
| 272 | +// Future: Orchestration layer with compile-time guarantees |
| 273 | +let workflow = Orchestrator::new() |
| 274 | + .step(provision_cmd) // Requires Created, produces Provisioned |
| 275 | + .step(configure_cmd) // Requires Provisioned, produces Configured |
| 276 | + .step(deploy_cmd); // Requires Configured, produces Deployed |
| 277 | + |
| 278 | +workflow.execute(created).await?; // Type-checked at compile time |
| 279 | +``` |
| 280 | + |
| 281 | +### Backward Compatibility |
| 282 | + |
| 283 | +Commands still work with existing E2E tests by extracting values from returned states: |
| 284 | + |
| 285 | +```rust |
| 286 | +// Old pattern (Phase 5 Subtask 1): |
| 287 | +let ip_address = provision_command.execute(&ssh_credentials).await?; |
| 288 | + |
| 289 | +// New pattern (Phase 5 Subtask 3+): |
| 290 | +let provisioned = provision_command.execute(environment).await?; |
| 291 | +let ip_address = provisioned.instance_ip(); // Getter method |
| 292 | +``` |
0 commit comments