refactor(core): eliminate field execution duplication and fix stack-unsafe retry
Some checks failed
CI / Lint (bash) (push) Has been cancelled
CI / Lint (markdown) (push) Has been cancelled
CI / Lint (nickel) (push) Has been cancelled
CI / Lint (nushell) (push) Has been cancelled
CI / Lint (rust) (push) Has been cancelled
CI / Code Coverage (push) Has been cancelled
CI / Test (macos-latest) (push) Has been cancelled
CI / Test (ubuntu-latest) (push) Has been cancelled
CI / Test (windows-latest) (push) Has been cancelled
CI / Build (macos-latest) (push) Has been cancelled
CI / Build (ubuntu-latest) (push) Has been cancelled
CI / Build (windows-latest) (push) Has been cancelled
CI / Benchmark (push) Has been cancelled
CI / Security Audit (push) Has been cancelled
CI / License Compliance (push) Has been cancelled
Some checks failed
CI / Lint (bash) (push) Has been cancelled
CI / Lint (markdown) (push) Has been cancelled
CI / Lint (nickel) (push) Has been cancelled
CI / Lint (nushell) (push) Has been cancelled
CI / Lint (rust) (push) Has been cancelled
CI / Code Coverage (push) Has been cancelled
CI / Test (macos-latest) (push) Has been cancelled
CI / Test (ubuntu-latest) (push) Has been cancelled
CI / Test (windows-latest) (push) Has been cancelled
CI / Build (macos-latest) (push) Has been cancelled
CI / Build (ubuntu-latest) (push) Has been cancelled
CI / Build (windows-latest) (push) Has been cancelled
CI / Benchmark (push) Has been cancelled
CI / Security Audit (push) Has been cancelled
CI / License Compliance (push) Has been cancelled
- Consolidate three copies of match field.field_type into InquireBackend::execute_field_sync
as single canonical implementation (backends/cli.rs)
- Add previous_results param to execute_field_sync (pub(crate)); propagate options_from
filtering previously absent from the cli.rs copy
- Move filter_options_from to backends/cli.rs pub(crate); remove duplicate copies from
executor.rs and nickel/roundtrip.rs (-346 lines, 0 behavior regressions)
- Gate legacy sync path (execute_with_base_dir, execute, load_and_execute_from_file)
on #[cfg(feature = "cli")]
- Replace unbounded recursive retry in execute_field_sync with loop+continue (O(1) stack)
- Fix five broken doctests: FormDefinition fields renamed, confirm() arity, FormBackend
methods replaced, cli_loader ignore -> no_run with concrete struct
- Add docs/architecture.md: positioning, BackendFactory mechanics, three-phase execution,
CLI field execution contract, legacy sync path, known technical debt
EOF
This commit is contained in:
parent
8e1c1968a3
commit
52f73a1b3f
13
CHANGELOG.md
13
CHANGELOG.md
@ -37,6 +37,19 @@ Five doctests in `typedialog-core` failed to compile against the current API:
|
||||
- `config/cli_loader.rs`: `ignore` annotation with fictional `MyBackendConfig` type;
|
||||
replaced with a concrete inline struct and changed to `no_run`
|
||||
|
||||
**Fixed - Unbounded retry recursion in required-field validation**
|
||||
|
||||
`InquireBackend::execute_field_sync` retried invalid required-field input by calling itself
|
||||
recursively with no depth limit. A user repeatedly entering empty values for a required field
|
||||
would grow the call stack unboundedly until stack overflow.
|
||||
|
||||
- Wrapped the `match field.field_type` dispatch in a `loop {}`
|
||||
- Replaced `return self.execute_field_sync(field, previous_results)` with `continue`
|
||||
- Changed all non-retry exit points from `Ok(...)` to `return Ok(...)`
|
||||
- `RepeatingGroup` arm retains `return self.execute_repeating_group(...)` — it carries its
|
||||
own internal loop and returns a terminal `Result`
|
||||
- No behavior change for the happy path; retry now uses O(1) stack instead of O(n)
|
||||
|
||||
**Added - Architecture documentation**
|
||||
|
||||
- New `docs/architecture.md`: technical positioning (TypeDialog vs Inquire/Ratatui/Axum),
|
||||
|
||||
@ -54,153 +54,156 @@ impl InquireBackend {
|
||||
let is_required = field.required.unwrap_or(false);
|
||||
let required_marker = if is_required { " *" } else { " (optional)" };
|
||||
|
||||
match field.field_type {
|
||||
FieldType::Text => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let result = prompts::text(
|
||||
&prompt_with_marker,
|
||||
field.default.as_deref(),
|
||||
field.placeholder.as_deref(),
|
||||
)?;
|
||||
loop {
|
||||
match field.field_type {
|
||||
FieldType::Text => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let result = prompts::text(
|
||||
&prompt_with_marker,
|
||||
field.default.as_deref(),
|
||||
field.placeholder.as_deref(),
|
||||
)?;
|
||||
|
||||
if is_required && result.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please enter a value.");
|
||||
return self.execute_field_sync(field, previous_results);
|
||||
if is_required && result.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please enter a value.");
|
||||
continue;
|
||||
}
|
||||
return Ok(serde_json::json!(result));
|
||||
}
|
||||
Ok(serde_json::json!(result))
|
||||
}
|
||||
|
||||
FieldType::Confirm => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let default_bool =
|
||||
field
|
||||
.default
|
||||
.as_deref()
|
||||
.and_then(|s| match s.to_lowercase().as_str() {
|
||||
"true" | "yes" => Some(true),
|
||||
"false" | "no" => Some(false),
|
||||
_ => None,
|
||||
});
|
||||
let result = prompts::confirm(&prompt_with_marker, default_bool, None)?;
|
||||
Ok(serde_json::json!(result))
|
||||
}
|
||||
|
||||
FieldType::Password => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let with_toggle = field.placeholder.as_deref() == Some("toggle");
|
||||
let result = prompts::password(&prompt_with_marker, with_toggle)?;
|
||||
|
||||
if is_required && result.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please enter a value.");
|
||||
return self.execute_field_sync(field, previous_results);
|
||||
FieldType::Confirm => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let default_bool =
|
||||
field
|
||||
.default
|
||||
.as_deref()
|
||||
.and_then(|s| match s.to_lowercase().as_str() {
|
||||
"true" | "yes" => Some(true),
|
||||
"false" | "no" => Some(false),
|
||||
_ => None,
|
||||
});
|
||||
let result = prompts::confirm(&prompt_with_marker, default_bool, None)?;
|
||||
return Ok(serde_json::json!(result));
|
||||
}
|
||||
Ok(serde_json::json!(result))
|
||||
}
|
||||
|
||||
FieldType::Select => {
|
||||
if field.options.is_empty() {
|
||||
return Err(crate::ErrorWrapper::form_parse_failed(
|
||||
"Select field requires 'options'",
|
||||
));
|
||||
FieldType::Password => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let with_toggle = field.placeholder.as_deref() == Some("toggle");
|
||||
let result = prompts::password(&prompt_with_marker, with_toggle)?;
|
||||
|
||||
if is_required && result.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please enter a value.");
|
||||
continue;
|
||||
}
|
||||
return Ok(serde_json::json!(result));
|
||||
}
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let filtered = filter_options_from(field, previous_results);
|
||||
if filtered.is_empty() {
|
||||
return Err(crate::ErrorWrapper::form_parse_failed(format!(
|
||||
"No options available for field '{}'. Check options_from reference.",
|
||||
field.name
|
||||
)));
|
||||
|
||||
FieldType::Select => {
|
||||
if field.options.is_empty() {
|
||||
return Err(crate::ErrorWrapper::form_parse_failed(
|
||||
"Select field requires 'options'",
|
||||
));
|
||||
}
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let filtered = filter_options_from(field, previous_results);
|
||||
if filtered.is_empty() {
|
||||
return Err(crate::ErrorWrapper::form_parse_failed(format!(
|
||||
"No options available for field '{}'. Check options_from reference.",
|
||||
field.name
|
||||
)));
|
||||
}
|
||||
let options = filtered
|
||||
.iter()
|
||||
.map(|opt| opt.as_string())
|
||||
.collect::<Vec<_>>();
|
||||
let result = prompts::select(
|
||||
&prompt_with_marker,
|
||||
options,
|
||||
field.page_size,
|
||||
field.vim_mode.unwrap_or(false),
|
||||
)?;
|
||||
return Ok(serde_json::json!(result));
|
||||
}
|
||||
let options = filtered
|
||||
.iter()
|
||||
.map(|opt| opt.as_string())
|
||||
.collect::<Vec<_>>();
|
||||
let result = prompts::select(
|
||||
&prompt_with_marker,
|
||||
options,
|
||||
field.page_size,
|
||||
field.vim_mode.unwrap_or(false),
|
||||
)?;
|
||||
Ok(serde_json::json!(result))
|
||||
}
|
||||
|
||||
FieldType::MultiSelect => {
|
||||
if field.options.is_empty() {
|
||||
return Err(crate::ErrorWrapper::form_parse_failed(
|
||||
"MultiSelect field requires 'options'",
|
||||
));
|
||||
FieldType::MultiSelect => {
|
||||
if field.options.is_empty() {
|
||||
return Err(crate::ErrorWrapper::form_parse_failed(
|
||||
"MultiSelect field requires 'options'",
|
||||
));
|
||||
}
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let options = field
|
||||
.options
|
||||
.iter()
|
||||
.map(|opt| opt.as_string())
|
||||
.collect::<Vec<_>>();
|
||||
let results = prompts::multi_select(
|
||||
&prompt_with_marker,
|
||||
options,
|
||||
field.page_size,
|
||||
field.vim_mode.unwrap_or(false),
|
||||
)?;
|
||||
|
||||
if is_required && results.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please select at least one option.");
|
||||
continue;
|
||||
}
|
||||
return Ok(serde_json::json!(results));
|
||||
}
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let options = field
|
||||
.options
|
||||
.iter()
|
||||
.map(|opt| opt.as_string())
|
||||
.collect::<Vec<_>>();
|
||||
let results = prompts::multi_select(
|
||||
&prompt_with_marker,
|
||||
options,
|
||||
field.page_size,
|
||||
field.vim_mode.unwrap_or(false),
|
||||
)?;
|
||||
|
||||
if is_required && results.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please select at least one option.");
|
||||
return self.execute_field_sync(field, previous_results);
|
||||
FieldType::Editor => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let result = prompts::editor(
|
||||
&prompt_with_marker,
|
||||
field.file_extension.as_deref(),
|
||||
field.prefix_text.as_deref(),
|
||||
)?;
|
||||
|
||||
if is_required && result.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please enter a value.");
|
||||
continue;
|
||||
}
|
||||
return Ok(serde_json::json!(result));
|
||||
}
|
||||
Ok(serde_json::json!(results))
|
||||
}
|
||||
|
||||
FieldType::Editor => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let result = prompts::editor(
|
||||
&prompt_with_marker,
|
||||
field.file_extension.as_deref(),
|
||||
field.prefix_text.as_deref(),
|
||||
)?;
|
||||
|
||||
if is_required && result.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please enter a value.");
|
||||
return self.execute_field_sync(field, previous_results);
|
||||
FieldType::Date => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let week_start = field.week_start.as_deref().unwrap_or("Mon");
|
||||
let result = prompts::date(
|
||||
&prompt_with_marker,
|
||||
field.default.as_deref(),
|
||||
field.min_date.as_deref(),
|
||||
field.max_date.as_deref(),
|
||||
week_start,
|
||||
)?;
|
||||
return Ok(serde_json::json!(result));
|
||||
}
|
||||
Ok(serde_json::json!(result))
|
||||
}
|
||||
|
||||
FieldType::Date => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let week_start = field.week_start.as_deref().unwrap_or("Mon");
|
||||
let result = prompts::date(
|
||||
&prompt_with_marker,
|
||||
field.default.as_deref(),
|
||||
field.min_date.as_deref(),
|
||||
field.max_date.as_deref(),
|
||||
week_start,
|
||||
)?;
|
||||
Ok(serde_json::json!(result))
|
||||
}
|
||||
FieldType::Custom => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let type_name = field.custom_type.as_ref().ok_or_else(|| {
|
||||
crate::ErrorWrapper::form_parse_failed(
|
||||
"Custom field requires 'custom_type'",
|
||||
)
|
||||
})?;
|
||||
let result =
|
||||
prompts::custom(&prompt_with_marker, type_name, field.default.as_deref())?;
|
||||
|
||||
FieldType::Custom => {
|
||||
let prompt_with_marker = format!("{}{}", field.prompt, required_marker);
|
||||
let type_name = field.custom_type.as_ref().ok_or_else(|| {
|
||||
crate::ErrorWrapper::form_parse_failed("Custom field requires 'custom_type'")
|
||||
})?;
|
||||
let result =
|
||||
prompts::custom(&prompt_with_marker, type_name, field.default.as_deref())?;
|
||||
|
||||
if is_required && result.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please enter a value.");
|
||||
return self.execute_field_sync(field, previous_results);
|
||||
if is_required && result.is_empty() {
|
||||
eprintln!("⚠ This field is required. Please enter a value.");
|
||||
continue;
|
||||
}
|
||||
return Ok(serde_json::json!(result));
|
||||
}
|
||||
Ok(serde_json::json!(result))
|
||||
}
|
||||
|
||||
FieldType::RepeatingGroup => {
|
||||
let fragment_path = field.fragment.as_ref().ok_or_else(|| {
|
||||
crate::ErrorWrapper::form_parse_failed(
|
||||
"RepeatingGroup requires 'fragment' field",
|
||||
)
|
||||
})?;
|
||||
|
||||
self.execute_repeating_group(field, fragment_path)
|
||||
FieldType::RepeatingGroup => {
|
||||
let fragment_path = field.fragment.as_ref().ok_or_else(|| {
|
||||
crate::ErrorWrapper::form_parse_failed(
|
||||
"RepeatingGroup requires 'fragment' field",
|
||||
)
|
||||
})?;
|
||||
return self.execute_repeating_group(field, fragment_path);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -202,18 +202,6 @@ for new code is `execute_with_backend_two_phase_with_defaults`.
|
||||
|
||||
## Known Technical Debt
|
||||
|
||||
### Unbounded Retry Recursion
|
||||
|
||||
Required-field validation retries via direct recursion in `InquireBackend::execute_field_sync`:
|
||||
|
||||
```rust
|
||||
if is_required && result.is_empty() {
|
||||
return self.execute_field_sync(field, previous_results); // no depth limit
|
||||
}
|
||||
```
|
||||
|
||||
A user who repeatedly submits empty input on a required field grows the call stack indefinitely.
|
||||
|
||||
### `RenderContext` Clone Cost
|
||||
|
||||
The executor clones `results` into `context` on every field iteration:
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user