Repository: projectsyn/reclass-rs (language: python)

Issue:
Improve error message for failed reference lookups
## Summary

We currently generate an error message containing the complete parameters object for failed reference lookups, cf. https://github.com/projectsyn/reclass-rs/blob/edd6767d709cb2a6f2954e35506eb768041067dc/src/types/value.rs#L717-L719

While this is manageable when using small test inventories, this generates a massive error message for real-world Project Syn inventories.

Python reclass uses the following error type for failed reference lookups: https://github.com/kapicorp/reclass/blob/856b34cb77811d665c6346883238d436ac5c4924/reclass/errors.py#L202-L210

## Acceptance criteria

* Adjust the reference lookup error message to be more concise
* Release reclass-rs

## Implementation ideas

* Try to mimic the Python reclass error message
* If necessary, track additional state during reference resolution


Discussion / hints:

Fix patch:
diff --git a/src/refs/mod.rs b/src/refs/mod.rs
index 61f6e66..34446e8 100644
--- a/src/refs/mod.rs
+++ b/src/refs/mod.rs
@@ -24,9 +24,37 @@ pub struct ResolveState {
     /// Recursion depth of the resolution (in number of calls to Token::resolve() for Token::Ref
     /// objects).
     depth: usize,
+    /// Parameters key which we're currently processing.
+    current_keys: Vec<String>,
 }
 
 impl ResolveState {
+    /// Pushes the provided index into the last element of current_keys as `[idx]`.
+    pub(crate) fn push_list_index(&mut self, idx: usize) {
+        let mut kcount = self.current_keys.len();
+        if kcount == 0 {
+            self.current_keys.push(String::new());
+            kcount = 1;
+        }
+        self.current_keys[kcount - 1].push_str(&format!("[{idx}]"));
+    }
+
+    /// Pushes mapping key into the `current_keys` list. If possible, the provided value is
+    /// formatted with `raw_string()`. Additionally, unprocessed `String` values are pushed as-is.
+    /// This function will return an error when it's called with a `Value::ValueList`.
+    pub(crate) fn push_mapping_key(&mut self, key: &Value) -> Result<()> {
+        let kstr = match key.raw_string() {
+            Ok(s) => s,
+            Err(_) => match key {
+                Value::String(s) => Ok(s.clone()),
+                Value::ValueList(_) => Err(anyhow!("Unable to render ValueList as key segment")),
+                _ => unreachable!("raw_string() implemented for other Value variants"),
+            }?,
+        };
+        self.current_keys.push(kstr);
+        Ok(())
+    }
+
     /// Formats paths that have been seen as a comma-separated list.
     fn seen_paths_list(&self) -> String {
         let mut paths = self
@@ -37,6 +65,45 @@ impl ResolveState {
         paths.sort();
         paths.join(", ")
     }
+
+    /// Formats current key by joining the segements with dots.
+    fn current_key(&self) -> String {
+        self.current_keys.join(".")
+    }
+
+    /// Renders a suitable error when a reference loop is detected.
+    fn render_reference_loop_error(&self) -> anyhow::Error {
+        let paths = self.seen_paths_list();
+        anyhow!("Detected reference loop with reference paths [{paths}].")
+    }
+
+    /// Renders a suitable error when the reference lookup depth limit is exceeded.
+    fn render_recursion_depth_error(&self) -> anyhow::Error {
+        let current_key = self.current_key();
+        let paths = self.seen_paths_list();
+        anyhow!(
+            "Token resolution exceeded recursion depth of {RESOLVE_MAX_DEPTH} for \
+            parameter '{current_key}'. We've seen the following reference paths: [{paths}].",
+        )
+    }
+
+    /// Renders a suitable error when the reference lookup fails due to a missing key.
+    fn render_missing_key_error(&self, path: &str, key: &str) -> anyhow::Error {
+        let current_key = self.current_key();
+        let r = format!("${{{path}}}");
+        anyhow!(
+            "lookup error for reference '{r}' in parameter '{current_key}': key '{key}' not found"
+        )
+    }
+
+    /// Renders a lookup error with the given message
+    fn render_lookup_error(&self, path: &str, key: &str, msg: &str) -> anyhow::Error {
+        let current_key = self.current_key();
+        let r = format!("${{{path}}}");
+        anyhow!(
+            "While looking up key '{key}' in reference '{r}' for parameter '{current_key}': {msg}"
+        )
+    }
 }
 
 /// Maximum allowed recursion depth for Token::resolve(). We're fairly conservative with the value,
@@ -114,11 +181,7 @@ impl Token {
                     // implementation. We abort at a recursion depth of 64, since it's quite
                     // unlikely that there's a legitimate case where we have a recursion depth of
                     // 64 when resolving references for a well formed inventory.
-                    let paths = state.seen_paths_list();
-                    return Err(anyhow!(
-                        "Token resolution exceeded recursion depth of {RESOLVE_MAX_DEPTH}. \
-                        We've seen the following reference paths: [{paths}].",
-                    ));
+                    return Err(state.render_recursion_depth_error());
                 }
                 // Construct flattened ref path by resolving any potential nested references in the
                 // Ref's Vec<Token>.
@@ -127,8 +190,7 @@ impl Token {
                 if state.seen_paths.contains(&path) {
                     // we've already seen this reference, so we know there's a loop, and can abort
                     // resolution.
-                    let paths = state.seen_paths_list();
-                    return Err(anyhow!("Reference loop with reference paths [{paths}]."));
+                    return Err(state.render_reference_loop_error());
                 }
                 state.seen_paths.insert(path.clone());
 
@@ -137,74 +199,60 @@ impl Token {
                 // we handle the first element separately, so we can establish a local mutable
                 // variable which we can update during the walk of the parameters Mapping.
                 let k0 = refpath_iter.next().unwrap();
-                let k0: Value = k0.into();
                 // v is the value which we update to point to the next value as we recursively
                 // descend into the params Mapping
                 let mut v = params
-                    .get(&k0)
-                    .ok_or_else(|| anyhow!("[k0] unable to lookup key '{k0}' for '{path}'"))?;
+                    .get(&k0.into())
+                    .ok_or_else(|| state.render_missing_key_error(&path, k0))?;
 
                 // newv is used to hold temporary Values generated by interpolating v
                 let mut newv;
 
                 // traversed is used to keep track of the path segments we've already processed
-                let mut traversed = vec![k0.as_str().unwrap().to_string()];
+                let mut traversed = vec![k0.to_string()];
 
                 // descend into the params Mapping, looking up each segment of the reference path
                 // sequentially, updating `v` and `newv` as we go.
                 for key in refpath_iter {
-                    match v {
+                    // For lookups into Strings and ValueLists, we locally interpolate the
+                    // value into `newv` so we don't have to worry about the order in which
+                    // individual references are resolved, and always do value lookups on
+                    // resolved references.
+                    newv = interpolate_string_or_valuelist(v, params, state)?;
+                    // at this point, newv should never be a Value::String or Value::ValueList.
+                    debug_assert!(!newv.is_string() && !newv.is_value_list());
+                    // Do lookup in interpolated value, return error if interpolated value doesn't
+                    // support lookups.
+                    match newv {
                         // trivial case: v is a Mapping, we can just lookup the next value based
                         // on `key`.
                         Value::Mapping(_) => {
-                            v = v.get(&key.into()).ok_or_else(|| {
-                                anyhow!("unable to lookup key '{key}' for '{path}'")
-                            })?;
+                            v = newv
+                                .get(&key.into())
+                                .ok_or_else(|| state.render_missing_key_error(&path, key))?;
                         }
                         // Sequence lookups aren't supported by Python Reclass. We may implement
                         // them in the future.
                         Value::Sequence(_) => {
-                            return Err(anyhow!(
-                                "While looking up {key} for '{path}': \
-                                Sequence lookups aren't supported for Reclass references!"
-                            ))
-                        }
-                        // For lookups into Strings and ValueLists, we locally interpolate the
-                        // value into `newv` so we don't have to worry about the order in which
-                        // individual references are resolved, and always do value lookups on
-                        // resolved references.
-                        Value::String(_) => {
-                            newv = v.interpolate(params, state)?;
-                            v = newv.get(&key.into()).ok_or_else(|| {
-                                anyhow!("unable to lookup key '{key}' for '{path}'")
-                            })?;
-                        }
-                        Value::ValueList(l) => {
-                            let mut i = vec![];
-                            for v in l {
-                                // When resolving references in ValueLists, we want to track state
-                                // separately for each layer, since reference loops can't be
-                                // stretched across layers.
-                                let mut st = state.clone();
-                                let v = if v.is_string() {
-                                    v.interpolate(params, &mut st)?
-                                } else {
-                                    v.clone()
-                                };
-                                i.push(v);
-                            }
-                            newv = Value::ValueList(i).flattened()?;
-                            v = newv.get(&key.into()).ok_or_else(|| {
-                                anyhow!("unable to lookup key '{key}' for '{path}'")
-                            })?;
+                            return Err(state.render_lookup_error(
+                                &path,
+                                key,
+                                "Sequence lookups aren't supported for Reclass references!",
+                            ));
                         }
+                        Value::String(_) | Value::ValueList(_) => unreachable!(
+                            "We should have rendered Value::String and Value::ValueList into some other variant"
+                        ),
                         // A lookup into any other Value variant is an error
                         _ => {
-                            return Err(anyhow!(
-                                "While looking up {key} for '{path}': \
-                                    Can't continue lookup, {} is a {}",
-                                traversed.join(":"),
-                                v.variant()
+                            return Err(state.render_lookup_error(
+                                &path,
+                                key,
+                                &format!(
+                                    "Can't continue lookup, {} is a {}",
+                                    traversed.join(":"),
+                                    newv.variant()
+                                ),
                             ));
                         }
                     }
@@ -274,6 +322,39 @@ fn interpolate_token_slice(
     Ok(res)
 }
 
+fn interpolate_string_or_valuelist(
+    v: &Value,
+    params: &Mapping,
+    state: &mut ResolveState,
+) -> Result<Value> {
+    match v {
+        // For Value::String, we can simply call `interpolate()` on the value.
+        Value::String(_) => v.interpolate(params, state),
+        // For Value::ValueList, we interpolate each layer, and flatten the resulting layers into a
+        // single Value.  We don't use `interpolate()` here, since we only want to flatten the
+        // resulting ValueList here.
+        Value::ValueList(l) => {
+            let mut i = vec![];
+            for v in l {
+                // When resolving references in ValueLists, we want to track state
+                // separately for each layer, since reference loops can't be
+                // stretched across layers.
+                let mut st = state.clone();
+                let v = if v.is_string() {
+                    v.interpolate(params, &mut st)?
+                } else {
+                    v.clone()
+                };
+                i.push(v);
+            }
+            // Finally we flatten the resulting ValueList into a single Value.
+            Value::ValueList(i).flattened()
+        }
+        // Do nothing for other types
+        _ => Ok(v.clone()),
+    }
+}
+
 #[derive(Debug)]
 /// Wraps errors generated when trying to parse a string which may contain Reclass references
 pub struct ParseError<'a> {
diff --git a/src/types/mapping.rs b/src/types/mapping.rs
index 5f0096a..05ec0f0 100644
--- a/src/types/mapping.rs
+++ b/src/types/mapping.rs
@@ -387,6 +387,7 @@ impl Mapping {
             // either manage to interpolate a value (in which case it doesn't contain a loop) or we
             // don't and the whole interpolation is aborted.
             let mut st = state.clone();
+            st.push_mapping_key(k)?;
             let mut v = v.interpolate(root, &mut st)?;
             v.flatten()?;
             // Propagate key properties to the resulting mapping by using `insert_impl()`.
diff --git a/src/types/value.rs b/src/types/value.rs
index 46087f9..1c00270 100644
--- a/src/types/value.rs
+++ b/src/types/value.rs
@@ -547,13 +547,14 @@ impl Value {
             Self::Sequence(s) => {
                 // Sequences are interpolated by calling interpolate() for each element.
                 let mut seq = vec![];
-                for it in s {
+                for (idx, it) in s.iter().enumerate() {
                     // References in separate entries in sequences can't form loops. Therefore we
                     // pass a copy of the current resolution state to the recursive call for each
                     // element. We don't need to update the input state after we're done with a
                     // Sequence either, since there's no potential to start recursing again, if
                     // we've fully interpolated a Sequence.
                     let mut st = state.clone();
+                    st.push_list_index(idx);
                     let e = it.interpolate(root, &mut st)?;
                     seq.push(e);
                 }
@@ -716,7 +717,7 @@ impl Value {
         let mut state = ResolveState::default();
         let mut v = self
             .interpolate(root, &mut state)
-            .map_err(|e| anyhow!("While resolving references in {self}: {e}"))?;
+            .map_err(|e| anyhow!("While resolving references: {e}"))?;
         v.flatten()?;
         Ok(v)
     }


Test patch:
diff --git a/src/refs/token_resolve_parse_tests.rs b/src/refs/token_resolve_parse_tests.rs
index c6d7c15..f12e37d 100644
--- a/src/refs/token_resolve_parse_tests.rs
+++ b/src/refs/token_resolve_parse_tests.rs
@@ -222,7 +222,7 @@ fn test_resolve_mapping_embedded() {
 }
 
 #[test]
-#[should_panic(expected = "Reference loop with reference paths [\"foo\"].")]
+#[should_panic(expected = "Detected reference loop with reference paths [\"foo\"].")]
 fn test_resolve_recursive_error() {
     let p = r#"
     foo: ${foo}
@@ -235,7 +235,7 @@ fn test_resolve_recursive_error() {
 }
 
 #[test]
-#[should_panic(expected = "Reference loop with reference paths [\"bar\", \"foo\"].")]
+#[should_panic(expected = "Detected reference loop with reference paths [\"bar\", \"foo\"].")]
 fn test_resolve_recursive_error_2() {
     let p = r#"
     foo: ${bar}
@@ -249,7 +249,7 @@ fn test_resolve_recursive_error_2() {
 }
 
 #[test]
-#[should_panic(expected = "Reference loop with reference paths [\"baz\", \"foo\"].")]
+#[should_panic(expected = "Detected reference loop with reference paths [\"baz\", \"foo\"].")]
 fn test_resolve_nested_recursive_error() {
     let p = r#"
     foo: ${baz}
diff --git a/src/types/value/value_interpolate_tests.rs b/src/types/value/value_interpolate_tests.rs
index 19bbc4c..a2db083 100644
--- a/src/types/value/value_interpolate_tests.rs
+++ b/src/types/value/value_interpolate_tests.rs
@@ -469,9 +469,9 @@ fn test_interpolate_nested_mapping_no_loop() {
 }
 
 #[test]
-#[should_panic(expected = "While resolving references in \
-    {\"foo\": {\"bar\": \"${bar}\"}, \"bar\": [{\"baz\": \"baz\", \"qux\": \"qux\"}, \
-    {\"baz\": \"${foo}\"}]}: Reference loop with reference paths [\"bar\", \"foo\"].")]
+#[should_panic(
+    expected = "While resolving references: Detected reference loop with reference paths [\"bar\", \"foo\"]."
+)]
 fn test_merge_interpolate_loop() {
     let base = r#"
     foo:
@@ -496,10 +496,8 @@ fn test_merge_interpolate_loop() {
 }
 
 #[test]
-#[should_panic(expected = "While resolving references in \
-     {\"foo\": {\"bar\": [\"${bar}\", \"${baz}\"]}, \"bar\": \"${qux}\", \
-     \"baz\": {\"bar\": \"${foo}\"}, \"qux\": 3.14}: \
-    Reference loop with reference paths [\"baz\", \"foo\"].")]
+#[should_panic(expected = "While resolving references: \
+    Detected reference loop with reference paths [\"baz\", \"foo\"].")]
 fn test_interpolate_sequence_loop() {
     let base = r#"
     foo:
@@ -518,11 +516,8 @@ fn test_interpolate_sequence_loop() {
 }
 
 #[test]
-#[should_panic(expected = "While resolving references in \
-    {\"foo\": {\"bar\": {\"baz\": \"${foo:baz:bar}\", \"qux\": \"${foo:qux:foo}\"}, \
-    \"baz\": {\"bar\": \"qux\", \"qux\": \"${foo:bar:qux}\"}, \"qux\": \
-    {\"foo\": \"${foo:baz:qux}\"}}}: \
-    Reference loop with reference paths [\"foo:bar:qux\", \"foo:baz:qux\", \"foo:qux:foo\"].")]
+#[should_panic(expected = "While resolving references: \
+    Detected reference loop with reference paths [\"foo:bar:qux\", \"foo:baz:qux\", \"foo:qux:foo\"]")]
 fn test_interpolate_nested_mapping_loop() {
     let m = r#"
     foo:
@@ -543,16 +538,8 @@ fn test_interpolate_nested_mapping_loop() {
 
 #[test]
 #[should_panic(
-    expected = "While resolving references in \"${foo:${foo:${foo:${foo:${foo:\
-        ${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:\
-        ${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:\
-        ${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:\
-        ${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:\
-        ${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:${foo:\
-        ${foo:${foo:${foo:${foo:${foo:${foo}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}\
-        }}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}\": \
-        Token resolution exceeded recursion depth of 64. \
-        We've seen the following reference paths: []."
+    expected = "While resolving references: Token resolution exceeded recursion depth of 64 \
+    for parameter 'baz'. We've seen the following reference paths: []."
 )]
 fn test_interpolate_depth_exceeded() {
     // construct a reference string which is a nested sequence of ${foo:....${foo}} with 70 nesting
@@ -560,11 +547,12 @@ fn test_interpolate_depth_exceeded() {
     // recursion limit before we even manage to construct the initial ref path in
     // `Token::resolve()`.
     let refstr = (0..70).fold("${foo}".to_string(), |s, _| format!("${{foo:{s}}}"));
-    let map = (0..70).fold(Mapping::from_str("foo: bar").unwrap(), |m, _| {
+    let mut map = (0..70).fold(Mapping::from_str("foo: bar").unwrap(), |m, _| {
         let mut n = Mapping::new();
         n.insert("foo".into(), Value::Mapping(m)).unwrap();
         n
     });
-    let v = Value::from(refstr);
-    v.rendered(&map).unwrap();
+    map.insert("baz".into(), refstr.into()).unwrap();
+    let mut v = Value::from(map);
+    v.render_with_self().unwrap();
 }