From e575fe60fb37153853351774ab559140c66cdd59 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:06:28 -0600 Subject: [PATCH 1/8] test(set): Clarify test cases --- tests/testsuite/set.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/testsuite/set.rs b/tests/testsuite/set.rs index 4a02b5c0..3237f20f 100644 --- a/tests/testsuite/set.rs +++ b/tests/testsuite/set.rs @@ -65,20 +65,20 @@ fn test_set_scalar_path() { #[cfg(feature = "json")] fn test_set_arr_path() { let config = Config::builder() - .set_override("items[0].name", "Ivan") + .set_override("present[0].name", "Ivan") .unwrap() - .set_override("data[0].things[1].name", "foo") + .set_override("absent[0].things[1].name", "foo") .unwrap() - .set_override("data[0].things[1].value", 42) + .set_override("absent[0].things[1].value", 42) .unwrap() - .set_override("data[1]", 0) + .set_override("absent[1]", 0) .unwrap() - .set_override("items[2]", "George") + .set_override("present[2]", "George") .unwrap() .add_source(File::from_str( r#" { - "items": [ + "present": [ { "name": "1" }, @@ -93,14 +93,14 @@ fn test_set_arr_path() { .build() .unwrap(); - assert_eq!(config.get("items[0].name").ok(), Some("Ivan".to_owned())); + assert_eq!(config.get("present[0].name").ok(), Some("Ivan".to_owned())); assert_eq!( - config.get("data[0].things[1].name").ok(), + config.get("absent[0].things[1].name").ok(), Some("foo".to_owned()) ); - assert_eq!(config.get("data[0].things[1].value").ok(), Some(42)); - assert_eq!(config.get("data[1]").ok(), Some(0)); - assert_eq!(config.get("items[2]").ok(), Some("George".to_owned())); + assert_eq!(config.get("absent[0].things[1].value").ok(), Some(42)); + assert_eq!(config.get("absent[1]").ok(), Some(0)); + assert_eq!(config.get("present[2]").ok(), Some("George".to_owned())); } #[test] From 92c44a3b48bc4b23cc8931841c77af28ba7e4fdd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:08:59 -0600 Subject: [PATCH 2/8] test(set): Verify negative indexes in overrides --- tests/testsuite/set.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/testsuite/set.rs b/tests/testsuite/set.rs index 3237f20f..de8f563f 100644 --- a/tests/testsuite/set.rs +++ b/tests/testsuite/set.rs @@ -75,6 +75,10 @@ fn test_set_arr_path() { .unwrap() .set_override("present[2]", "George") .unwrap() + .set_override("reverse[-1]", "Bob") + .unwrap() + .set_override("reverse[-2]", "Alice") + .unwrap() .add_source(File::from_str( r#" { @@ -85,6 +89,14 @@ fn test_set_arr_path() { { "name": "2" } + ], + "reverse": [ + { + "name": "l1" + }, + { + "name": "l2" + } ] } "#, @@ -101,6 +113,8 @@ fn test_set_arr_path() { assert_eq!(config.get("absent[0].things[1].value").ok(), Some(42)); assert_eq!(config.get("absent[1]").ok(), Some(0)); assert_eq!(config.get("present[2]").ok(), Some("George".to_owned())); + assert_eq!(config.get("reverse[1]").ok(), Some("Bob".to_owned())); + assert_eq!(config.get("reverse[0]").ok(), Some("Alice".to_owned())); } #[test] From 4814f53f75cd8281c880fd2f229636fbceb7c567 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:15:00 -0600 Subject: [PATCH 3/8] test: Verify negative indices --- tests/testsuite/errors.rs | 47 +++++++++++++++++++++++++++++++++++++++ tests/testsuite/set.rs | 10 ++++++++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/errors.rs b/tests/testsuite/errors.rs index 983a3c61..ad6d1582 100644 --- a/tests/testsuite/errors.rs +++ b/tests/testsuite/errors.rs @@ -3,6 +3,53 @@ use snapbox::{assert_data_eq, str}; use config::{Config, ConfigError, File, FileFormat, Map, Value}; +#[test] +#[cfg(feature = "json")] +fn test_error_path_index_bounds() { + let c = Config::builder() + .add_source(File::from_str( + r#" +{ + "arr": [1] +} +"#, + FileFormat::Json, + )) + .build() + .unwrap(); + + let res = c.get::("arr[2]"); + assert!(res.is_err()); + assert_data_eq!( + res.unwrap_err().to_string(), + str![[r#"configuration property "arr[2]" not found"#]] + ); +} + +#[test] +#[cfg(feature = "json")] +#[should_panic] +fn test_error_path_index_negative_bounds() { + let c = Config::builder() + .add_source(File::from_str( + r#" +{ + "arr": [] +} +"#, + FileFormat::Json, + )) + .build() + .unwrap(); + + let res = c.get::("arr[-1]"); + assert!(res.is_err()); + assert_data_eq!( + res.unwrap_err().to_string(), + str![[r#"configuration property "arr[2]" not found"#]] + ); +} + #[test] #[cfg(feature = "json")] fn test_error_parse() { diff --git a/tests/testsuite/set.rs b/tests/testsuite/set.rs index de8f563f..41aa0366 100644 --- a/tests/testsuite/set.rs +++ b/tests/testsuite/set.rs @@ -63,6 +63,7 @@ fn test_set_scalar_path() { #[test] #[cfg(feature = "json")] +#[should_panic] fn test_set_arr_path() { let config = Config::builder() .set_override("present[0].name", "Ivan") @@ -79,6 +80,10 @@ fn test_set_arr_path() { .unwrap() .set_override("reverse[-2]", "Alice") .unwrap() + .set_override("empty[-1]", "Bob") + .unwrap() + .set_override("empty[-2]", "Alice") + .unwrap() .add_source(File::from_str( r#" { @@ -97,7 +102,8 @@ fn test_set_arr_path() { { "name": "l2" } - ] + ], + "empty": [] } "#, FileFormat::Json, @@ -115,6 +121,8 @@ fn test_set_arr_path() { assert_eq!(config.get("present[2]").ok(), Some("George".to_owned())); assert_eq!(config.get("reverse[1]").ok(), Some("Bob".to_owned())); assert_eq!(config.get("reverse[0]").ok(), Some("Alice".to_owned())); + assert_eq!(config.get("empty[1]").ok(), Some("Bob".to_owned())); + assert_eq!(config.get("empty[0]").ok(), Some("Alice".to_owned())); } #[test] From f013b6d21fa77b1af2106913dd257bbea4c3b76b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:17:21 -0600 Subject: [PATCH 4/8] fix(get): Don't crash on bad negative bounds --- src/path/mod.rs | 12 ++++++------ tests/testsuite/errors.rs | 3 +-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 3c0b4213..5ea64473 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -40,11 +40,11 @@ impl std::fmt::Display for ParseError { impl std::error::Error for ParseError {} -fn sindex_to_uindex(index: isize, len: usize) -> usize { +fn sindex_to_uindex(index: isize, len: usize) -> Option { if index >= 0 { - index as usize + Some(index as usize) } else { - len - index.unsigned_abs() + len.checked_sub(index.unsigned_abs()) } } @@ -80,7 +80,7 @@ impl Expression { Self::Subscript(expr, index) => match expr.get(root) { Some(value) => match value.kind { ValueKind::Array(ref array) => { - let index = sindex_to_uindex(index, array.len()); + let index = sindex_to_uindex(index, array.len())?; if index >= array.len() { None @@ -141,7 +141,7 @@ impl Expression { match value.kind { ValueKind::Array(ref mut array) => { - let index = sindex_to_uindex(index, array.len()); + let index = sindex_to_uindex(index, array.len())?; if index >= array.len() { array.resize(index + 1, Value::new(None, ValueKind::Nil)); @@ -216,7 +216,7 @@ impl Expression { } if let ValueKind::Array(ref mut array) = parent.kind { - let uindex = sindex_to_uindex(index, array.len()); + let uindex = sindex_to_uindex(index, array.len()).unwrap(); if uindex >= array.len() { array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); } diff --git a/tests/testsuite/errors.rs b/tests/testsuite/errors.rs index ad6d1582..74edd41a 100644 --- a/tests/testsuite/errors.rs +++ b/tests/testsuite/errors.rs @@ -28,7 +28,6 @@ fn test_error_path_index_bounds() { #[test] #[cfg(feature = "json")] -#[should_panic] fn test_error_path_index_negative_bounds() { let c = Config::builder() .add_source(File::from_str( @@ -46,7 +45,7 @@ fn test_error_path_index_negative_bounds() { assert!(res.is_err()); assert_data_eq!( res.unwrap_err().to_string(), - str![[r#"configuration property "arr[2]" not found"#]] + str![[r#"configuration property "arr[-1]" not found"#]] ); } From f51ae034095bfd3af3c04ca1004a975a18416259 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:24:09 -0600 Subject: [PATCH 5/8] refactor(path): Clarify index operation --- src/path/mod.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 5ea64473..145a70b0 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -40,7 +40,8 @@ impl std::fmt::Display for ParseError { impl std::error::Error for ParseError {} -fn sindex_to_uindex(index: isize, len: usize) -> Option { +/// Convert a relative index into an absolute index +fn abs_index(index: isize, len: usize) -> Option { if index >= 0 { Some(index as usize) } else { @@ -80,7 +81,7 @@ impl Expression { Self::Subscript(expr, index) => match expr.get(root) { Some(value) => match value.kind { ValueKind::Array(ref array) => { - let index = sindex_to_uindex(index, array.len())?; + let index = abs_index(index, array.len())?; if index >= array.len() { None @@ -141,7 +142,7 @@ impl Expression { match value.kind { ValueKind::Array(ref mut array) => { - let index = sindex_to_uindex(index, array.len())?; + let index = abs_index(index, array.len())?; if index >= array.len() { array.resize(index + 1, Value::new(None, ValueKind::Nil)); @@ -216,7 +217,7 @@ impl Expression { } if let ValueKind::Array(ref mut array) = parent.kind { - let uindex = sindex_to_uindex(index, array.len()).unwrap(); + let uindex = abs_index(index, array.len()).unwrap(); if uindex >= array.len() { array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); } From 0319e38d2dc413c25dcc94833cf77464ced1a50a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:28:21 -0600 Subject: [PATCH 6/8] refactor(path): Clarify index operation --- src/path/mod.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index 145a70b0..bc1cc53b 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -82,12 +82,7 @@ impl Expression { Some(value) => match value.kind { ValueKind::Array(ref array) => { let index = abs_index(index, array.len())?; - - if index >= array.len() { - None - } else { - Some(&array[index]) - } + array.get(index) } _ => None, From 3638cdcc5c825463b531f8656f7f91f3162a3fff Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:31:52 -0600 Subject: [PATCH 7/8] refactor(error): Allow overriding the default error --- src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/error.rs b/src/error.rs index 13e13b82..2d738b94 100644 --- a/src/error.rs +++ b/src/error.rs @@ -174,7 +174,7 @@ impl ConfigError { } /// Alias for a `Result` with the error type set to `ConfigError`. -pub(crate) type Result = result::Result; +pub(crate) type Result = result::Result; // Forward Debug to Display for readable panic! messages impl fmt::Debug for ConfigError { From da18a3689260041c1b8c96fa88b30d8230adb4bd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Dec 2024 09:36:05 -0600 Subject: [PATCH 8/8] fix(set): Dont crash on bad negative bounds Fixes #619 --- src/path/mod.rs | 31 ++++++++++++++++++++++--------- tests/testsuite/set.rs | 1 - 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/path/mod.rs b/src/path/mod.rs index bc1cc53b..e98e92a3 100644 --- a/src/path/mod.rs +++ b/src/path/mod.rs @@ -41,11 +41,13 @@ impl std::fmt::Display for ParseError { impl std::error::Error for ParseError {} /// Convert a relative index into an absolute index -fn abs_index(index: isize, len: usize) -> Option { +fn abs_index(index: isize, len: usize) -> Result { if index >= 0 { - Some(index as usize) + Ok(index as usize) + } else if let Some(index) = len.checked_sub(index.unsigned_abs()) { + Ok(index) } else { - len.checked_sub(index.unsigned_abs()) + Err((len as isize + index).unsigned_abs()) } } @@ -81,7 +83,7 @@ impl Expression { Self::Subscript(expr, index) => match expr.get(root) { Some(value) => match value.kind { ValueKind::Array(ref array) => { - let index = abs_index(index, array.len())?; + let index = abs_index(index, array.len()).ok()?; array.get(index) } @@ -137,7 +139,7 @@ impl Expression { match value.kind { ValueKind::Array(ref mut array) => { - let index = abs_index(index, array.len())?; + let index = abs_index(index, array.len()).ok()?; if index >= array.len() { array.resize(index + 1, Value::new(None, ValueKind::Nil)); @@ -212,10 +214,21 @@ impl Expression { } if let ValueKind::Array(ref mut array) = parent.kind { - let uindex = abs_index(index, array.len()).unwrap(); - if uindex >= array.len() { - array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); - } + let uindex = match abs_index(index, array.len()) { + Ok(uindex) => { + if uindex >= array.len() { + array.resize(uindex + 1, Value::new(None, ValueKind::Nil)); + } + uindex + } + Err(insertion) => { + array.splice( + 0..0, + (0..insertion).map(|_| Value::new(None, ValueKind::Nil)), + ); + 0 + } + }; array[uindex] = value; } diff --git a/tests/testsuite/set.rs b/tests/testsuite/set.rs index 41aa0366..c5d29de0 100644 --- a/tests/testsuite/set.rs +++ b/tests/testsuite/set.rs @@ -63,7 +63,6 @@ fn test_set_scalar_path() { #[test] #[cfg(feature = "json")] -#[should_panic] fn test_set_arr_path() { let config = Config::builder() .set_override("present[0].name", "Ivan")