Construct Records 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
This commit is contained in:
Stefan Holderbach 2023-12-21 16:48:15 +01:00 committed by GitHub
parent 6f384da57e
commit 8cfa96b4c0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 39 additions and 59 deletions

View File

@ -874,7 +874,7 @@ fn series_to_values(
.iter() .iter()
.map(|field| field.name.to_string()) .map(|field| field.name.to_string())
.collect(); .collect();
let record = Record { cols, vals: vals? }; let record = Record::from_raw_cols_vals(cols, vals?);
Ok(Value::record(record, span)) Ok(Value::record(record, span))
}) })
.collect(); .collect();
@ -982,10 +982,7 @@ fn any_value_to_value(any_value: &AnyValue, span: Span) -> Result<Value, ShellEr
.map(|f| f.name().to_string()) .map(|f| f.name().to_string())
.collect(); .collect();
Ok(Value::Record { Ok(Value::Record {
val: Record { val: Record::from_raw_cols_vals(fields, values?),
cols: fields,
vals: values?,
},
internal_span: span, internal_span: span,
}) })
} }
@ -1059,6 +1056,7 @@ fn time_from_midnight(nanos: i64, span: Span) -> Result<Value, ShellError> {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use indexmap::indexmap; use indexmap::indexmap;
use nu_protocol::record;
use polars::export::arrow::array::{BooleanArray, PrimitiveArray}; use polars::export::arrow::array::{BooleanArray, PrimitiveArray};
use polars::prelude::Field; use polars::prelude::Field;
use polars_io::prelude::StructArray; use polars_io::prelude::StructArray;
@ -1249,13 +1247,10 @@ mod tests {
Field::new(field_name_1, DataType::Boolean), Field::new(field_name_1, DataType::Boolean),
]; ];
let test_owned_struct = AnyValue::StructOwned(Box::new((values, fields.clone()))); let test_owned_struct = AnyValue::StructOwned(Box::new((values, fields.clone())));
let comparison_owned_record = Value::record( let comparison_owned_record = Value::test_record(record!(
Record { field_name_0 => Value::int(1, span),
cols: vec![field_name_0.to_owned(), field_name_1.to_owned()], field_name_1 => Value::bool(true, span),
vals: vec![Value::int(1, span), Value::bool(true, span)], ));
},
span,
);
assert_eq!( assert_eq!(
any_value_to_value(&test_owned_struct, span)?, any_value_to_value(&test_owned_struct, span)?,
comparison_owned_record.clone() comparison_owned_record.clone()

View File

@ -158,7 +158,10 @@ impl NuDataFrame {
.map(|i| format!("{i}")) .map(|i| format!("{i}"))
.collect::<Vec<String>>(); .collect::<Vec<String>>();
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, .. } => { Value::Record { val: record, .. } => {
conversion::insert_record(&mut column_values, record)? conversion::insert_record(&mut column_values, record)?

View File

@ -92,7 +92,7 @@ fn color_string_to_nustyle(color_string: String) -> Style {
mod tests { mod tests {
use super::*; use super::*;
use nu_ansi_term::{Color, Style}; use nu_ansi_term::{Color, Style};
use nu_protocol::{Span, Value}; use nu_protocol::{record, Span, Value};
#[test] #[test]
fn test_color_string_to_nustyle_empty_string() { fn test_color_string_to_nustyle_empty_string() {
@ -120,13 +120,10 @@ mod tests {
#[test] #[test]
fn test_get_style_from_value() { fn test_get_style_from_value() {
// Test case 1: all values are valid // Test case 1: all values are valid
let record = Record { let record = record! {
cols: vec!["bg".to_string(), "fg".to_string(), "attr".to_string()], "bg" => Value::test_string("red"),
vals: vec![ "fg" => Value::test_string("blue"),
Value::string("red", Span::unknown()), "attr" => Value::test_string("bold"),
Value::string("blue", Span::unknown()),
Value::string("bold", Span::unknown()),
],
}; };
let expected_style = NuStyle { let expected_style = NuStyle {
bg: Some("red".to_string()), bg: Some("red".to_string()),
@ -136,19 +133,15 @@ mod tests {
assert_eq!(get_style_from_value(&record), Some(expected_style)); assert_eq!(get_style_from_value(&record), Some(expected_style));
// Test case 2: no values are valid // Test case 2: no values are valid
let record = Record { let record = record! {
cols: vec!["invalid".to_string()], "invalid" => Value::nothing(Span::test_data()),
vals: vec![Value::nothing(Span::unknown())],
}; };
assert_eq!(get_style_from_value(&record), None); assert_eq!(get_style_from_value(&record), None);
// Test case 3: some values are valid // Test case 3: some values are valid
let record = Record { let record = record! {
cols: vec!["bg".to_string(), "invalid".to_string()], "bg" => Value::test_string("green"),
vals: vec![ "invalid" => Value::nothing(Span::unknown()),
Value::string("green", Span::unknown()),
Value::nothing(Span::unknown()),
],
}; };
let expected_style = NuStyle { let expected_style = NuStyle {
bg: Some("green".to_string()), bg: Some("green".to_string()),

View File

@ -258,16 +258,16 @@ fn histogram_impl(
result.push(( result.push((
count, // attach count first for easily sorting. count, // attach count first for easily sorting.
Value::record( Value::record(
Record { Record::from_raw_cols_vals(
cols: result_cols.clone(), result_cols.clone(),
vals: vec![ vec![
val.into_value(), val.into_value(),
Value::int(count, span), Value::int(count, span),
Value::float(quantile, span), Value::float(quantile, span),
Value::string(percentage, span), Value::string(percentage, span),
Value::string(freq, span), Value::string(freq, span),
], ],
}, ),
span, span,
), ),
)); ));

View File

@ -499,13 +499,7 @@ pub fn convert_sqlite_row_to_nu_value(row: &Row, span: Span, column_names: Vec<S
vals.push(val); vals.push(val);
} }
Value::record( Value::record(Record::from_raw_cols_vals(column_names, vals), span)
Record {
cols: column_names,
vals,
},
span,
)
} }
pub fn convert_sqlite_value_to_nu_value(value: ValueRef, span: Span) -> Value { pub fn convert_sqlite_value_to_nu_value(value: ValueRef, span: Span) -> Value {

View File

@ -55,10 +55,7 @@ fn from_delimited_string_to_value(
.collect::<Vec<Value>>(); .collect::<Vec<Value>>();
rows.push(Value::record( rows.push(Value::record(
Record { Record::from_raw_cols_vals(headers.clone(), output_row),
cols: headers.clone(),
vals: output_row,
},
span, span,
)); ));
} }

View File

@ -403,13 +403,7 @@ fn convert_to_value(
} }
for row in cells { for row in cells {
let mut vals = vec![]; if cols.len() != row.len() {
for cell in row {
vals.push(convert_to_value(cell, span, original_text)?);
}
if cols.len() != vals.len() {
return Err(ShellError::OutsideSpannedLabeledError { return Err(ShellError::OutsideSpannedLabeledError {
src: original_text.to_string(), src: original_text.to_string(),
error: "Error when loading".into(), error: "Error when loading".into(),
@ -417,12 +411,13 @@ fn convert_to_value(
span: expr.span, span: expr.span,
}); });
} }
let vals: Vec<Value> = row
.into_iter()
.map(|cell| convert_to_value(cell, span, original_text))
.collect::<Result<_, _>>()?;
output.push(Value::record( output.push(Value::record(
Record { Record::from_raw_cols_vals(cols.clone(), vals),
cols: cols.clone(),
vals,
},
span, span,
)); ));
} }

View File

@ -199,7 +199,10 @@ fn detect_columns(
}; };
if !(l_idx <= r_idx && (r_idx >= 0 || l_idx < (cols.len() as isize))) { 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())) (l_idx.max(0) as usize, (r_idx as usize + 1).min(cols.len()))
@ -210,7 +213,7 @@ fn detect_columns(
} }
} }
} else { } else {
return Value::record(Record { cols, vals }, name_span); return Value::record(Record::from_raw_cols_vals(cols, vals), name_span);
}; };
// Merge Columns // Merge Columns
@ -232,7 +235,7 @@ fn detect_columns(
vals.push(binding); vals.push(binding);
last_seg.into_iter().for_each(|v| vals.push(v)); 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)) .into_pipeline_data(ctrlc))
} else { } else {