Fix insert/upsert creation for nested lists (#15131) (#15133)

# Description
This PR fixes #15131 by allowing the `insert` and `upsert` commands to
create lists where they may be expected based on the cell path provided.
For example, the below would have previously thrown an error, but now
creates lists and list elements where necessary
<img width="173" alt="Screenshot 2025-02-17 at 2 46 12 AM"
src="https://github.com/user-attachments/assets/6d680e7e-6268-42ed-a037-a0795014a7e0"
/>
<img width="200" alt="Screenshot 2025-02-17 at 2 46 16 AM"
src="https://github.com/user-attachments/assets/50d0e8eb-aabb-49fe-b961-5f7489fdc993"
/>
<img width="284" alt="Screenshot 2025-02-17 at 2 45 43 AM"
src="https://github.com/user-attachments/assets/242a2ec6-7e8f-4a51-92ce-9d5ec10f867f"
/>

# User-Facing Changes
This change removes errors that were previously raised by
`insert_data_at_cell_path` and `upsert_data_at_cell_path`. If one of
these commands encountered an unknown cell path in cases such as these,
it would either raise a "Not a list value" as the list index is used on
a record:

<img width="326" alt="Screenshot 2025-02-17 at 2 46 43 AM"
src="https://github.com/user-attachments/assets/39b9b006-388b-49b3-82a0-8cc9b739feaa"
/>


Or a "Row number too large" when required to create a new list element
along the way:
<img width="475" alt="Screenshot 2025-02-17 at 2 46 51 AM"
src="https://github.com/user-attachments/assets/007d1268-7d26-42aa-9bf5-d54c0abf4058"
/>


But both now succeed, which seems to be the intention as it is in parity
with record behavior. Any consumers depending on this specific behavior
will see these errors subside.

This change also includes the static method
`Value::with_data_at_cell_path` that creates a value with a given nested
value at a given cell path, creating records or lists based on the path
member type. 

# Tests + Formatting
In addition to unit tests for the altered behavior, both affected
user-facing commands (`insert` and `upsert`) gained a new command
example to both explain and test this change at the user level.
<img width="382" alt="Screenshot 2025-02-17 at 2 29 26 AM"
src="https://github.com/user-attachments/assets/e6973640-3ce6-4ea7-9ba5-d256fe5cb38b"
/>

Note: A single test did fail locally, due to my config directory
differing from expected, but works where this variable is unset
(`with-env { XDG_CONFIG_HOME: null } {cargo test}`):
```
---- repl::test_config_path::test_default_config_path stdout ----
thread 'repl::test_config_path::test_default_config_path' panicked at tests/repl/test_config_path.rs:101:5:
assertion failed: `(left == right)`

Diff < left / right > :
<[home_dir]/Library/Application Support/nushell
>[home_dir]/.config/nushell
```
This commit is contained in:
Marcel Mukundi 2025-02-22 22:53:25 -07:00 committed by GitHub
parent bda3245725
commit 083c534948
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 225 additions and 48 deletions

View File

@ -114,6 +114,22 @@ When inserting into a specific index, the closure will instead get the current v
Value::test_int(4), Value::test_int(4),
])), ])),
}, },
Example {
description: "Insert into a nested path, creating new values as needed",
example: "[{} {a: [{}]}] | insert a.0.b \"value\"",
result: Some(Value::test_list(vec![
Value::test_record(record!(
"a" => Value::test_list(vec![Value::test_record(record!(
"b" => Value::test_string("value"),
))]),
)),
Value::test_record(record!(
"a" => Value::test_list(vec![Value::test_record(record!(
"b" => Value::test_string("value"),
))]),
)),
])),
},
] ]
} }
} }

View File

@ -144,6 +144,22 @@ If the command is inserting at the end of a list or table, then both of these va
Value::test_int(4), Value::test_int(4),
])), ])),
}, },
Example {
description: "Upsert into a nested path, creating new values as needed",
example: "[{} {a: [{}]}] | upsert a.0.b \"value\"",
result: Some(Value::test_list(vec![
Value::test_record(record!(
"a" => Value::test_list(vec![Value::test_record(record!(
"b" => Value::test_string("value"),
))]),
)),
Value::test_record(record!(
"a" => Value::test_list(vec![Value::test_record(record!(
"b" => Value::test_string("value"),
))]),
)),
])),
},
] ]
} }
} }

View File

