feat: add JSON output to register command (12.13) #396#397
Conversation
- Add RegisterDetailsData DTO with environment_name, instance_ip, ssh_port, registered fields - Add JsonView and TextView strategy renderers for register command - Add output_format parameter to RegisterCommandController::execute() - Update complete_workflow() to use Environment<Provisioned> and dispatch to correct view - Pass output_format from dispatch router to register controller - All linters pass, 431 tests pass
|
ACK 10d304a |
There was a problem hiding this comment.
Pull request overview
Adds JSON output support to the register CLI command as part of the ongoing “JSON output for all commands” epic, using the same view/DTO strategy pattern used by other commands.
Changes:
- Introduces
RegisterDetailsDataDTO plusTextViewandJsonViewrenderers forregister. - Updates
RegisterCommandController::execute()to acceptoutput_formatand dispatch rendering accordingly. - Updates the CLI router to pass
context.output_format()into the register controller.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/presentation/cli/views/commands/register/mod.rs | Adds the register views module root and re-exports DTO/views. |
| src/presentation/cli/views/commands/register/view_data/register_details.rs | Adds RegisterDetailsData DTO + constructor from Environment<Provisioned>. |
| src/presentation/cli/views/commands/register/views/json_view.rs | Adds JSON rendering for register output. |
| src/presentation/cli/views/commands/register/views/text_view.rs | Adds text rendering to preserve prior register output format. |
| src/presentation/cli/views/commands/mod.rs | Exposes the new register views module. |
| src/presentation/cli/controllers/register/handler.rs | Adds output_format to execute() and renders via TextView/JsonView. |
| src/presentation/cli/dispatch/router.rs | Passes context.output_format() into the register controller. |
| environment_name: env.name().to_string(), | ||
| instance_ip: env | ||
| .instance_ip() | ||
| .map_or_else(String::new, |ip| ip.to_string()), |
There was a problem hiding this comment.
instance_ip() is optional, but here it’s converted to an empty string when missing. For the register success path the IP should be present (provided by the user and set during the Created→Provisioned transition), and emitting "" can silently violate the expected JSON contract. Consider treating None as an internal invariant violation (e.g., expect with a clear message) or representing absence explicitly in the DTO (e.g., Option<IpAddr>/Option<String>) and reflecting that in the JSON view.
| .map_or_else(String::new, |ip| ip.to_string()), | |
| .expect("Provisioned environment must have an instance IP on register success path") | |
| .to_string(), |
| // The constructor only produces RegisterDetailsData on the success path. | ||
| // We verify the field directly since we cannot construct Environment<Provisioned> | ||
| // without full infrastructure in a unit test. | ||
| let data = RegisterDetailsData { |
There was a problem hiding this comment.
These tests don’t cover RegisterDetailsData::from_environment(), and the comment suggests Environment<Provisioned> can’t be constructed in a unit test. The domain state machine provides Environment<Created>::register(ip) -> Environment<Provisioned>, so it should be possible to build a minimal environment and assert the DTO mapping (env name, instance_ip, ssh_port, registered) is correct.
| format!( | ||
| r#"{{ | ||
| "error": "Failed to serialize register details", | ||
| "message": "{e}" | ||
| }}"# | ||
| ) |
There was a problem hiding this comment.
The unwrap_or_else fallback interpolates the error {e} directly into a JSON string. If the message contains quotes, backslashes, or newlines, this fallback can produce invalid JSON. To guarantee valid JSON even on the error path, build the error object with serde_json (e.g., serde_json::json! + to_string_pretty) so the message is properly escaped.
| format!( | |
| r#"{{ | |
| "error": "Failed to serialize register details", | |
| "message": "{e}" | |
| }}"# | |
| ) | |
| serde_json::to_string_pretty(&serde_json::json!({ | |
| "error": "Failed to serialize register details", | |
| "message": e.to_string(), | |
| })) | |
| .unwrap_or_else(|_| { | |
| r#"{ | |
| "error": "Failed to serialize error message" | |
| }"# | |
| .to_string() | |
| }) |
…alid JSON
Replace unsafe format!() string interpolation of error messages with
serde_json::json! construction in all JsonView::render() error fallbacks.
The previous format!(r#"{{ "message": "{e}" }}"#) approach would
produce invalid JSON if the error message contained quotes, backslashes,
or newlines. Using serde_json::json! ensures the message is always
properly escaped.
Applies to 11 json_view.rs files:
- configure, release, render, destroy, purge, test, register, validate,
list, run, show
Reported by Copilot review on PR #397.
Description
Implements JSON output support for the
registercommand, closing #396.Follows the same Strategy Pattern used for all previous commands in epic #348.
Changes
New files
src/presentation/cli/views/commands/register/mod.rs— module root with re-exportssrc/presentation/cli/views/commands/register/view_data/register_details.rs—RegisterDetailsDataDTOsrc/presentation/cli/views/commands/register/views/json_view.rs— JSON renderersrc/presentation/cli/views/commands/register/views/text_view.rs— text rendererModified files
src/presentation/cli/views/commands/mod.rs— addedpub mod registersrc/presentation/cli/controllers/register/handler.rs—execute()now takesoutput_format: OutputFormat;complete_workflow()takes&Environment<Provisioned>and dispatches to the correct viewsrc/presentation/cli/dispatch/router.rs— passescontext.output_format()toexecute()JSON Output Format
{ "environment_name": "my-env", "instance_ip": "192.168.1.100", "ssh_port": 22, "registered": true }Testing
cargo run --bin linter all)Related
registercommand (12.13) #396