From 8cfa96b4c0f082b76e9dd39308b4369294506990 Mon Sep 17 00:00:00 2001 From: Stefan Holderbach Date: Thu, 21 Dec 2023 16:48:15 +0100 Subject: [PATCH] Construct `Record`s only through checked helpers (#11386) # Description Constructing the internals of `Record` without checking the lengths is bad. (also incompatible with changes to how we store records) - Use `Record::from_raw_cols_vals` in dataframe code - Use `record!` macro in dataframe test - Use `record!` in `nu-color-config` tests - Stop direct record construction in `nu-command` - Refactor table construction in `from nuon` # User-Facing Changes None # Tests + Formatting No new tests, updated tests in equal fashion --- .../values/nu_dataframe/conversion.rs | 19 +++++-------- .../src/dataframe/values/nu_dataframe/mod.rs | 5 +++- crates/nu-color-config/src/color_config.rs | 27 +++++++------------ crates/nu-command/src/charting/histogram.rs | 8 +++--- .../nu-command/src/database/values/sqlite.rs | 8 +----- .../nu-command/src/formats/from/delimited.rs | 5 +--- crates/nu-command/src/formats/from/nuon.rs | 17 +++++------- .../nu-command/src/strings/detect_columns.rs | 9 ++++--- 8 files changed, 39 insertions(+), 59 deletions(-) diff --git a/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/conversion.rs b/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/conversion.rs index d7b3ba7d8a..931fa40f42 100644 --- a/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/conversion.rs +++ b/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/conversion.rs @@ -874,7 +874,7 @@ fn series_to_values( .iter() .map(|field| field.name.to_string()) .collect(); - let record = Record { cols, vals: vals? }; + let record = Record::from_raw_cols_vals(cols, vals?); Ok(Value::record(record, span)) }) .collect(); @@ -982,10 +982,7 @@ fn any_value_to_value(any_value: &AnyValue, span: Span) -> Result Result { #[cfg(test)] mod tests { use indexmap::indexmap; + use nu_protocol::record; use polars::export::arrow::array::{BooleanArray, PrimitiveArray}; use polars::prelude::Field; use polars_io::prelude::StructArray; @@ -1249,13 +1247,10 @@ mod tests { Field::new(field_name_1, DataType::Boolean), ]; let test_owned_struct = AnyValue::StructOwned(Box::new((values, fields.clone()))); - let comparison_owned_record = Value::record( - Record { - cols: vec![field_name_0.to_owned(), field_name_1.to_owned()], - vals: vec![Value::int(1, span), Value::bool(true, span)], - }, - span, - ); + let comparison_owned_record = Value::test_record(record!( + field_name_0 => Value::int(1, span), + field_name_1 => Value::bool(true, span), + )); assert_eq!( any_value_to_value(&test_owned_struct, span)?, comparison_owned_record.clone() diff --git a/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/mod.rs b/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/mod.rs index b19e94ff95..8cf2fc8c66 100644 --- a/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/mod.rs +++ b/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/mod.rs @@ -158,7 +158,10 @@ impl NuDataFrame { .map(|i| format!("{i}")) .collect::>(); - conversion::insert_record(&mut column_values, Record { cols, vals })? + conversion::insert_record( + &mut column_values, + Record::from_raw_cols_vals(cols, vals), + )? } Value::Record { val: record, .. } => { conversion::insert_record(&mut column_values, record)? diff --git a/crates/nu-color-config/src/color_config.rs b/crates/nu-color-config/src/color_config.rs index 4519e4548c..8f25ada576 100644 --- a/crates/nu-color-config/src/color_config.rs +++ b/crates/nu-color-config/src/color_config.rs @@ -92,7 +92,7 @@ fn color_string_to_nustyle(color_string: String) -> Style { mod tests { use super::*; use nu_ansi_term::{Color, Style}; - use nu_protocol::{Span, Value}; + use nu_protocol::{record, Span, Value}; #[test] fn test_color_string_to_nustyle_empty_string() { @@ -120,13 +120,10 @@ mod tests { #[test] fn test_get_style_from_value() { // Test case 1: all values are valid - let record = Record { - cols: vec!["bg".to_string(), "fg".to_string(), "attr".to_string()], - vals: vec![ - Value::string("red", Span::unknown()), - Value::string("blue", Span::unknown()), - Value::string("bold", Span::unknown()), - ], + let record = record! { + "bg" => Value::test_string("red"), + "fg" => Value::test_string("blue"), + "attr" => Value::test_string("bold"), }; let expected_style = NuStyle { bg: Some("red".to_string()), @@ -136,19 +133,15 @@ mod tests { assert_eq!(get_style_from_value(&record), Some(expected_style)); // Test case 2: no values are valid - let record = Record { - cols: vec!["invalid".to_string()], - vals: vec![Value::nothing(Span::unknown())], + let record = record! { + "invalid" => Value::nothing(Span::test_data()), }; assert_eq!(get_style_from_value(&record), None); // Test case 3: some values are valid - let record = Record { - cols: vec!["bg".to_string(), "invalid".to_string()], - vals: vec![ - Value::string("green", Span::unknown()), - Value::nothing(Span::unknown()), - ], + let record = record! { + "bg" => Value::test_string("green"), + "invalid" => Value::nothing(Span::unknown()), }; let expected_style = NuStyle { bg: Some("green".to_string()), diff --git a/crates/nu-command/src/charting/histogram.rs b/crates/nu-command/src/charting/histogram.rs index d10e83ce55..b84ef485bc 100755 --- a/crates/nu-command/src/charting/histogram.rs +++ b/crates/nu-command/src/charting/histogram.rs @@ -258,16 +258,16 @@ fn histogram_impl( result.push(( count, // attach count first for easily sorting. Value::record( - Record { - cols: result_cols.clone(), - vals: vec![ + Record::from_raw_cols_vals( + result_cols.clone(), + vec![ val.into_value(), Value::int(count, span), Value::float(quantile, span), Value::string(percentage, span), Value::string(freq, span), ], - }, + ), span, ), )); diff --git a/crates/nu-command/src/database/values/sqlite.rs b/crates/nu-command/src/database/values/sqlite.rs index b5f390df72..d9eb2711aa 100644 --- a/crates/nu-command/src/database/values/sqlite.rs +++ b/crates/nu-command/src/database/values/sqlite.rs @@ -499,13 +499,7 @@ pub fn convert_sqlite_row_to_nu_value(row: &Row, span: Span, column_names: Vec Value { diff --git a/crates/nu-command/src/formats/from/delimited.rs b/crates/nu-command/src/formats/from/delimited.rs index 33e744fe26..87d22bb22c 100644 --- a/crates/nu-command/src/formats/from/delimited.rs +++ b/crates/nu-command/src/formats/from/delimited.rs @@ -55,10 +55,7 @@ fn from_delimited_string_to_value( .collect::>(); rows.push(Value::record( - Record { - cols: headers.clone(), - vals: output_row, - }, + Record::from_raw_cols_vals(headers.clone(), output_row), span, )); } diff --git a/crates/nu-command/src/formats/from/nuon.rs b/crates/nu-command/src/formats/from/nuon.rs index 7e28e3f496..c56fb5eabd 100644 --- a/crates/nu-command/src/formats/from/nuon.rs +++ b/crates/nu-command/src/formats/from/nuon.rs @@ -403,13 +403,7 @@ fn convert_to_value( } for row in cells { - let mut vals = vec![]; - - for cell in row { - vals.push(convert_to_value(cell, span, original_text)?); - } - - if cols.len() != vals.len() { + if cols.len() != row.len() { return Err(ShellError::OutsideSpannedLabeledError { src: original_text.to_string(), error: "Error when loading".into(), @@ -417,12 +411,13 @@ fn convert_to_value( span: expr.span, }); } + let vals: Vec = row + .into_iter() + .map(|cell| convert_to_value(cell, span, original_text)) + .collect::>()?; output.push(Value::record( - Record { - cols: cols.clone(), - vals, - }, + Record::from_raw_cols_vals(cols.clone(), vals), span, )); } diff --git a/crates/nu-command/src/strings/detect_columns.rs b/crates/nu-command/src/strings/detect_columns.rs index c80cd47202..94d1a50f9e 100644 --- a/crates/nu-command/src/strings/detect_columns.rs +++ b/crates/nu-command/src/strings/detect_columns.rs @@ -199,7 +199,10 @@ fn detect_columns( }; if !(l_idx <= r_idx && (r_idx >= 0 || l_idx < (cols.len() as isize))) { - return Value::record(Record { cols, vals }, name_span); + return Value::record( + Record::from_raw_cols_vals(cols, vals), + name_span, + ); } (l_idx.max(0) as usize, (r_idx as usize + 1).min(cols.len())) @@ -210,7 +213,7 @@ fn detect_columns( } } } else { - return Value::record(Record { cols, vals }, name_span); + return Value::record(Record::from_raw_cols_vals(cols, vals), name_span); }; // Merge Columns @@ -232,7 +235,7 @@ fn detect_columns( vals.push(binding); last_seg.into_iter().for_each(|v| vals.push(v)); - Value::record(Record { cols, vals }, name_span) + Value::record(Record::from_raw_cols_vals(cols, vals), name_span) }) .into_pipeline_data(ctrlc)) } else {