@ -1333,15 +1333,8 @@ impl Value {
if let Some(val) = record.get_mut(col_name) { if let Some(val) = record.get_mut(col_name) {
val.upsert_data_at_cell_path(path, new_val.clone())?; val.upsert_data_at_cell_path(path, new_val.clone())?;
} else { } else {
let new_col = if path.is_empty() { let new_col =
new_val.clone() Value::with_data_at_cell_path(path, new_val.clone())?;
} else {
let mut new_col =
Value::record(Record::new(), new_val.span());
new_col
.upsert_data_at_cell_path(path, new_val.clone())?;
new_col
};
record.push(col_name, new_col); record.push(col_name, new_col);
} }
} }
@ -1361,13 +1354,7 @@ impl Value {
if let Some(val) = record.get_mut(col_name) { if let Some(val) = record.get_mut(col_name) {
val.upsert_data_at_cell_path(path, new_val)?; val.upsert_data_at_cell_path(path, new_val)?;
} else { } else {
let new_col = if path.is_empty() { let new_col = Value::with_data_at_cell_path(path, new_val.clone())?;
new_val
} else {
let mut new_col = Value::record(Record::new(), new_val.span());
new_col.upsert_data_at_cell_path(path, new_val)?;
new_col
};
record.push(col_name, new_col); record.push(col_name, new_col);
} }
} }
@ -1391,14 +1378,9 @@ impl Value {
available_idx: vals.len(), available_idx: vals.len(),
span: *span, span: *span,
}); });
} else if !path.is_empty() {
return Err(ShellError::AccessBeyondEnd {
max_idx: vals.len() - 1,
span: *span,
});
} else { } else {
// If the upsert is at 1 + the end of the list, it's OK. // If the upsert is at 1 + the end of the list, it's OK.
vals.push(new_val); vals.push(Value::with_data_at_cell_path(path, new_val)?);
} }
} }
Value::Error { error, .. } => return Err(*error.clone()), Value::Error { error, .. } => return Err(*error.clone()),
@ -1679,7 +1661,6 @@ impl Value {
} }
} }
} }
pub fn insert_data_at_cell_path( pub fn insert_data_at_cell_path(
&mut self, &mut self,
cell_path: &[PathMember], cell_path: &[PathMember],
@ -1715,18 +1696,8 @@ impl Value {
)?; )?;
} }
} else { } else {
let new_col = if path.is_empty() { let new_col =
new_val.clone() Value::with_data_at_cell_path(path, new_val.clone())?;
} else {
let mut new_col =
Value::record(Record::new(), new_val.span());
new_col.insert_data_at_cell_path(
path,
new_val.clone(),
head_span,
)?;
new_col
};
record.push(col_name, new_col); record.push(col_name, new_col);
} }
} }
@ -1755,13 +1726,7 @@ impl Value {
val.insert_data_at_cell_path(path, new_val, head_span)?; val.insert_data_at_cell_path(path, new_val, head_span)?;
} }
} else { } else {
let new_col = if path.is_empty() { let new_col = Value::with_data_at_cell_path(path, new_val)?;
new_val
} else {
let mut new_col = Value::record(Record::new(), new_val.span());
new_col.insert_data_at_cell_path(path, new_val, head_span)?;
new_col
};
record.push(col_name, new_col); record.push(col_name, new_col);
} }
} }
@ -1789,14 +1754,9 @@ impl Value {
available_idx: vals.len(), available_idx: vals.len(),
span: *span, span: *span,
}); });
} else if !path.is_empty() {
return Err(ShellError::AccessBeyondEnd {
max_idx: vals.len() - 1,
span: *span,
});
} else { } else {
// If the insert is at 1 + the end of the list, it's OK. // If the insert is at 1 + the end of the list, it's OK.
vals.push(new_val); vals.push(Value::with_data_at_cell_path(path, new_val)?);
} }
} }
_ => { _ => {
@ -1813,6 +1773,36 @@ impl Value {
Ok(()) Ok(())
} }
/// Creates a new [Value] with the specified member at the specified path.
/// This is used by [Value::insert_data_at_cell_path] and [Value::upsert_data_at_cell_path] whenever they have the need to insert a non-existent element
fn with_data_at_cell_path(cell_path: &[PathMember], value: Value) -> Result<Value, ShellError> {
if let Some((member, path)) = cell_path.split_first() {
let span = value.span();
match member {
PathMember::String { val, .. } => Ok(Value::record(
std::iter::once((val.clone(), Value::with_data_at_cell_path(path, value)?))
.collect(),
span,
)),
PathMember::Int { val, .. } => {
if *val == 0usize {
Ok(Value::list(
vec![Value::with_data_at_cell_path(path, value)?],
span,
))
} else {
Err(ShellError::InsertAfterNextFreeIndex {
available_idx: 0,
span,
})
}
}
}
} else {
Ok(value)
}
}
/// Visits all values contained within the value (including this value) with a mutable reference /// Visits all values contained within the value (including this value) with a mutable reference
/// given to the closure. /// given to the closure.
/// ///
@ -4027,6 +4017,161 @@ mod tests {
use super::{Record, Value}; use super::{Record, Value};
use crate::record; use crate::record;
mod at_cell_path {
use crate::{IntoValue, Span};
use super::super::PathMember;
use super::*;
#[test]
fn test_record_with_data_at_cell_path() {
let value_to_insert = Value::test_string("value");
let span = Span::test_data();
assert_eq!(
Value::with_data_at_cell_path(
&[
PathMember::test_string("a".to_string(), false),
PathMember::test_string("b".to_string(), false),
PathMember::test_string("c".to_string(), false),
PathMember::test_string("d".to_string(), false),
],
value_to_insert,
),
// {a:{b:c{d:"value"}}}
Ok(record!(
"a" => record!(
"b" => record!(
"c" => record!(
"d" => Value::test_string("value")
).into_value(span)
).into_value(span)
).into_value(span)
)
.into_value(span))
);
}
#[test]
fn test_lists_with_data_at_cell_path() {
let value_to_insert = Value::test_string("value");
assert_eq!(
Value::with_data_at_cell_path(
&[
PathMember::test_int(0, false),
PathMember::test_int(0, false),
PathMember::test_int(0, false),
PathMember::test_int(0, false),
],
value_to_insert.clone(),
),
// [[[[["value"]]]]]
Ok(Value::test_list(vec![Value::test_list(vec![
Value::test_list(vec![Value::test_list(vec![value_to_insert])])
])]))
);
}
#[test]
fn test_mixed_with_data_at_cell_path() {
let value_to_insert = Value::test_string("value");
let span = Span::test_data();
assert_eq!(
Value::with_data_at_cell_path(
&[
PathMember::test_string("a".to_string(), false),
PathMember::test_int(0, false),
PathMember::test_string("b".to_string(), false),
PathMember::test_int(0, false),
PathMember::test_string("c".to_string(), false),
PathMember::test_int(0, false),
PathMember::test_string("d".to_string(), false),
PathMember::test_int(0, false),
],
value_to_insert.clone(),
),
// [{a:[{b:[{c:[{d:["value"]}]}]}]]}
Ok(record!(
"a" => Value::test_list(vec![record!(
"b" => Value::test_list(vec![record!(
"c" => Value::test_list(vec![record!(
"d" => Value::test_list(vec![value_to_insert])
).into_value(span)])
).into_value(span)])
).into_value(span)])
)
.into_value(span))
);
}
#[test]
fn test_nested_upsert_data_at_cell_path() {
let span = Span::test_data();
let mut base_value = record!(
"a" => Value::test_list(vec![])
)
.into_value(span);
let value_to_insert = Value::test_string("value");
let res = base_value.upsert_data_at_cell_path(
&[
PathMember::test_string("a".to_string(), false),
PathMember::test_int(0, false),
PathMember::test_string("b".to_string(), false),
PathMember::test_int(0, false),
],
value_to_insert.clone(),
);
assert_eq!(res, Ok(()));
assert_eq!(
base_value,
// {a:[{b:["value"]}]}
record!(
"a" => Value::test_list(vec![
record!(
"b" => Value::test_list(vec![value_to_insert])
)
.into_value(span)
])
)
.into_value(span)
);
}
#[test]
fn test_nested_insert_data_at_cell_path() {
let span = Span::test_data();
let mut base_value = record!(
"a" => Value::test_list(vec![])
)
.into_value(span);
let value_to_insert = Value::test_string("value");
let res = base_value.insert_data_at_cell_path(
&[
PathMember::test_string("a".to_string(), false),
PathMember::test_int(0, false),
PathMember::test_string("b".to_string(), false),
PathMember::test_int(0, false),
],
value_to_insert.clone(),
span,
);
assert_eq!(res, Ok(()));
assert_eq!(
base_value,
// {a:[{b:["value"]}]}
record!(
"a" => Value::test_list(vec![
record!(
"b" => Value::test_list(vec![value_to_insert])
)
.into_value(span)
])
)
.into_value(span)
);
}
}
mod is_empty { mod is_empty {
use super::*; use super::*;