Repository: cobalt-org/liquid-rust
Pull request #352 (2019-08-31 13:59:47)

Feature/standard filter conformance
- [ ] Tests created for any new feature or regression tests for bugfixes.

* This PR should fix issues #262, #333, #334, #335, #257, #250.
* It also partially fixes issue #246. 
* The macro introduced here (`as_sequence`) should help fixing #246, #255, #256, #266 (and maybe others). It helps writing filters that take a value and "coerce" it into an array the same way Ruby's [InputIterator](https://github.com/Shopify/liquid/blob/98dfe198e168eef580345842ff49c4f2549e238b/lib/liquid/standardfilters.rb#L457) does.

I know this is a heavy PR, but the issues fixed here are very closely related.


epage: I'm curious, where is it you are using liquid?
or17191: I'm using it to learn rust.
or17191: > I'm curious, where is it you are using liquid?

I'm using it to learn rust.
epage: > I'm using it to learn rust.

I'd say you are doing a great job! If there is a specific area you want more experience with, I can find some issue that would be more targeted.

Patch:
diff --git a/src/filters/std/array.rs b/src/filters/std/array.rs
index 34c9daa..3db0f23 100644
--- a/src/filters/std/array.rs
+++ b/src/filters/std/array.rs
@@ -4,9 +4,20 @@ use liquid_derive::*;
 use liquid_error::Result;
 use liquid_interpreter::Context;
 use liquid_interpreter::Expression;
-use liquid_value::{Object, Scalar, Value};
+use liquid_value::{Scalar, Value};
 use std::cmp;
 
+macro_rules! as_sequence {
+    ($value: expr, |$c:ident| $e:expr) => {
+        #[allow(clippy::redundant_closure_call)] // Clippy is angry about IIFE
+        match &$value {
+            Value::Array(array) => (|$c: std::slice::Iter<'_, Value>| $e)(array.iter()),
+            Value::Nil => (|$c: std::iter::Empty<&Value>| $e)(std::iter::empty()),
+            value => (|$c: std::iter::Once<&Value>| $e)(std::iter::once(value)),
+        }
+    };
+}
+
 #[derive(Debug, FilterParameters)]
 struct JoinArgs {
     #[parameter(
@@ -47,28 +58,84 @@ impl Filter for JoinFilter {
     }
 }
 
+fn nil_safe_compare(a: &Value, b: &Value) -> Option<cmp::Ordering> {
+    match (a, b) {
+        (Value::Nil, Value::Nil) => Some(cmp::Ordering::Equal),
+        (Value::Nil, _) => Some(cmp::Ordering::Greater),
+        (_, Value::Nil) => Some(cmp::Ordering::Less),
+        (a, b) => a.partial_cmp(b),
+    }
+}
+
+fn nil_safe_casecmp_key(value: &Value) -> Option<String> {
+    match value {
+        Value::Nil => None,
+        value => Some(value.to_str().to_lowercase()),
+    }
+}
+
+fn nil_safe_casecmp(a: &Option<String>, b: &Option<String>) -> Option<cmp::Ordering> {
+    match (a, b) {
+        (None, None) => Some(cmp::Ordering::Equal),
+        (None, _) => Some(cmp::Ordering::Greater),
+        (_, None) => Some(cmp::Ordering::Less),
+        (a, b) => a.partial_cmp(b),
+    }
+}
+
+#[derive(Debug, Default, FilterParameters)]
+struct PropertyArgs {
+    #[parameter(description = "The property accessed by the filter.", arg_type = "str")]
+    property: Option<Expression>,
+}
+
 #[derive(Clone, ParseFilter, FilterReflection)]
 #[filter(
     name = "sort",
     description = "Sorts items in an array. The order of the sorted array is case-sensitive.",
+    parameters(PropertyArgs),
     parsed(SortFilter)
 )]
 pub struct Sort;
 
-#[derive(Debug, Default, Display_filter)]
+#[derive(Debug, Default, FromFilterParameters, Display_filter)]
 #[name = "sort"]
