Repository: mozilla/nunjucks (js)

Issue:
Exceptions being swallowed after import errors in included templates
As stated in topic #1127, there are still problems with exceptions not being thrown in certain cases. I found out that if some problem happens with import errors inside included templates, Nunjucks will fail to generate the required exceptions. For example, create a template **included.njk**:

```njk
{% import 'unknown.njk' as nothing %}
String is {{ str | whatever }}
```
and the following script:

```js
const path = 'C:/users/etc/thispath';
const nunjucks = require("nunjucks");
nunjucks.configure(path);
console.log(nunjucks.renderString("{% include 'included.njk' %}", { str: 'okay' }));
```
Nunjucks silently outputs a `null` string. Some notes:

- The exception is captured correctly if the contents of the njk file are directly used as the template.
- The exception is also captured if the line with `import` is removed from the njk file.

I hope this is clear. Thanks in advance!


Pull request description:
Fix issue where sync render would not raise errors in included templates
## Summary

The fix introduced in PR #1049, preventing multiple calls to the render callback, was a bit too aggressive and prevented some template errors from being surfaced when `render` or `renderString` were called synchronously.

The real bug here lies in the compiler, which should not be calling the same callback function more than once. But the way that waterfall tasks are chained makes it difficult to properly halt execution in a template. We ought to fix the underlying issue, but for now this should (hopefully) ensure that thrown errors always surface when rendering synchronously, while not causing a regression for #1029.

Closes #1272 . I suspect this also fixes #1246, but I can't confirm because the reporter of that issue hasn't provided a test case I can use.


## Checklist

* [x] Proposed change helps towards [*purpose of this project*](https://github.com/mozilla/nunjucks/blob/master/CONTRIBUTING.md#purpose).
* [x] [*Tests*](https://github.com/mozilla/nunjucks/tree/master/tests) are added / updated to cover proposed change.
* [x] [*Changelog*](https://github.com/mozilla/nunjucks/blob/master/CHANGELOG.md) has an entry for proposed change (if user-facing fix or feature).
* ~[*Documentation*](https://github.com/mozilla/nunjucks/tree/master/docs/) is added / updated to describe proposed change. **(N/A)**~

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b17d2bc..2956b9b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,9 @@ Changelog
   Merge of [#1276](https://github.com/mozilla/nunjucks/pull/1276); fixes
   [#1198](https://github.com/mozilla/nunjucks/issues/1198). Thanks
   [ogonkov](https://github.com/ogonkovv)!
+* Fix bug that prevented errors in included templates from being raised when
+  rendering templates synchronously. Fixes
+  [#1272](https://github.com/mozilla/nunjucks/pull/1276).
 
 3.2.1 (Mar 17 2020)
 -------------------
diff --git a/nunjucks/src/environment.js b/nunjucks/src/environment.js
index 1cd7537..d255aff 100644
--- a/nunjucks/src/environment.js
+++ b/nunjucks/src/environment.js
@@ -476,14 +476,15 @@ class Template extends Obj {
     let didError = false;
 
     this.rootRenderFunc(this.env, context, frame, globalRuntime, (err, res) => {
-      if (didError) {
+      // TODO: this is actually a bug in the compiled template (because waterfall
+      // tasks are both not passing errors up the chain of callbacks AND are not
+      // causing a return from the top-most render function). But fixing that
+      // will require a more substantial change to the compiler.
+      if (didError && cb && typeof res !== 'undefined') {
         // prevent multiple calls to cb
-        if (cb) {
-          return;
-        } else {
-          throw err;
-        }
+        return;
       }
+
       if (err) {
         err = lib._prettifyError(this.path, this.env.opts.dev, err);
         didError = true;


Test patch:
diff --git a/tests/compiler.js b/tests/compiler.js
index c1c38ec..8e133e7 100644
--- a/tests/compiler.js
+++ b/tests/compiler.js
@@ -847,6 +847,25 @@
       });
     }
 
+    it('should throw exceptions from included templates when called synchronously', function() {
+      function templateRender() {
+        render('{% include "broken-import.njk" %}', {str: 'abc'});
+      }
+      expect(templateRender).to.throwException(/template not found: doesnotexist/);
+    });
+
+    it('should pass errors from included templates to callback when async', function(done) {
+      render(
+        '{% include "broken-import.njk" %}',
+        {str: 'abc'},
+        {noThrow: true},
+        function(err, res) {
+          expect(err).to.match(/template not found: doesnotexist/);
+          expect(res).to.be(undefined);
+          done();
+        });
+    });
+
     it('should compile string concatenations with tilde', function(done) {
       equal('{{ 4 ~ \'hello\' }}', '4hello');
       equal('{{ 4 ~ 5 }}', '45');
diff --git a/tests/templates/broken-import.njk b/tests/templates/broken-import.njk
new file mode 100644
index 0000000..64a45f4
--- /dev/null
+++ b/tests/templates/broken-import.njk
@@ -0,0 +1,2 @@
+{% import 'doesnotexist' as doesnotexist %}
+str = {{ str | undefinedfilter }}
\ No newline at end of file