fix(output): reject unknown --fields names instead of rendering empty rows by jpage-godaddy · Pull Request #108 · godaddy/cli-engine

jpage-godaddy · 2026-08-28T21:37:19Z

Summary

--fields silently dropped names that don't exist in the response data, rendering rows with no data instead of a clear error (e.g. gddy api search "catalog" --fields OPERATIONID,Description, where the actual field is operationId).
apply_pipeline now validates an explicit --fields against the response data's actual top-level keys before projecting, and rejects unknown names with a "did you mean" suggestion plus the list of valid fields. Validation is skipped when the data's shape gives no signal (empty list, scalar, non-object array items), matching filter_fields's existing no-op behavior for those shapes.
--fields is parsed once (a FieldTree) and reused for both validation and projection, instead of parsing/traversing the data twice.
Validation only runs for an explicit --fields flag, never for a command's default_fields fallback — default_fields is author-controlled and applied to every invocation, so a legitimately optional field absent from one particular response must not hard-error that command for everyone. Distinguishing the two required a new GlobalFlags::fields_explicit (from clap's value_source), threaded through Middleware and PipelineOpts::fields_are_default, since self.fields alone can't tell "user typed --fields" apart from "clap filled in default_fields as that flag's native default."
The same explicit-vs-default distinction fixed two related pre-existing gaps surfaced during review:
- A registered human view's column narrowing (select_columns in human.rs) silently skipped an unknown explicit --fields name the same way the original bug did — now validated against the view's column catalog when fields_explicit is set.
- effective_fields branched on self.fields.is_empty(), so an explicit --fields "" (meant to keep everything, like all/*) silently fell back to default_fields instead. Switched to branch on fields_explicit.

Test plan

cargo fmt --all --check
cargo clippy --all-targets -- -D warnings
RUSTDOCFLAGS='-D warnings' cargo doc --no-deps
cargo test --all-targets / cargo test --doc
Unit tests in pipeline.rs: typo detection + suggestion, valid names, nested-path top-level check, no-signal shapes, all/*/empty bypass, default_fields bypass
CLI-level integration tests in tests/consumer_cli.rs: the reported scenario end-to-end, default_fields with an absent optional field, human-view column validation, explicit empty --fields
Manually verified against a local gddy build patched to this branch, reproducing the original report and the default-fields/human-view/empty-fields edge cases

… rows Selecting a field that doesn't exist in the response data (e.g. a typo or wrong case) silently projected to nothing, producing rows with no data instead of a clear error. `apply_pipeline` now validates requested field names against the response data's actual top-level keys and errors with a "did you mean" suggestion and the list of valid fields.

Copilot

Pull request overview

This PR improves the CLI output pipeline so --fields no longer silently drops unknown field names (which previously could produce empty projected rows), instead returning a clear error with a “did you mean …?” suggestion and the list of valid top-level fields when the response shape provides enough signal.

Changes:

Added --fields validation in the output pipeline to reject unknown top-level field names (with nearest-name suggestion and valid-field listing).
Extended FieldTree with a top-level-name iterator to support field validation.
Added unit tests for the new validation behavior and a CLI-level integration test reproducing the reported “empty rows” scenario end-to-end.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
cli-engine/tests/consumer_cli.rs	Adds an integration test ensuring unknown `--fields` values error instead of producing empty rows, and that valid `--fields` still projects correctly.
cli-engine/src/output/pipeline.rs	Introduces `validate_fields` into `apply_pipeline` plus supporting helpers and unit tests for field-name validation and suggestions.
cli-engine/src/output/fields.rs	Adds `FieldTree::top_level_names()` to expose requested top-level field segments for validation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Addresses Copilot review feedback: apply_pipeline previously parsed the --fields string twice (once in validate_fields, once inside filter_fields) and scanned the response data on each pass. Extract project_fields (the tree-consuming core of filter_fields) and parse the FieldTree once in apply_pipeline, reusing it for both validation and projection.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

cli-engine/src/output/pipeline.rs:103

When multiple unknown field names are provided, the error message still includes a single “did you mean …?” suggestion derived from only the first unknown name. This can be misleading because it reads like a suggestion for the entire set of unknown fields. Consider only adding the suggestion when exactly one field is unknown.

    if let Some(first) = unknown.first()
        && let Some(suggestion) = nearest_field(first, known)
    {
        message.push_str(&format!(" (did you mean \"{suggestion}\"?)"));
    }

Addresses Copilot review feedback: a command's default_fields is author-controlled and applied on every invocation, so validating it against the response the same way as a user-typed --fields risked hard-erroring a command for everyone the first time a legitimately optional default field was absent from a particular result set. Distinguishing "explicit --fields" from "clap filled in default_fields as that flag's native default" requires clap's value_source, since middleware's `fields` string is identical in both cases once a command has default_fields configured. Added GlobalFlags::fields_explicit (via matches.value_source("fields") == CommandLine) and threaded it through Middleware and PipelineOpts::fields_are_default.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

…s too Addresses Copilot review feedback: apply_pipeline's field validation never runs when a registered human view is active, since projection_fields is forced to "" so the view can narrow its own columns instead of the data being projected. That narrowing (select_columns in human.rs) silently skips a --fields name with no matching column, reproducing the exact "typo produces an empty/partial table instead of an error" bug this PR fixes elsewhere. Check an explicit --fields (never a default_fields fallback, gated on fields_explicit like the response-data validation) against the view's registered column catalog before rendering. Shares unknown_fields_message (now pub(crate)) with the response-data check so both surfaces produce the same message shape.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

cli-engine/src/output/pipeline.rs:101

validate_fields always fully traverses the entire array to build the union of top-level keys (known_top_level_keys), and then project_fields traverses the array again to actually project. For large list responses this adds a full extra pass even when all requested field names are valid and present early. Consider short-circuiting validation for arrays by scanning until every requested top-level name has been observed at least once, and only building the full known set (for the error message) if an unknown remains after the scan.

fn known_top_level_keys(data: &Value) -> Option<BTreeSet<String>> {
    match data {
        Value::Object(map) => Some(map.keys().cloned().collect()),
        Value::Array(items) => {
            let mut keys = BTreeSet::new();
            let mut saw_object = false;
            for item in items {
                match item {
                    Value::Object(map) => {
                        saw_object = true;
                        keys.extend(map.keys().cloned());
                    }
                    Value::Null => {}
                    _ => return None,
                }
            }
            saw_object.then_some(keys)
        }

… default_fields Addresses Copilot review feedback: effective_fields branched on self.fields.is_empty(), but once a command has default_fields set, clap fills self.fields with that same non-empty string whether or not the user typed --fields — so emptiness alone can't tell "user explicitly cleared --fields" apart from "user never touched it." Only fields_explicit (value_source) can. Switched the branch to fields_explicit so an explicit --fields "" keeps everything, matching the documented all/*/empty behavior, instead of silently narrowing to default_fields. Updates a pre-existing test that mutates Middleware.fields directly (bypassing clap) to also set the new fields_explicit flag, preserving its intent now that emptiness alone no longer implies "not explicit."

Copilot