-struct SortFilter;
+struct SortFilter {
+    #[parameters]
+    args: PropertyArgs,
+}
+
+fn safe_property_getter<'a>(value: &'a Value, property: &str) -> &'a Value {
+    value
+        .as_object()
+        .and_then(|obj| obj.get(property))
+        .unwrap_or(&Value::Nil)
+}
 
 impl Filter for SortFilter {
-    fn evaluate(&self, input: &Value, _context: &Context) -> Result<Value> {
-        // TODO(#333) optional property parameter
+    fn evaluate(&self, input: &Value, context: &Context) -> Result<Value> {
+        let args = self.args.evaluate(context)?;
 
-        let array = input
-            .as_array()
-            .ok_or_else(|| invalid_input("Array expected"))?;
-        let mut sorted = array.clone();
-        sorted.sort_by(|a, b| a.partial_cmp(b).unwrap_or(cmp::Ordering::Equal));
-        Ok(Value::array(sorted))
+        as_sequence!(input, |input| {
+            if args.property.is_some() && !input.clone().all(Value::is_object) {
+                return Err(invalid_input("Array of objects expected"));
+            }
+
+            let mut sorted: Vec<Value> = input.cloned().collect();
+            if let Some(property) = &args.property {
+                // Using unwrap is ok since all of the elements are objects
+                sorted.sort_by(|a, b| {
+                    nil_safe_compare(
+                        safe_property_getter(a, property),
+                        safe_property_getter(b, property),
+                    )
+                    .unwrap_or(cmp::Ordering::Equal)
+                });
+            } else {
+                sorted.sort_by(|a, b| nil_safe_compare(a, b).unwrap_or(cmp::Ordering::Equal));
+            }
+            Ok(Value::array(sorted))
+        })
     }
 }
 
@@ -76,28 +143,45 @@ impl Filter for SortFilter {
 #[filter(
     name = "sort_natural",
     description = "Sorts items in an array.",
+    parameters(PropertyArgs),
     parsed(SortNaturalFilter)
 )]
 pub struct SortNatural;
 
-#[derive(Debug, Default, Display_filter)]
+#[derive(Debug, Default, FromFilterParameters, Display_filter)]
 #[name = "sort_natural"]
-struct SortNaturalFilter;
+struct SortNaturalFilter {
+    #[parameters]
+    args: PropertyArgs,
+}
 
 impl Filter for SortNaturalFilter {
-    fn evaluate(&self, input: &Value, _context: &Context) -> Result<Value> {
-        // TODO(#334) optional property parameter
+    fn evaluate(&self, input: &Value, context: &Context) -> Result<Value> {
+        let args = self.args.evaluate(context)?;
 
-        let array = input
-            .as_array()
-            .ok_or_else(|| invalid_input("Array expected"))?;
-        let mut sorted: Vec<_> = array
-            .iter()
-            .map(|v| (v.to_str().to_lowercase(), v.clone()))
-            .collect();
-        sorted.sort_by(|a, b| a.partial_cmp(b).unwrap_or(cmp::Ordering::Equal));
-        let result: Vec<_> = sorted.into_iter().map(|(_, v)| v).collect();
-        Ok(Value::array(result))
+        as_sequence!(input, |input| {
+            if args.property.is_some() && !input.clone().all(Value::is_object) {
+                return Err(invalid_input("Array of objects expected"));
+            }
+
+            let mut sorted: Vec<_> = if let Some(property) = &args.property {
+                input
+                    .map(|v| {
+                        (
+                            nil_safe_casecmp_key(safe_property_getter(v, property)),
+                            v.clone(),
+                        )
+                    })
+                    .collect()
+            } else {
+                input
+                    .map(|v| (nil_safe_casecmp_key(v), v.clone()))
+                    .collect()
+            };
+            sorted.sort_by(|a, b| nil_safe_casecmp(&a.0, &b.0).unwrap_or(cmp::Ordering::Equal));
+            let result: Vec<_> = sorted.into_iter().map(|(_, v)| v).collect();
+            Ok(Value::array(result))
+        })
     }
 }
 
@@ -129,56 +213,46 @@ struct WhereFilter {
     args: WhereArgs,
 }
 
-fn apply_where_filter<'a, T: Iterator<Item = &'a Object>>(
-    input: T,
-    property: &str,
-    target_value: Option<&Value>,
-) -> Vec<Value> {
-    match target_value {
-        None => input
-            .filter(|object| object.get(property).map_or(false, Value::is_truthy))
-            .map(|object| Value::Object(object.clone()))
-            .collect(),
-        Some(target_value) => input
-            .filter(|object| {
-                object
-                    .get(property)
-                    .map_or(false, |value| value == target_value)
-            })
-            .map(|object| Value::Object(object.clone()))
-            .collect(),
-    }
-}
-
 impl Filter for WhereFilter {
     fn evaluate(&self, input: &Value, context: &Context) -> Result<Value> {
         let args = self.args.evaluate(context)?;
+        let property: &str = &args.property;
+        let target_value: Option<&Value> = args.target_value;
 
-        match input {
+        match &input {
             Value::Array(array) => {
-                if array.is_empty() {
-                    // Retrun an empty array only if we got an empty array
-                    return Ok(Value::array(vec![]));
-                }
                 if !array.iter().all(Value::is_object) {
                     return Ok(Value::Nil);
                 }
-                Ok(Value::array(apply_where_filter(
-                    array.iter().filter_map(Value::as_object),
-                    &args.property,
-                    args.target_value,
-                )))
             }
-            // In the case of a single object, take it, but return an array.
-            Value::Object(object) => Ok(Value::array(apply_where_filter(
-                std::iter::once(object),
-                &args.property,
-                args.target_value,
-            ))),
-            _ => Err(invalid_input(
-                "Array of objects or a single object expected",
-            )),
-        }
+            Value::Object(_) => (),
+            _ => {
+                return Err(invalid_input(
+                    "Array of objects or a single object expected",
+                ));
+            }
+        };
+
+        as_sequence!(input, |input| {
+            let array: Vec<_> = match target_value {
+                None => input
+                    .filter_map(Value::as_object)
+                    .filter(|object| object.get(property).map_or(false, Value::is_truthy))
+                    .map(|object| Value::Object(object.clone()))
+                    .collect(),
+                Some(target_value) => input
+                    .filter_map(Value::as_object)
+                    .filter(|object| {
+                        object
+                            .get(property)
+                            .as_ref()
+                            .map_or(false, |value| value == &target_value)
+                    })
+                    .map(|object| Value::Object(object.clone()))
+                    .collect(),
+            };
+            Ok(Value::array(array))
+        })
     }
 }
 
@@ -284,23 +358,43 @@ impl Filter for MapFilter {
 #[filter(
     name = "compact",
     description = "Remove nulls from an iterable.",
+    parameters(PropertyArgs),
     parsed(CompactFilter)
 )]
 pub struct Compact;
 
