Merge pull request #437 from sunng87/refacor/call_inner_provided

Refactor call_inner default implementation

CHANGELOG.md

@@ -9,15 +9,16 @@
 * [Changed] Updated `rhai` to 0.19 and then 0.20 [#391]
 * [Changed] `#each` helper now renders else block for non-iterable data [#380]
 * [Changed] `TemplateError` and `ScriptError` is now a cause of `RenderError` [#395]
-* [Changed] `RenderContext::get_partial` now returns `Option<&Template>`
-* [Changed] Capitalize names like `HtmlExpression` and `IoError` based on clippy recommendations [#424]
+* [Changed] **Breaking** `RenderContext::get_partial` now returns `Option<&Template>`
+* [Changed] **Breaking** Capitalize names like `HtmlExpression` and `IoError` based on clippy recommendations [#424]
+* [Changed] **Breaking** Improved return type of `call_inner` from `HelperDef` to avoid misleading [#437]
 * [Fixed] reference starts with `null`, `true` and `false` were parsed incorrectly [#382]
 * [Fixed] dir source path separator bug on windows [#389] [#405]
 * [Fixed] stack overflow with nested `@partial-block` [#401]
 * [Fixed] value access issue when upper block has a base value [#419]
 * [Fixed] escape rules for Json string literal [#423]
-* [Removed] option to disable source map is removed [#395]
-* [Removed] `TemplateFileError` and `TemplateRenderError` are removed and merged into
+* [Removed] **Breaking** option to disable source map is removed [#395]
+* [Removed] **Breaking** `TemplateFileError` and `TemplateRenderError` are removed and merged into
   `TemplateError` and `RenderError` [#395]
 
 ## [3.4.0](https://github.com/sunng87/handlebars-rust/compare/3.3.0...3.4.0) - 2020-08-14

src/error.rs

@@ -12,13 +12,14 @@ use walkdir::Error as WalkdirError;
 use rhai::{EvalAltResult, ParseError};
 
 /// Error when rendering data on template.
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub struct RenderError {
     pub desc: String,
     pub template_name: Option<String>,
     pub line_no: Option<usize>,
     pub column_no: Option<usize>,
     cause: Option<Box<dyn Error + Send + Sync + 'static>>,
+    unimplemented: bool,
 }
 
 impl fmt::Display for RenderError {
@@ -93,10 +94,14 @@ impl RenderError {
     pub fn new<T: AsRef<str>>(desc: T) -> RenderError {
         RenderError {
             desc: desc.as_ref().to_owned(),
-            template_name: None,
-            line_no: None,
-            column_no: None,
-            cause: None,
+            ..Default::default()
+        }
+    }
+
+    pub(crate) fn unimplemented() -> RenderError {
+        RenderError {
+            unimplemented: true,
+            ..Default::default()
         }
     }
 
@@ -117,6 +122,11 @@ impl RenderError {
 
         e
     }
+
+    #[inline]
+    pub(crate) fn is_unimplemented(&self) -> bool {
+        self.unimplemented
+    }
 }
 
 quick_error! {

src/helpers/helper_lookup.rs

@@ -17,7 +17,7 @@ impl HelperDef for LookupHelper {
         r: &'reg Registry<'reg>,
         _: &'rc Context,
         _: &mut RenderContext<'reg, 'rc>,
-    ) -> Result<Option<ScopedJson<'reg, 'rc>>, RenderError> {
+    ) -> Result<ScopedJson<'reg, 'rc>, RenderError> {
         let collection_value = h
             .param(0)
             .ok_or_else(|| RenderError::new("Param not found for helper \"lookup\""))?;
@@ -30,18 +30,18 @@ impl HelperDef for LookupHelper {
                 .value()
                 .as_u64()
                 .and_then(|u| v.get(u as usize))
-                .map(|i| ScopedJson::Derived(i.clone())),
+                .unwrap_or(&Json::Null),
             Json::Object(ref m) => index
                 .value()
                 .as_str()
                 .and_then(|k| m.get(k))
-                .map(|i| ScopedJson::Derived(i.clone())),
-            _ => None,
+                .unwrap_or(&Json::Null),
+            _ => &Json::Null,
         };
-        if r.strict_mode() && value.is_none() {
+        if r.strict_mode() && value.is_null() {
             Err(RenderError::strict_error(None))
         } else {
-            Ok(value)
+            Ok(value.clone().into())
         }
     }
 }

src/helpers/mod.rs

@@ -82,8 +82,8 @@ pub trait HelperDef {
         _: &'reg Registry<'reg>,
         _: &'rc Context,
         _: &mut RenderContext<'reg, 'rc>,
-    ) -> Result<Option<ScopedJson<'reg, 'rc>>, RenderError> {
-        Ok(None)
+    ) -> Result<ScopedJson<'reg, 'rc>, RenderError> {
+        Err(RenderError::unimplemented())
     }
 
     fn call<'reg: 'rc, 'rc>(
@@ -94,17 +94,26 @@ pub trait HelperDef {
         rc: &mut RenderContext<'reg, 'rc>,
         out: &mut dyn Output,
     ) -> HelperResult {
-        if let Some(result) = self.call_inner(h, r, ctx, rc)? {
-            if r.strict_mode() && result.is_missing() {
-                return Err(RenderError::strict_error(None));
-            } else {
-                // auto escape according to settings
-                let output = do_escape(r, rc, result.render());
-                out.write(output.as_ref())?;
+        match self.call_inner(h, r, ctx, rc) {
+            Ok(result) => {
+                if r.strict_mode() && result.is_missing() {
+                    Err(RenderError::strict_error(None))
+                } else {
+                    // auto escape according to settings
+                    let output = do_escape(r, rc, result.render());
+                    out.write(output.as_ref())?;
+                    Ok(())
+                }
+            }
+            Err(e) => {
+                if e.is_unimplemented() {
+                    // default implementation, do nothing
+                    Ok(())
+                } else {
+                    Err(e)
+                }
             }
         }
-
-        Ok(())
     }
 }
 

src/helpers/scripting.rs

@@ -22,7 +22,7 @@ fn call_script_helper<'reg: 'rc, 'rc>(
     hash: &BTreeMap<&'reg str, PathAndJson<'reg, 'rc>>,
     engine: &Engine,
     script: &AST,
-) -> Result<Option<ScopedJson<'reg, 'rc>>, RenderError> {
+) -> Result<ScopedJson<'reg, 'rc>, RenderError> {
     let params: Dynamic = to_dynamic(params.iter().map(|p| p.value()).collect::<Vec<&Json>>())?;
 
     let hash: Dynamic = to_dynamic(
@@ -41,7 +41,7 @@ fn call_script_helper<'reg: 'rc, 'rc>(
 
     let result_json: Json = from_dynamic(&result)?;
 
-    Ok(Some(ScopedJson::Derived(result_json)))
+    Ok(ScopedJson::Derived(result_json))
 }
 
 impl HelperDef for ScriptHelper {
@@ -51,7 +51,7 @@ impl HelperDef for ScriptHelper {
         reg: &'reg Registry<'reg>,
         _ctx: &'rc Context,
         _rc: &mut RenderContext<'reg, 'rc>,
-    ) -> Result<Option<ScopedJson<'reg, 'rc>>, RenderError> {
+    ) -> Result<ScopedJson<'reg, 'rc>, RenderError> {
         call_script_helper(h.params(), h.hash(), &reg.engine, &self.script)
     }
 }
@@ -104,7 +104,6 @@ mod test {
         };
 
         let result = call_script_helper(&params, &hash, &engine, &ast)
-            .unwrap()
             .unwrap()
             .render();
         assert_eq!("1,true,2,no", &result);

src/macros.rs

@@ -54,7 +54,7 @@ macro_rules! handlebars_helper {
                 _: &'reg $crate::Handlebars<'reg>,
                 _: &'rc $crate::Context,
                 _: &mut $crate::RenderContext<'reg, 'rc>,
-            ) -> Result<Option<$crate::ScopedJson<'reg, 'rc>>, $crate::RenderError> {
+            ) -> Result<$crate::ScopedJson<'reg, 'rc>, $crate::RenderError> {
                 let mut param_idx = 0;
 
                 $(
@@ -97,7 +97,7 @@ macro_rules! handlebars_helper {
                     $(let $kwargs = h.hash().iter().map(|(k, v)| (k.to_owned(), v.value())).collect::<std::collections::BTreeMap<&str, &serde_json::Value>>();)?
 
                 let result = $body;
-                Ok(Some($crate::ScopedJson::Derived($crate::JsonValue::from(result))))
+                Ok($crate::ScopedJson::Derived($crate::JsonValue::from(result)))
             }
         }
     };

src/registry.rs

@@ -862,8 +862,8 @@ mod test {
             _: &'reg Registry<'reg>,
             _: &'rc Context,
             _: &mut RenderContext<'reg, 'rc>,
-        ) -> Result<Option<ScopedJson<'reg, 'rc>>, RenderError> {
-            Ok(Some(ScopedJson::Missing))
+        ) -> Result<ScopedJson<'reg, 'rc>, RenderError> {
+            Ok(ScopedJson::Missing)
         }
     }
 

src/render.rs

@@ -539,25 +539,30 @@ fn call_helper_for_value<'reg: 'rc, 'rc>(
     ctx: &'rc Context,
     rc: &mut RenderContext<'reg, 'rc>,
 ) -> Result<PathAndJson<'reg, 'rc>, RenderError> {
-    if let Some(result) = hd.call_inner(ht, r, ctx, rc)? {
-        Ok(PathAndJson::new(None, result))
-    } else {
-        // parse value from output
-        let mut so = StringOutput::new();
-
-        // here we don't want subexpression result escaped,
-        // so we temporarily disable it
-        let disable_escape = rc.is_disable_escape();
-        rc.set_disable_escape(true);
-
-        hd.call(ht, r, ctx, rc, &mut so)?;
-        rc.set_disable_escape(disable_escape);
-
-        let string = so.into_string().map_err(RenderError::from)?;
-        Ok(PathAndJson::new(
-            None,
-            ScopedJson::Derived(Json::String(string)),
-        ))
+    match hd.call_inner(ht, r, ctx, rc) {
+        Ok(result) => Ok(PathAndJson::new(None, result)),
+        Err(e) => {
+            if e.is_unimplemented() {
+                // parse value from output
+                let mut so = StringOutput::new();
+
+                // here we don't want subexpression result escaped,
+                // so we temporarily disable it
+                let disable_escape = rc.is_disable_escape();
+                rc.set_disable_escape(true);
+
+                hd.call(ht, r, ctx, rc, &mut so)?;
+                rc.set_disable_escape(disable_escape);
+
+                let string = so.into_string().map_err(RenderError::from)?;
+                Ok(PathAndJson::new(
+                    None,
+                    ScopedJson::Derived(Json::String(string)),
+                ))
+            } else {
+                Err(e)
+            }
+        }
     }
 }
 

tests/subexpression.rs

@@ -2,6 +2,9 @@ extern crate handlebars;
 #[macro_use]
 extern crate serde_json;
 
+use std::sync::atomic::{AtomicU16, Ordering};
+use std::sync::Arc;
+
 use handlebars::{Context, Handlebars, Helper, HelperDef, RenderContext, RenderError, ScopedJson};
 
 #[test]
@@ -75,11 +78,11 @@ impl HelperDef for MyHelper {
         _: &'reg Handlebars,
         _: &'rc Context,
         _: &mut RenderContext<'reg, 'rc>,
-    ) -> Result<Option<ScopedJson<'reg, 'rc>>, RenderError> {
-        Ok(Some(ScopedJson::Derived(json!({
+    ) -> Result<ScopedJson<'reg, 'rc>, RenderError> {
+        Ok(ScopedJson::Derived(json!({
             "a": 1,
             "b": 2,
-        }))))
+        })))
     }
 }
 
@@ -95,3 +98,57 @@ fn test_lookup_with_subexpression() {
 
     assert_eq!("1", result);
 }
+
+struct CallCounterHelper {
+    pub(crate) c: Arc<AtomicU16>,
+}
+
+impl HelperDef for CallCounterHelper {
+    fn call_inner<'reg: 'rc, 'rc>(
+        &self,
+        h: &Helper<'reg, 'rc>,
+        _: &'reg Handlebars,
+        _: &'rc Context,
+        _: &mut RenderContext<'reg, 'rc>,
+    ) -> Result<ScopedJson<'reg, 'rc>, RenderError> {
+        // inc counter
+        self.c.fetch_add(1, Ordering::SeqCst);
+
+        if let Some(_) = h.param(0) {
+            Ok(json!({
+                "a": 1,
+            })
+            .into())
+        } else {
+            Ok(json!(null).into())
+        }
+    }
+}
+
+#[test]
+fn test_helper_call_count() {
+    let mut registry = Handlebars::new();
+
+    let counter = Arc::new(AtomicU16::new(0));
+    let helper = Box::new(CallCounterHelper { c: counter.clone() });
+
+    registry.register_helper("myhelper", helper);
+
+    registry
+        .render_template(
+            "{{#if (myhelper a)}}something{{else}}nothing{{/if}}",
+            &json!(null),
+        ) // If returns true
+        .unwrap();
+
+    assert_eq!(1, counter.load(Ordering::SeqCst));
+
+    registry
+        .render_template(
+            "{{#if (myhelper)}}something{{else}}nothing{{/if}}",
+            &json!(null),
+        ) // If returns false
+        .unwrap();
+
+    assert_eq!(2, counter.load(Ordering::SeqCst));
+}