diff --git a/cli-engine/src/cli.rs b/cli-engine/src/cli.rs index 91944f4..ceef2d5 100644 --- a/cli-engine/src/cli.rs +++ b/cli-engine/src/cli.rs @@ -2707,6 +2707,7 @@ fn apply_global_flags(middleware: &mut Middleware, flags: &GlobalFlags, timeout: middleware.verbose = flags.verbose.clone(); middleware.dry_run = flags.dry_run; middleware.fields = flags.fields.clone(); + middleware.fields_explicit = flags.fields_explicit; middleware.filter = flags.filter.clone(); middleware.expr = flags.expr.clone(); middleware.reason = flags.reason.clone(); diff --git a/cli-engine/src/flags.rs b/cli-engine/src/flags.rs index 438f9e3..9d1b66c 100644 --- a/cli-engine/src/flags.rs +++ b/cli-engine/src/flags.rs @@ -66,6 +66,12 @@ pub struct GlobalFlags { pub dry_run: bool, /// Field projection. pub fields: String, + /// Whether `fields` came from an explicit `--fields` flag on the command + /// line, rather than clap filling in a command's `default_fields` (a + /// command with `default_fields` set registers it as that flag's native + /// default, so `fields` is non-empty even when the user never typed + /// `--fields` — this is the only reliable way to tell the two apart). + pub fields_explicit: bool, /// JMESPath per-item filter. pub filter: String, /// JMESPath whole-result expression. @@ -92,6 +98,7 @@ impl Default for GlobalFlags { verbose: String::new(), dry_run: false, fields: String::new(), + fields_explicit: false, filter: String::new(), expr: String::new(), schema: false, @@ -561,6 +568,8 @@ pub fn global_flags_from_matches( .get_one::("fields") .cloned() .unwrap_or_default(), + fields_explicit: matches.value_source("fields") + == Some(clap::parser::ValueSource::CommandLine), filter: matches .get_one::("filter") .cloned() diff --git a/cli-engine/src/middleware.rs b/cli-engine/src/middleware.rs index 3b5ce15..644e197 100644 --- a/cli-engine/src/middleware.rs +++ b/cli-engine/src/middleware.rs @@ -1,5 +1,5 @@ use std::{ - collections::BTreeMap, + collections::{BTreeMap, BTreeSet}, future::Future, sync::Arc, time::{Duration, Instant}, @@ -17,6 +17,7 @@ use crate::{ output::{ Envelope, HumanViewRegistry, NextAction, OutputFormat, PipelineOpts, apply_pipeline, build_error_envelope, is_valid_output_format, render_human_with_registry_selected, + unknown_fields_message, }, }; @@ -489,6 +490,10 @@ pub struct Middleware { pub dry_run: bool, /// User field projection. pub fields: String, + /// Whether `fields` came from an explicit `--fields` flag rather than a + /// command's `default_fields` fallback. See + /// [`GlobalFlags::fields_explicit`](crate::GlobalFlags::fields_explicit). + pub fields_explicit: bool, /// JMESPath per-item list predicate. pub filter: String, /// JMESPath whole-result expression. @@ -1044,19 +1049,63 @@ impl Middleware { } } let output_format = self.output_format.parse::()?; - // The effective field selection: an explicit `--fields` wins, otherwise - // the command's `default_fields` is the default. The same selection is - // applied two ways. With a registered human view, it narrows which of the - // view's columns show, so the view reads the full payload — the data is - // not projected, which would otherwise blank out the kept columns. - // Everywhere else (JSON/TOON, or generic human output) it projects the - // output data. Empty / `all` / `*` keeps everything. - let effective_fields = if self.fields.is_empty() { - default_fields - } else { + // The effective field selection: an explicit `--fields` wins — + // including an explicit empty string, which keeps everything, same + // as `all`/`*` — otherwise the command's `default_fields` is the + // default. Gated on `fields_explicit` rather than + // `self.fields.is_empty()`: 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 can't tell "user + // explicitly cleared it" apart from "user never touched it" — only + // `value_source` (what `fields_explicit` is built from) can. The + // same selection is applied two ways: with a registered human view, + // it narrows which of the view's columns show, so the view reads + // the full payload — the data is not projected, which would + // otherwise blank out the kept columns. Everywhere else (JSON/TOON, + // or generic human output) it projects the output data. + let effective_fields = if self.fields_explicit { self.fields.as_str() + } else { + default_fields }; let human_view = output_format == OutputFormat::Human && self.human_views.has_view(view_id); + // `apply_pipeline` never sees `effective_fields` for a registered + // view (`projection_fields` below is forced to `""` so the view reads + // the full payload), and the view's own column narrowing + // (`select_columns` in `human.rs`) silently skips a name with no + // matching column — the same "typo produces an empty/partial table + // instead of an error" gap `apply_pipeline`'s field validation + // closes elsewhere. So an explicit `--fields` (never a + // `default_fields` fallback — same reasoning as + // `PipelineOpts::fields_are_default`) is checked against the view's + // column catalog here instead. + if human_view + && self.fields_explicit + && let Some(columns) = self.human_views.columns(view_id) + { + let fields = effective_fields.trim(); + if !fields.is_empty() && fields != "all" && fields != "*" { + let known: BTreeSet = + columns.iter().map(|column| column.field.clone()).collect(); + let unknown: BTreeSet<&str> = fields + .split(',') + .map(str::trim) + .filter(|part| !part.is_empty() && !known.contains(*part)) + .collect(); + if !unknown.is_empty() { + let unknown: Vec<&str> = unknown.into_iter().collect(); + let err = CliCoreError::message(unknown_fields_message(&unknown, &known)); + return self.render_error( + &err, + &self.app_id, + start, + user_args, + effective_args, + identity, + ); + } + } + } let projection_fields = if human_view { "" } else { effective_fields }; if let Some(data) = &mut envelope.data { let pagination = apply_pipeline( @@ -1067,6 +1116,7 @@ impl Middleware { offset: self.offset, expr: self.expr.clone(), fields: projection_fields.to_owned(), + fields_are_default: !self.fields_explicit, }, )?; if let Some(pagination) = pagination { diff --git a/cli-engine/src/output/fields.rs b/cli-engine/src/output/fields.rs index dac391f..ba08c16 100644 --- a/cli-engine/src/output/fields.rs +++ b/cli-engine/src/output/fields.rs @@ -8,6 +8,13 @@ pub struct FieldTree { children: BTreeMap>>, } +impl FieldTree { + /// Iterates the top-level field names requested at this level of the tree. + pub(crate) fn top_level_names(&self) -> impl Iterator { + self.children.keys().map(String::as_str) + } +} + /// Parses comma-separated field paths. #[must_use] pub fn parse_fields(fields: &str) -> FieldTree { @@ -29,7 +36,14 @@ pub fn filter_fields(data: &Value, fields: &str) -> Value { if fields.is_empty() || fields == "all" || fields == "*" { return data.clone(); } - let allowed = parse_fields(fields); + project_fields(data, &parse_fields(fields)) +} + +/// Applies an already-parsed field-selection tree, so a caller that also +/// needs the tree (e.g. to validate requested names) can parse the +/// `--fields` string once and reuse it here instead of paying for +/// [`parse_fields`] a second time. +pub(crate) fn project_fields(data: &Value, allowed: &FieldTree) -> Value { match data { Value::Array(items) => { if items @@ -42,13 +56,13 @@ pub fn filter_fields(data: &Value, fields: &str) -> Value { items .iter() .map(|item| match item { - Value::Object(map) => Value::Object(filter_map(map, &allowed)), + Value::Object(map) => Value::Object(filter_map(map, allowed)), other => other.clone(), }) .collect(), ) } - Value::Object(map) => Value::Object(filter_map(map, &allowed)), + Value::Object(map) => Value::Object(filter_map(map, allowed)), other => other.clone(), } } diff --git a/cli-engine/src/output/mod.rs b/cli-engine/src/output/mod.rs index d2056a3..93fe354 100644 --- a/cli-engine/src/output/mod.rs +++ b/cli-engine/src/output/mod.rs @@ -22,6 +22,7 @@ pub use envelope::{ Envelope, ErrorEnvelope, Metadata, NextAction, NextActionParam, PaginationMeta, build_detailed_error_envelope, build_error_envelope, }; +pub(crate) use fields::project_fields; pub use fields::{FieldTree, filter_fields, parse_fields}; pub(crate) use human::terminal_width; pub use human::{ @@ -32,6 +33,7 @@ pub use human::{ render_human_with_registry_selected, render_human_with_view, }; pub use json::render_json; +pub(crate) use pipeline::unknown_fields_message; pub use pipeline::{PipelineOpts, apply_pipeline}; pub use renderer::{ OutputFormat, RendererFactory, is_valid_output_format, render, render_data, render_data_format, diff --git a/cli-engine/src/output/pipeline.rs b/cli-engine/src/output/pipeline.rs index b805319..96d6775 100644 --- a/cli-engine/src/output/pipeline.rs +++ b/cli-engine/src/output/pipeline.rs @@ -1,8 +1,10 @@ +use std::collections::BTreeSet; + use serde_json::Value; use crate::{CliCoreError, Result}; -use super::{PaginationMeta, filter_fields}; +use super::{FieldTree, PaginationMeta, parse_fields, project_fields}; /// Options for the output pipeline. #[derive(Clone, Debug, Default, Eq, PartialEq)] @@ -17,9 +19,22 @@ pub struct PipelineOpts { pub expr: String, /// Comma-separated field projection. pub fields: String, + /// Whether `fields` came from a command's `default_fields` fallback + /// rather than an explicit `--fields` flag. Default-field selections are + /// author-controlled, not user input, so they're projected but not + /// validated — see [`apply_pipeline`]'s note on why. + pub fields_are_default: bool, } /// Applies filter, pagination, expression, and field projection in framework order. +/// +/// Field validation (rejecting names absent from the response data) only +/// runs for an explicit `--fields` flag, not for a command's +/// `default_fields` fallback (`opts.fields_are_default`): default fields are +/// author-controlled and applied to every invocation of a command, so a +/// legitimate optional field that happens to be absent from every row of one +/// particular response (rather than genuinely misspelled) would otherwise +/// hard-error that command for everyone until the author noticed. pub fn apply_pipeline(data: &mut Value, opts: &PipelineOpts) -> Result> { if !opts.filter.is_empty() { apply_filter(data, &opts.filter)?; @@ -32,12 +47,106 @@ pub fn apply_pipeline(data: &mut Value, opts: &PipelineOpts) -> Result Result<()> { + let Some(known) = known_top_level_keys(data) else { + return Ok(()); + }; + let unknown: Vec<&str> = requested + .top_level_names() + .filter(|name| !known.contains(*name)) + .collect(); + if unknown.is_empty() { + return Ok(()); + } + Err(CliCoreError::message(unknown_fields_message( + &unknown, &known, + ))) +} + +fn known_top_level_keys(data: &Value) -> Option> { + 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) + } + _ => None, + } +} + +/// Formats an "unknown field" error: a quoted list of the bad names, a +/// nearest-match suggestion for the first one, and the valid names — shared +/// by response-data field validation ([`validate_fields`]) and human-view +/// column validation (`middleware`'s explicit-`--fields`-against-a-view- +/// registered-columns check), so both surfaces produce the same message shape. +pub(crate) fn unknown_fields_message(unknown: &[&str], known: &BTreeSet) -> String { + let plural = if unknown.len() > 1 { "s" } else { "" }; + let quoted = unknown + .iter() + .map(|name| format!("\"{name}\"")) + .collect::>() + .join(", "); + let mut message = format!("fields: unknown field{plural} {quoted}"); + if let Some(first) = unknown.first() + && let Some(suggestion) = nearest_field(first, known) + { + message.push_str(&format!(" (did you mean \"{suggestion}\"?)")); + } + if !known.is_empty() { + let valid = known.iter().cloned().collect::>().join(", "); + message.push_str(&format!("; valid fields: {valid}")); + } + message +} + +/// Finds the closest known field name within edit-distance `max(1, name_len / +/// 3)`, mirroring `nearest_subcommand`'s tolerance in `cli.rs`. Ties break +/// alphabetically. +fn nearest_field(name: &str, known: &BTreeSet) -> Option { + let name = name.to_ascii_lowercase(); + let max_distance = 1.max(name.chars().count() / 3); + known + .iter() + .map(|candidate| { + ( + strsim::osa_distance(&name, &candidate.to_ascii_lowercase()), + candidate, + ) + }) + .filter(|(distance, _)| *distance <= max_distance) + .min_by(|a, b| a.0.cmp(&b.0).then_with(|| a.1.cmp(b.1))) + .map(|(_, candidate)| candidate.clone()) +} + fn apply_pagination(data: &mut Value, offset: i64, limit: i64) -> Result> { let Value::Array(items) = data else { return Ok(None); @@ -133,7 +242,10 @@ fn search_query(expression: &jmespath::Expression<'_>, data: &Value) -> Result(&unknown.rendered) + .expect("error envelope should be json")["error"]["message"] + .as_str() + .expect("message should be a string") + .to_owned(); + assert_eq!( + message, + "fields: unknown field \"ID\" (did you mean \"id\"?); valid fields: id, name, status" + ); + + let known = cli + .run([ + "my-cli", + "project", + "list", + "--team", + "platform", + "--fields", + "id,status", + ]) + .await; + assert_eq!(known.exit_code, 0); + assert_eq!( + serde_json::from_str::(&known.rendered).expect("json"), + json!({"data": [ + {"id": "p1", "status": "active"}, + {"id": "p2", "status": "disabled"} + ]}) + ); +} + +#[tokio::test] +async fn default_fields_are_projected_without_validation_against_the_response() { + // A command's `default_fields` can list a field the backend simply + // didn't populate for a given response (e.g. an optional column with no + // value in this particular result set) — unlike a user-typed `--fields` + // typo, that must still succeed rather than hard-error every caller of + // this command until the author notices. + let cli = Cli::new( + CliConfig::new("my-cli", "Team CLI", "my-cli").with_module(Module::new( + "Platform Systems", + |_context| { + RuntimeGroupSpec::new(GroupSpec::new("widget", "Manage widgets")).with_command( + RuntimeCommandSpec::new( + CommandSpec::new("list", "List widgets") + .with_default_fields("id,description") + .no_auth(true), + async |_credential, _args| Ok(CommandResult::new(json!([{"id": "w1"}]))), + ), + ) + }, + )), + ); + + let output = cli + .run(["my-cli", "widget", "list", "--output", "json"]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + assert_eq!( + serde_json::from_str::(&output.rendered).expect("json"), + json!({"data": [{"id": "w1"}]}) + ); +} + +#[tokio::test] +async fn explicit_empty_fields_keeps_everything_instead_of_falling_back_to_default_fields() { + // Once a command has `default_fields` set, clap fills `--fields` with + // that same non-empty string whether or not the user typed the flag — + // so `self.fields.is_empty()` can't tell "user explicitly cleared + // --fields" apart from "user never touched it" (`fields_explicit` can). + // An explicit `--fields ""` means "keep everything," same as `all`/`*`, + // and must not silently narrow to the command's own default instead. + let cli = Cli::new( + CliConfig::new("my-cli", "Team CLI", "my-cli").with_module(Module::new( + "Platform Systems", + |_context| { + RuntimeGroupSpec::new(GroupSpec::new("widget", "Manage widgets")).with_command( + RuntimeCommandSpec::new( + CommandSpec::new("list", "List widgets") + .with_default_fields("id") + .no_auth(true), + async |_credential, _args| { + Ok(CommandResult::new(json!([{"id": "w1", "extra": true}]))) + }, + ), + ) + }, + )), + ); + + let output = cli + .run([ + "my-cli", "widget", "list", "--output", "json", "--fields", "", + ]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + assert_eq!( + serde_json::from_str::(&output.rendered).expect("json"), + json!({"data": [{"id": "w1", "extra": true}]}) + ); +} + +#[tokio::test] +async fn human_view_rejects_an_unknown_explicit_field_instead_of_narrowing_to_nothing() { + // With a registered human view, `apply_pipeline`'s field validation never + // runs — the view narrows its own columns instead of projecting the data + // (see the comment in `middleware.rs` above `human_view`'s validation + // check). Column narrowing (`select_columns` in `human.rs`) silently + // skips a name with no matching column, so an explicit `--fields` typo + // here needs its own check against the view's column catalog. + let cli = consumer_cli(); + + let unknown = cli + .run([ + "my-cli", "project", "list", "--team", "platform", "--output", "human", "--fields", + "ID", + ]) + .await; + assert_ne!(unknown.exit_code, 0, "{}", unknown.rendered); + assert!( + unknown.rendered.contains("unknown field \"ID\""), + "{}", + unknown.rendered + ); + assert!( + unknown.rendered.contains("did you mean \"id\"?"), + "{}", + unknown.rendered + ); + assert!( + unknown.rendered.contains("valid fields: id, name, status"), + "{}", + unknown.rendered + ); + + let known = cli + .run([ + "my-cli", "project", "list", "--team", "platform", "--output", "human", "--fields", + "id", + ]) + .await; + assert_eq!(known.exit_code, 0, "{}", known.rendered); + assert!(known.rendered.contains("ID"), "{}", known.rendered); + assert!(!known.rendered.contains("STATUS"), "{}", known.rendered); +} + fn consumer_cli_with_root_actions() -> Cli { Cli::new( CliConfig::new("my-cli", "Team CLI", "my-cli") diff --git a/cli-engine/tests/exhaustive_public_api.rs b/cli-engine/tests/exhaustive_public_api.rs index 9b47da0..191a2eb 100644 --- a/cli-engine/tests/exhaustive_public_api.rs +++ b/cli-engine/tests/exhaustive_public_api.rs @@ -146,6 +146,7 @@ fn parsed_global_flags_cover_defaults_short_aliases_and_optional_values() { verbose: "all".to_owned(), dry_run: true, fields: "id,name".to_owned(), + fields_explicit: true, filter: "active == `true`".to_owned(), expr: "[].id".to_owned(), schema: false, diff --git a/cli-engine/tests/foundation.rs b/cli-engine/tests/foundation.rs index 896fa7e..ffdd173 100644 --- a/cli-engine/tests/foundation.rs +++ b/cli-engine/tests/foundation.rs @@ -4666,6 +4666,7 @@ fn global_flag_defaults_and_derived_flag_classes_cover_common_clap_actions() { verbose: String::new(), dry_run: false, fields: String::new(), + fields_explicit: false, filter: String::new(), expr: String::new(), schema: false, @@ -7315,6 +7316,7 @@ async fn middleware_success_authz_audit_activity_and_fields() { middleware.env = "prod".to_owned(); middleware.verbose = "all".to_owned(); middleware.fields = "name".to_owned(); + middleware.fields_explicit = true; middleware.auditor = Some(audit.clone()); middleware.activity = Some(activity.clone()); middleware.authz = Some(Arc::new(AllowAuthorizer)); @@ -9561,6 +9563,7 @@ fn output_pipeline_applies_filter_pagination_expr_and_fields_in_order() { offset: 1, expr: String::new(), fields: "name,status".to_owned(), + fields_are_default: false, }, ) .expect("pipeline should apply"); @@ -9652,6 +9655,7 @@ fn output_pipeline_defaults_and_non_list_pagination_are_noops() { offset: 0, expr: String::new(), fields: String::new(), + fields_are_default: false, } );