-#[derive(Debug, Default, Display_filter)]
+#[derive(Debug, Default, FromFilterParameters, Display_filter)]
 #[name = "compact"]
-struct CompactFilter;
+struct CompactFilter {
+    #[parameters]
+    args: PropertyArgs,
+}
 
 impl Filter for CompactFilter {
-    fn evaluate(&self, input: &Value, _context: &Context) -> Result<Value> {
+    fn evaluate(&self, input: &Value, context: &Context) -> Result<Value> {
+        let args = self.args.evaluate(context)?;
+
         let array = input
             .as_array()
             .ok_or_else(|| invalid_input("Array expected"))?;
 
-        // TODO(#335) optional property parameter
-
-        let result: Vec<_> = array.iter().filter(|v| !v.is_nil()).cloned().collect();
+        let result: Vec<_> = if let Some(property) = &args.property {
+            if !array.iter().all(Value::is_object) {
+                return Err(invalid_input("Array of objects expected"));
+            }
+            // Reject non objects that don't have the required property
+            array
+                .iter()
+                .filter(|v| {
+                    !v.as_object()
+                        .and_then(|obj| obj.get(property.as_ref()))
+                        .map_or(true, Value::is_nil)
+                })
+                .cloned()
+                .collect()
+        } else {
+            array.iter().filter(|v| !v.is_nil()).cloned().collect()
+        };
 
         Ok(Value::array(result))
     }
@@ -373,7 +467,7 @@ impl Filter for FirstFilter {
                     .unwrap_or_else(|| "".to_owned());
                 Ok(Value::scalar(c))
             }
-            Value::Array(ref x) => Ok(x.first().cloned().unwrap_or_else(|| Value::scalar(""))),
+            Value::Array(ref x) => Ok(x.first().cloned().unwrap_or_else(|| Value::Nil)),
             _ => Err(invalid_input("String or Array expected")),
         }
     }
@@ -403,7 +497,7 @@ impl Filter for LastFilter {
                     .unwrap_or_else(|| "".to_owned());
                 Ok(Value::scalar(c))
             }
-            Value::Array(ref x) => Ok(x.last().cloned().unwrap_or_else(|| Value::scalar(""))),
+            Value::Array(ref x) => Ok(x.last().cloned().unwrap_or_else(|| Value::Nil)),
             _ => Err(invalid_input("String or Array expected")),
         }
     }
