diff --git a/core/json/json_operations.rs b/core/json/json_operations.rs index fe1a1b359..46f416448 100644 --- a/core/json/json_operations.rs +++ b/core/json/json_operations.rs @@ -155,16 +155,16 @@ pub fn json_remove(args: &[OwnedValue]) -> crate::Result { return Ok(OwnedValue::Null); } - let mut json = convert_dbtype_to_jsonb(&args[0])?; + let mut json = convert_dbtype_to_jsonb(&args[0], true)?; for arg in &args[1..] { if let Some(path) = json_path_from_owned_value(arg, true)? { - json.remove_by_path(&path)?; + let _ = json.remove_by_path(&path); } } let el_type = json.is_valid()?; - Ok(json_string_to_db_type(json, el_type, false)?) + Ok(json_string_to_db_type(json, el_type, false, true)?) } pub fn jsonb_remove(args: &[OwnedValue]) -> crate::Result { @@ -172,7 +172,7 @@ pub fn jsonb_remove(args: &[OwnedValue]) -> crate::Result { return Ok(OwnedValue::Null); } - let mut json = convert_dbtype_to_jsonb(&args[0])?; + let mut json = convert_dbtype_to_jsonb(&args[0], true)?; for arg in &args[1..] { if let Some(path) = json_path_from_owned_value(arg, true)? { json.remove_by_path(&path)?; @@ -187,32 +187,12 @@ pub fn json_replace(args: &[OwnedValue]) -> crate::Result { return Ok(OwnedValue::Null); } - let mut json = convert_dbtype_to_jsonb(&args[0])?; - let other = args[1..].chunks_exact(2); - for chunk in other { - println!("{:?}", chunk); - let path = json_path_from_owned_value(&chunk[0], true)?; - let value = convert_dbtype_to_jsonb(&chunk[1])?; - if let Some(path) = path { - json.replace_by_path(&path, value)?; - } - } - - let el_type = json.is_valid()?; - - Ok(json_string_to_db_type(json, el_type, false)?) -} - -pub fn jsonb_replace(args: &[OwnedValue]) -> crate::Result { - if args.is_empty() { - return Ok(OwnedValue::Null); - } - - let mut json = convert_dbtype_to_jsonb(&args[0])?; + let mut json = convert_dbtype_to_jsonb(&args[0], true)?; let other = args[1..].chunks_exact(2); for chunk in other { let path = json_path_from_owned_value(&chunk[0], true)?; - let value = convert_dbtype_to_jsonb(&chunk[1])?; + + let value = convert_dbtype_to_jsonb(&chunk[1], false)?; if let Some(path) = path { let _ = json.replace_by_path(&path, value); } @@ -220,7 +200,27 @@ pub fn jsonb_replace(args: &[OwnedValue]) -> crate::Result { let el_type = json.is_valid()?; - Ok(json_string_to_db_type(json, el_type, false)?) + Ok(json_string_to_db_type(json, el_type, false, false)?) +} + +pub fn jsonb_replace(args: &[OwnedValue]) -> crate::Result { + if args.is_empty() { + return Ok(OwnedValue::Null); + } + + let mut json = convert_dbtype_to_jsonb(&args[0], true)?; + let other = args[1..].chunks_exact(2); + for chunk in other { + let path = json_path_from_owned_value(&chunk[0], true)?; + let value = convert_dbtype_to_jsonb(&chunk[1], false)?; + if let Some(path) = path { + let _ = json.replace_by_path(&path, value); + } + } + + let el_type = json.is_valid()?; + + Ok(json_string_to_db_type(json, el_type, false, true)?) } #[cfg(test)] diff --git a/core/json/jsonb.rs b/core/json/jsonb.rs index 9a0f56dc4..924aaf35c 100644 --- a/core/json/jsonb.rs +++ b/core/json/jsonb.rs @@ -1603,14 +1603,6 @@ impl Jsonb { match target { TraverseResult::Value(target_pos) | TraverseResult::ObjectValue(target_pos, _) => { - // fast path - if target_pos == 0 { - let null = JsonbHeader::make_null().into_bytes(); - self.data.clear(); - self.data.push(null.as_bytes()[0]); - return Ok(()); - }; - let (JsonbHeader(_, target_size), target_header_size) = self.read_header(target_pos)?; let target_delta = target_header_size + target_size; @@ -2498,3 +2490,284 @@ world""#, assert!(result.contains(r#""unicode":"\u00A9 2023""#)); } } + +#[cfg(test)] +mod path_mutation_tests { + use super::*; + use crate::json::json_path; + + #[test] + fn test_remove_by_path_simple_object() { + // Test removing a simple key from an object + let mut jsonb = Jsonb::from_str(r#"{"a": 1, "b": 2, "c": 3}"#).unwrap(); + let path = json_path("$.b").unwrap(); + + jsonb.remove_by_path(&path).unwrap(); + assert_eq!(jsonb.to_string().unwrap(), r#"{"a":1,"c":3}"#); + } + + #[test] + fn test_remove_by_path_array_element() { + // Test removing an element from an array + let mut jsonb = Jsonb::from_str(r#"[10, 20, 30, 40]"#).unwrap(); + let path = json_path("$[1]").unwrap(); + + jsonb.remove_by_path(&path).unwrap(); + assert_eq!(jsonb.to_string().unwrap(), r#"[10,30,40]"#); + } + + #[test] + fn test_remove_by_path_nested_object() { + // Test removing a nested property + let mut jsonb = Jsonb::from_str( + r#"{"user": {"name": "Alice", "age": 30, "email": "alice@example.com"}}"#, + ) + .unwrap(); + let path = json_path("$.user.email").unwrap(); + + jsonb.remove_by_path(&path).unwrap(); + assert_eq!( + jsonb.to_string().unwrap(), + r#"{"user":{"name":"Alice","age":30}}"# + ); + } + + #[test] + fn test_remove_by_path_nested_array() { + // Test removing an element from a nested array + let mut jsonb = Jsonb::from_str(r#"{"data": {"values": [1, 2, 3, 4, 5]}}"#).unwrap(); + let path = json_path("$.data.values[2]").unwrap(); + + jsonb.remove_by_path(&path).unwrap(); + assert_eq!( + jsonb.to_string().unwrap(), + r#"{"data":{"values":[1,2,4,5]}}"# + ); + } + + #[test] + fn test_remove_by_path_quoted_key() { + // Test removing an element with a key that contains special characters + let mut jsonb = + Jsonb::from_str(r#"{"normal": 1, "key.with.dots": 2, "key[0]": 3}"#).unwrap(); + let path = json_path(r#"$."key.with.dots""#).unwrap(); + + jsonb.remove_by_path(&path).unwrap(); + assert_eq!(jsonb.to_string().unwrap(), r#"{"normal":1,"key[0]":3}"#); + } + + #[test] + fn test_remove_by_path_entire_object() { + // Test removing the entire object + let mut jsonb = Jsonb::from_str(r#"{"a": 1, "b": 2}"#).unwrap(); + let path = json_path("$").unwrap(); + + jsonb.remove_by_path(&path).unwrap(); + assert_eq!(jsonb.to_string().unwrap(), "null"); + } + + #[test] + fn test_remove_by_path_nonexistent() { + // Test behavior when the path doesn't exist + let mut jsonb = Jsonb::from_str(r#"{"a": 1, "b": 2}"#).unwrap(); + let path = json_path("$.c").unwrap(); + + // This should return an error + let result = jsonb.remove_by_path(&path); + assert!(result.is_err()); + + // Original JSON should remain unchanged + assert_eq!(jsonb.to_string().unwrap(), r#"{"a":1,"b":2}"#); + } + + #[test] + fn test_remove_by_path_complex_nested() { + // Test removing from a complex nested structure + let mut jsonb = Jsonb::from_str( + r#" + { + "store": { + "book": [ + { + "category": "fiction", + "author": "J.R.R. Tolkien", + "title": "The Lord of the Rings", + "price": 22.99 + }, + { + "category": "fiction", + "author": "George R.R. Martin", + "title": "A Song of Ice and Fire", + "price": 19.99 + } + ], + "bicycle": { + "color": "red", + "price": 399.99 + } + } + } + "#, + ) + .unwrap(); + + // Remove the first book's title + let path = json_path("$.store.book[0].title").unwrap(); + jsonb.remove_by_path(&path).unwrap(); + + // Verify the first book no longer has a title but everything else is intact + let result = jsonb.to_string().unwrap(); + assert!(result.contains(r#""author":"J.R.R. Tolkien"#)); + assert!(result.contains(r#""price":22.99"#)); + assert!(!result.contains(r#""title":"The Lord of the Rings"#)); + assert!(result.contains(r#""title":"A Song of Ice and Fire"#)); + } + + #[test] + fn test_replace_by_path_simple_value() { + // Test replacing a simple value + let mut jsonb = Jsonb::from_str(r#"{"a": 1, "b": 2, "c": 3}"#).unwrap(); + let path = json_path("$.b").unwrap(); + let new_value = Jsonb::from_str("42").unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert_eq!(jsonb.to_string().unwrap(), r#"{"a":1,"b":42,"c":3}"#); + } + + #[test] + fn test_replace_by_path_complex_value() { + // Test replacing with a more complex value + let mut jsonb = Jsonb::from_str(r#"{"name": "Original", "value": 123}"#).unwrap(); + let path = json_path("$.value").unwrap(); + let new_value = Jsonb::from_str(r#"{"nested": true, "array": [1, 2, 3]}"#).unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert_eq!( + jsonb.to_string().unwrap(), + r#"{"name":"Original","value":{"nested":true,"array":[1,2,3]}}"# + ); + } + + #[test] + fn test_replace_by_path_array_element() { + // Test replacing an array element + let mut jsonb = Jsonb::from_str(r#"[10, 20, 30, 40]"#).unwrap(); + let path = json_path("$[2]").unwrap(); + let new_value = Jsonb::from_str(r#""replaced""#).unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert_eq!(jsonb.to_string().unwrap(), r#"[10,20,"replaced",40]"#); + } + + #[test] + fn test_replace_by_path_nested_object() { + // Test replacing a property in a nested object + let mut jsonb = Jsonb::from_str(r#"{"user": {"name": "Alice", "age": 30}}"#).unwrap(); + let path = json_path("$.user.age").unwrap(); + let new_value = Jsonb::from_str("31").unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert_eq!( + jsonb.to_string().unwrap(), + r#"{"user":{"name":"Alice","age":31}}"# + ); + } + + #[test] + fn test_replace_by_path_entire_object() { + // Test replacing the entire object + let mut jsonb = Jsonb::from_str(r#"{"old": "data"}"#).unwrap(); + let path = json_path("$").unwrap(); + let new_value = Jsonb::from_str(r#"["completely", "new", "structure"]"#).unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert_eq!( + jsonb.to_string().unwrap(), + r#"["completely","new","structure"]"# + ); + } + + #[test] + fn test_replace_by_path_with_longer_value() { + // Test replacing with a significantly longer value to trigger header recalculation + let mut jsonb = Jsonb::from_str(r#"{"key": "short"}"#).unwrap(); + let path = json_path("$.key").unwrap(); + let new_value = Jsonb::from_str(r#""this is a much longer string that will require more storage space and potentially change the header size""#).unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert_eq!( + jsonb.to_string().unwrap(), + r#"{"key":"this is a much longer string that will require more storage space and potentially change the header size"}"# + ); + } + + #[test] + fn test_replace_by_path_with_shorter_value() { + // Test replacing with a significantly shorter value to trigger header recalculation + let mut jsonb = Jsonb::from_str(r#"{"key": "this is a long string that takes up considerable space in the binary format"}"#).unwrap(); + let path = json_path("$.key").unwrap(); + let new_value = Jsonb::from_str(r#""short""#).unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert_eq!(jsonb.to_string().unwrap(), r#"{"key":"short"}"#); + } + + #[test] + fn test_replace_by_path_deeply_nested() { + // Test replacing a value in a deeply nested structure + let mut jsonb = Jsonb::from_str( + r#" + { + "level1": { + "level2": { + "level3": { + "level4": { + "target": "original value" + } + } + } + } + } + "#, + ) + .unwrap(); + + let path = json_path("$.level1.level2.level3.level4.target").unwrap(); + let new_value = Jsonb::from_str(r#""replaced value""#).unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert!(jsonb + .to_string() + .unwrap() + .contains(r#""target":"replaced value""#)); + } + + #[test] + fn test_replace_by_path_null_with_complex() { + // Test replacing a null value with a complex structure + let mut jsonb = Jsonb::from_str(r#"{"data": null}"#).unwrap(); + let path = json_path("$.data").unwrap(); + let new_value = Jsonb::from_str(r#"{"complex": {"nested": [1, 2, 3]}}"#).unwrap(); + + jsonb.replace_by_path(&path, new_value).unwrap(); + assert_eq!( + jsonb.to_string().unwrap(), + r#"{"data":{"complex":{"nested":[1,2,3]}}}"# + ); + } + + #[test] + fn test_replace_by_path_nonexistent() { + // Test behavior when the path doesn't exist + let mut jsonb = Jsonb::from_str(r#"{"a": 1, "b": 2}"#).unwrap(); + let path = json_path("$.c").unwrap(); + let new_value = Jsonb::from_str("42").unwrap(); + + // This should return an error + let result = jsonb.replace_by_path(&path, new_value); + assert!(result.is_err()); + + // Original JSON should remain unchanged + assert_eq!(jsonb.to_string().unwrap(), r#"{"a":1,"b":2}"#); + } +} diff --git a/core/json/mod.rs b/core/json/mod.rs index e1977af70..cee674c4b 100644 --- a/core/json/mod.rs +++ b/core/json/mod.rs @@ -76,7 +76,7 @@ pub fn get_json(json_value: &OwnedValue, indent: Option<&str>) -> crate::Result< } pub fn jsonb(json_value: &OwnedValue) -> crate::Result { - let jsonbin = convert_dbtype_to_jsonb(json_value); + let jsonbin = convert_dbtype_to_jsonb(json_value, true); match jsonbin { Ok(jsonbin) => Ok(OwnedValue::Blob(Rc::new(jsonbin.data()))), Err(_) => { @@ -85,18 +85,38 @@ pub fn jsonb(json_value: &OwnedValue) -> crate::Result { } } -fn convert_dbtype_to_jsonb(val: &OwnedValue) -> crate::Result { +fn convert_dbtype_to_jsonb(val: &OwnedValue, strict: bool) -> crate::Result { match val { - OwnedValue::Text(text) => Jsonb::from_str(text.as_str()), + OwnedValue::Text(text) => { + let res = if text.subtype == TextSubtype::Json || strict { + // Parse directly as JSON if it's already JSON subtype or strict mode is on + Jsonb::from_str(text.as_str()) + } else { + // Handle as a string literal otherwise + let mut str = text.as_str().replace('"', "\\\""); + if &str == "null" { + // Special case for "null" + Jsonb::from_str(&str) + } else { + // Quote the string to make it a JSON string + str.insert(0, '"'); + str.push('"'); + Jsonb::from_str(&str) + } + }; + res + } OwnedValue::Blob(blob) => { let json = Jsonb::from_raw_data(blob); json.is_valid()?; Ok(json) } OwnedValue::Record(_) | OwnedValue::Agg(_) => { - bail_constraint_error!("Wront number of arguments"); + bail_constraint_error!("Wrong number of arguments"); } - OwnedValue::Null => Jsonb::from_str("null"), + OwnedValue::Null => Ok(Jsonb::from_raw_data( + JsonbHeader::make_null().into_bytes().as_bytes(), + )), OwnedValue::Float(float) => { let mut buff = ryu::Buffer::new(); Jsonb::from_str(buff.format(*float)) @@ -167,7 +187,7 @@ pub fn json_array_length( json_value: &OwnedValue, json_path: Option<&OwnedValue>, ) -> crate::Result { - let json = convert_dbtype_to_jsonb(json_value)?; + let json = convert_dbtype_to_jsonb(json_value, true)?; if json_path.is_none() { let result = json.array_len()?; @@ -236,7 +256,7 @@ pub fn json_arrow_extract(value: &OwnedValue, path: &OwnedValue) -> crate::Resul } if let Some(path) = json_path_from_owned_value(path, false)? { - let json = convert_dbtype_to_jsonb(value)?; + let json = convert_dbtype_to_jsonb(value, true)?; let extracted = json.get_by_path(&path); if let Ok((json, _)) = extracted { Ok(OwnedValue::Text(Text::json(json.to_string()?))) @@ -258,10 +278,10 @@ pub fn json_arrow_shift_extract( return Ok(OwnedValue::Null); } if let Some(path) = json_path_from_owned_value(path, false)? { - let json = convert_dbtype_to_jsonb(value)?; + let json = convert_dbtype_to_jsonb(value, true)?; let extracted = json.get_by_path(&path); if let Ok((json, element_type)) = extracted { - Ok(json_string_to_db_type(json, element_type, false)?) + Ok(json_string_to_db_type(json, element_type, false, true)?) } else { Ok(OwnedValue::Null) } @@ -281,9 +301,9 @@ pub fn json_extract(value: &OwnedValue, paths: &[OwnedValue]) -> crate::Result crate::Result< } let (json, element_type) = jsonb_extract_internal(value, paths)?; - let result = json_string_to_db_type(json, element_type, true)?; + let result = json_string_to_db_type(json, element_type, false, true)?; Ok(result) } @@ -310,7 +330,7 @@ fn jsonb_extract_internal( let null = Jsonb::from_raw_data(JsonbHeader::make_null().into_bytes().as_bytes()); if paths.len() == 1 { if let Some(path) = json_path_from_owned_value(&paths[0], true)? { - let json = convert_dbtype_to_jsonb(value)?; + let json = convert_dbtype_to_jsonb(value, true)?; if let Ok((json, value_type)) = json.get_by_path(&path) { return Ok((json, value_type)); } else { @@ -321,7 +341,7 @@ fn jsonb_extract_internal( } } - let json = convert_dbtype_to_jsonb(value)?; + let json = convert_dbtype_to_jsonb(value, true)?; let mut result = Jsonb::make_empty_array(json.len()); let paths = paths @@ -347,20 +367,28 @@ fn json_string_to_db_type( json: Jsonb, element_type: ElementType, raw_flag: bool, + raw_text: bool, ) -> crate::Result { let mut json_string = json.to_string()?; - if raw_flag && matches!(element_type, ElementType::ARRAY | ElementType::OBJECT) { + if raw_flag { return Ok(OwnedValue::Blob(Rc::new(json.data()))); } match element_type { ElementType::ARRAY | ElementType::OBJECT => Ok(OwnedValue::Text(Text::json(json_string))), ElementType::TEXT | ElementType::TEXT5 | ElementType::TEXTJ | ElementType::TEXTRAW => { - json_string.remove(json_string.len() - 1); - json_string.remove(0); - Ok(OwnedValue::Text(Text { - value: Rc::new(json_string.into_bytes()), - subtype: TextSubtype::Text, - })) + if raw_text { + json_string.remove(json_string.len() - 1); + json_string.remove(0); + Ok(OwnedValue::Text(Text { + value: Rc::new(json_string.into_bytes()), + subtype: TextSubtype::Json, + })) + } else { + Ok(OwnedValue::Text(Text { + value: Rc::new(json_string.into_bytes()), + subtype: TextSubtype::Text, + })) + } } ElementType::FLOAT5 | ElementType::FLOAT => Ok(OwnedValue::Float( json_string.parse().expect("Should be valid f64"), @@ -439,13 +467,13 @@ pub fn json_type(value: &OwnedValue, path: Option<&OwnedValue>) -> crate::Result return Ok(OwnedValue::Null); } if path.is_none() { - let json = convert_dbtype_to_jsonb(value)?; + let json = convert_dbtype_to_jsonb(value, true)?; let element_type = json.is_valid()?; return Ok(OwnedValue::Text(Text::json(element_type.into()))); } if let Some(path) = json_path_from_owned_value(path.unwrap(), true)? { - let json = convert_dbtype_to_jsonb(value)?; + let json = convert_dbtype_to_jsonb(value, true)?; if let Ok((_, element_type)) = json.get_by_path(&path) { Ok(OwnedValue::Text(Text::json(element_type.into()))) @@ -640,7 +668,7 @@ pub fn is_json_valid(json_value: &OwnedValue) -> OwnedValue { if matches!(json_value, OwnedValue::Null) { return OwnedValue::Null; } - convert_dbtype_to_jsonb(json_value) + convert_dbtype_to_jsonb(json_value, true) .map(|_| OwnedValue::Integer(1)) .unwrap_or(OwnedValue::Integer(0)) } diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index 452cd8eda..e92e82ba6 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -2247,24 +2247,40 @@ impl Program { state.registers[*dest] = json_patch(target, patch)?; } JsonFunc::JsonRemove => { - state.registers[*dest] = json_remove( + if let Ok(json) = json_remove( &state.registers[*start_reg..*start_reg + arg_count], - )?; + ) { + state.registers[*dest] = json; + } else { + state.registers[*dest] = OwnedValue::Null; + } } JsonFunc::JsonbRemove => { - state.registers[*dest] = jsonb_remove( + if let Ok(json) = jsonb_remove( &state.registers[*start_reg..*start_reg + arg_count], - )?; + ) { + state.registers[*dest] = json; + } else { + state.registers[*dest] = OwnedValue::Null; + } } JsonFunc::JsonReplace => { - state.registers[*dest] = json_replace( + if let Ok(json) = json_replace( &state.registers[*start_reg..*start_reg + arg_count], - )?; + ) { + state.registers[*dest] = json; + } else { + state.registers[*dest] = OwnedValue::Null; + } } JsonFunc::JsonbReplace => { - state.registers[*dest] = jsonb_replace( + if let Ok(json) = jsonb_replace( &state.registers[*start_reg..*start_reg + arg_count], - )?; + ) { + state.registers[*dest] = json; + } else { + state.registers[*dest] = OwnedValue::Null; + } } JsonFunc::JsonPretty => { let json_value = &state.registers[*start_reg];