Ensure consistent map ordering when reading YAML (#9155)

# Description

This change ensures that the ordering of map keys when reading YAML
files is consistent. Previously a `HashMap` was used to store the
mappings, but that would result in non-deterministic ordering of the
keys. Switching to an `IndexMap` fixes this.

Fixes https://github.com/nushell/nushell/issues/8662

# User-Facing Changes

User's can rely on consistent ordering of map keys from YAML.

# Tests + Formatting

A unit test ensuring the ordering has been added.

# After Submitting

None.
This commit is contained in:
Michael Albers 2023-05-10 05:30:55 -06:00 committed by GitHub
parent 2a484a3e7e
commit 6c13c67528
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -1,3 +1,4 @@
use indexmap::IndexMap;
use itertools::Itertools; use itertools::Itertools;
use nu_protocol::ast::Call; use nu_protocol::ast::Call;
use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::engine::{Command, EngineState, Stack};
@ -6,7 +7,6 @@ use nu_protocol::{
Value, Value,
}; };
use serde::de::Deserialize; use serde::de::Deserialize;
use std::collections::HashMap;
#[derive(Clone)] #[derive(Clone)]
pub struct FromYaml; pub struct FromYaml;
@ -113,7 +113,8 @@ fn convert_yaml_value_to_nu_value(
} }
serde_yaml::Value::Mapping(t) => { serde_yaml::Value::Mapping(t) => {
let mut collected = Spanned { let mut collected = Spanned {
item: HashMap::new(), // Using an IndexMap ensures consistent ordering
item: IndexMap::new(),
span, span,
}; };
@ -339,6 +340,68 @@ mod test {
test_examples(FromYaml {}) test_examples(FromYaml {})
} }
#[test]
fn test_consistent_mapping_ordering() {
let test_yaml = "- a: b
b: c
- a: g
b: h";
// Before the fix this test is verifying, the ordering of columns in the resulting
// table was non-deterministic. It would take a few executions of the YAML conversion to
// see this ordering difference. This loop should be far more than enough to catch a regression.
for ii in 1..1000 {
let actual = from_yaml_string_to_value(
String::from(test_yaml),
Span::test_data(),
Span::test_data(),
);
let expected: Result<Value, ShellError> = Ok(Value::List {
vals: vec![
Value::Record {
cols: vec!["a".to_string(), "b".to_string()],
vals: vec![Value::test_string("b"), Value::test_string("c")],
span: Span::test_data(),
},
Value::Record {
cols: vec!["a".to_string(), "b".to_string()],
vals: vec![Value::test_string("g"), Value::test_string("h")],
span: Span::test_data(),
},
],
span: Span::test_data(),
});
// Unfortunately the eq function for Value doesn't compare well enough to detect
// ordering errors in List columns or values.
assert!(actual.is_ok());
let actual = actual.ok().unwrap();
let expected = expected.ok().unwrap();
let actual_vals = actual.as_list().unwrap();
let expected_vals = expected.as_list().unwrap();
assert_eq!(expected_vals.len(), actual_vals.len(), "iteration {ii}");
for jj in 0..expected_vals.len() {
let actual_record = actual_vals[jj].as_record().unwrap();
let expected_record = expected_vals[jj].as_record().unwrap();
let actual_columns = actual_record.0;
let expected_columns = expected_record.0;
assert_eq!(
expected_columns, actual_columns,
"record {jj}, iteration {ii}"
);
let actual_vals = actual_record.1;
let expected_vals = expected_record.1;
assert_eq!(expected_vals, actual_vals, "record {jj}, iteration {ii}")
}
}
}
#[test] #[test]
fn test_convert_yaml_value_to_nu_value_for_tagged_values() { fn test_convert_yaml_value_to_nu_value_for_tagged_values() {
struct TestCase { struct TestCase {