@@ -540,7 +634,7 @@ mod tests {
             unit!(First, Value::Array(vec![tos!("test"), tos!("two")])),
             tos!("test")
         );
-        assert_eq!(unit!(First, Value::Array(vec![])), tos!(""));
+        assert_eq!(unit!(First, Value::Array(vec![])), Value::Nil);
     }
 
     #[test]
@@ -606,7 +700,7 @@ mod tests {
             unit!(Last, Value::Array(vec![tos!("test"), tos!("last")])),
             tos!("last")
         );
-        assert_eq!(unit!(Last, Value::Array(vec![])), tos!(""));
+        assert_eq!(unit!(Last, Value::Array(vec![])), Value::Nil);
     }
 
     #[test]


Test patch:
diff --git a/tests/conformance_ruby/filter_test.rs b/tests/conformance_ruby/filter_test.rs
index 8ee9af8..f05680c 100644
--- a/tests/conformance_ruby/filter_test.rs
+++ b/tests/conformance_ruby/filter_test.rs
@@ -113,7 +113,6 @@ fn test_join() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#250
 fn test_sort() {
     let assigns = v!({
         "value": 3,
@@ -135,7 +134,6 @@ fn test_sort() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#249
 fn test_sort_natural() {
     let assigns = v!({
         "words": ["case", "Assert", "Insensitive"],
@@ -161,7 +159,6 @@ fn test_sort_natural() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#246
 fn test_compact() {
     let assigns = v!({
         "words": ["a", nil, "b", nil, "c"],
diff --git a/tests/conformance_ruby/standard_filter_test.rs b/tests/conformance_ruby/standard_filter_test.rs
index 098f832..1fd6575 100644
--- a/tests/conformance_ruby/standard_filter_test.rs
+++ b/tests/conformance_ruby/standard_filter_test.rs
@@ -210,7 +210,6 @@ fn test_join() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_sort() {
     assert_eq!(v!([1, 2, 3, 4]), filters!(Sort, v!([4, 3, 2, 1])));
     assert_eq!(
@@ -224,7 +223,6 @@ fn test_sort() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#262
 fn test_sort_with_nils() {
     assert_eq!(v!([1, 2, 3, 4, nil]), filters!(Sort, v!([nil, 4, 3, 2, 1])));
     assert_eq!(
@@ -238,7 +236,6 @@ fn test_sort_with_nils() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_sort_when_property_is_sometimes_missing_puts_nils_last() {
     let input = v!([
       { "price": 4, "handle": "alpha" },
@@ -247,18 +244,28 @@ fn test_sort_when_property_is_sometimes_missing_puts_nils_last() {
       { "handle": "delta" },
       { "price": 2, "handle": "epsilon" }
     ]);
-    let expectation = v!([
-      { "price": 1, "handle": "gamma" },
-      { "price": 2, "handle": "epsilon" },
-      { "price": 4, "handle": "alpha" },
-      { "handle": "delta" },
-      { "handle": "beta" }
-    ]);
-    assert_eq!(expectation, filters!(Sort, input, v!("price")));
+    let expectation_start = vec![
+        v!({ "price": 1, "handle": "gamma" }),
+        v!({ "price": 2, "handle": "epsilon" }),
+        v!({ "price": 4, "handle": "alpha" }),
+    ];
+    // Those two results are viable, since both values "map" to nil.
+    // Since the sorting is unstable, the order between the results is undefined.
+    // The original test (Ruby implementation) tests for only one of the results,
+    // which, I think, was the one produced by their specific implementation.
+    let expectation_end = (
+        vec![v!({ "handle": "delta" }), v!({ "handle": "beta" })],
+        vec![v!({ "handle": "beta" }), v!({ "handle": "delta" })],
+    );
+    let result = filters!(Sort, input, v!("price"));
+    let result = result.into_array();
+    assert!(result.is_some());
+    let result = result.unwrap();
+    assert_eq!(expectation_start, &result[..3]);
+    assert!(expectation_end.0 == &result[3..] || expectation_end.1 == &result[3..]);
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_sort_natural() {
     assert_eq!(
         v!(["a", "B", "c", "D"]),
@@ -275,7 +282,6 @@ fn test_sort_natural() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#262
 fn test_sort_natural_with_nils() {
     assert_eq!(
         v!(["a", "B", "c", "D", nil]),
@@ -292,7 +298,6 @@ fn test_sort_natural_with_nils() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_sort_natural_when_property_is_sometimes_missing_puts_nils_last() {
     let input = v!([
       { "price": "4", "handle": "alpha" },
@@ -301,18 +306,28 @@ fn test_sort_natural_when_property_is_sometimes_missing_puts_nils_last() {
       { "handle": "delta" },
       { "price": 2, "handle": "epsilon" }
     ]);
-    let expectation = v!([
-      { "price": "1", "handle": "gamma" },
-      { "price": 2, "handle": "epsilon" },
-      { "price": "4", "handle": "alpha" },
-      { "handle": "delta" },
-      { "handle": "beta" }
-    ]);
-    assert_eq!(expectation, filters!(SortNatural, input, v!("price")));
+    let expectation_start = vec![
+        v!({ "price": "1", "handle": "gamma" }),
+        v!({ "price": 2, "handle": "epsilon" }),
+        v!({ "price": "4", "handle": "alpha" }),
+    ];
+    // Those two results are viable, since both values "map" to nil.
+    // Since the sorting is unstable, the order between the results is undefined.
+    // The original test (Ruby implementation) tests for only one of the results,
+    // which, I think, was the one produced by their specific implementation.
+    let expectation_end = (
+        vec![v!({ "handle": "delta" }), v!({ "handle": "beta" })],
+        vec![v!({ "handle": "beta" }), v!({ "handle": "delta" })],
+    );
+    let result = filters!(SortNatural, input, v!("price"));
+    let result = result.into_array();
+    assert!(result.is_some());
+    let result = result.unwrap();
+    assert_eq!(expectation_start, &result[..3]);
+    assert!(expectation_end.0 == &result[3..] || expectation_end.1 == &result[3..]);
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_sort_natural_case_check() {
     let input = v!([
       { "key": "X" },
@@ -340,19 +355,16 @@ fn test_sort_natural_case_check() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_sort_empty_array() {
     assert_eq!(v!([]), filters!(Sort, v!([]), v!("a")));
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_sort_natural_empty_array() {
     assert_eq!(v!([]), filters!(SortNatural, v!([]), v!("a")));
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_legacy_sort_hash() {
     assert_eq!(
         v!([{ "a": 1, "b": 2 }]),
@@ -361,7 +373,6 @@ fn test_legacy_sort_hash() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#257
 fn test_numerical_vs_lexicographical_sort() {
     assert_eq!(v!([2, 10]), filters!(Sort, v!([10, 2])));
     assert_eq!(
@@ -401,11 +412,17 @@ fn test_uniq_empty_array() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#335
 fn test_compact_empty_array() {
     assert_eq!(v!([]), filters!(Compact, v!([]), v!("a")));
 }
 
+#[test]
+fn test_compact_invalid_property() {
+    let input = v!([[1], [2], [3]]);
+
+    filters_fail!(Compact, input, v!("bar"));
+}
+
 #[test]
 fn test_reverse() {
     assert_eq!(v!([4, 3, 2, 1]), filters!(Reverse, v!([1, 2, 3, 4])));
@@ -593,7 +610,6 @@ fn test_date() {
 }
 
 #[test]
-#[should_panic] // liquid-rust#254
 fn test_first_last() {
     assert_eq!(v!(1), filters!(First, v!([1, 2, 3])));
     assert_eq!(v!(3), filters!(Last, v!([1, 2, 3])));
diff --git a/tests/filters.rs b/tests/filters.rs
index 1e23556..15a57e7 100644
--- a/tests/filters.rs
+++ b/tests/filters.rs
@@ -675,3 +675,23 @@ text: foo
     let output = template.render(&globals).unwrap();
     assert_eq!(output, "foo".to_string());
 }
+
+#[test]
+fn test_compact() {
+    let text = "{{hashes | compact: 'a' | map: 'a' | join}}";
+    let globals: liquid::value::Object = serde_yaml::from_str(
+        r#"
+words: ["a", null, "b", null, "c"]
+hashes: [{"a": "A"}, {"a": null}, {"a": "C"}, {}]
+"#,
+    )
+    .unwrap();
+
+    let template = liquid::ParserBuilder::with_liquid()
+        .build()
+        .unwrap()
+        .parse(text)
+        .unwrap();
+    let output = template.render(&globals).unwrap();
+    assert_eq!(output, "A C".to_string());
+}