Improved #attempt error reporting:

- Added new configuration setting, attempt_exception_reporter 
(Configurable.setAttemptExceptionReporter(AttemptExceptionReporter)), to allow 
the customization of how the exceptions handled (and thus suppressed) by the 
"attempt" directive are reported. The default AttemptExceptionReporter logs the 
exception as an error, just as it was in earlier versions, though now the error 
message will indicate that the exception was thrown inside an attempt directive 
block.
- Bug fixed: When the TemplateExceptionHandler suppresses (i.e., doesn't 
re-throw) an exception, the attempt directive won't log it anymore. (To be 
precise, the AttemptExceptionReporter won't be invoked for it anymore; the 
default one logs as error.)


Project: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/repo
Commit: 
http://git-wip-us.apache.org/repos/asf/incubator-freemarker/commit/304df82a
Tree: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/tree/304df82a
Diff: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/diff/304df82a

Branch: refs/heads/2.3
Commit: 304df82a7501f66386b2c5be740d377961f93dc5
Parents: 4b5b7e8
Author: ddekany <ddek...@apache.org>
Authored: Wed Jun 28 01:42:10 2017 +0200
Committer: ddekany <ddek...@apache.org>
Committed: Wed Jun 28 01:42:26 2017 +0200

----------------------------------------------------------------------
 src/main/java/freemarker/core/Configurable.java | 107 ++++++++++++++++---
 src/main/java/freemarker/core/Environment.java  |  27 +++--
 .../freemarker/core/TemplateConfiguration.java  |   7 ++
 .../template/AttemptExceptionReporter.java      |  48 +++++++++
 .../java/freemarker/template/Configuration.java |  45 ++++++++
 .../LoggingAttemptExceptionReporter.java        |  29 +++++
 .../template/TemplateExceptionHandler.java      |   8 +-
 .../java/freemarker/template/_TemplateAPI.java  |   5 +
 src/manual/en_US/book.xml                       |  66 +++++++++---
 .../freemarker/core/AttemptLoggingTest.java     |  77 +++++++++++++
 .../core/TemplateConfigurationTest.java         |   2 +
 .../freemarker/template/ConfigurationTest.java  |  12 +++
 12 files changed, 392 insertions(+), 41 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/main/java/freemarker/core/Configurable.java
----------------------------------------------------------------------
diff --git a/src/main/java/freemarker/core/Configurable.java 
b/src/main/java/freemarker/core/Configurable.java
index 914015a..9084956 100644
--- a/src/main/java/freemarker/core/Configurable.java
+++ b/src/main/java/freemarker/core/Configurable.java
@@ -52,6 +52,7 @@ import freemarker.cache.PathRegexMatcher;
 import freemarker.cache.TemplateLoader;
 import freemarker.ext.beans.BeansWrapper;
 import freemarker.ext.beans.BeansWrapperBuilder;
+import freemarker.template.AttemptExceptionReporter;
 import freemarker.template.Configuration;
 import freemarker.template.DefaultObjectWrapper;
 import freemarker.template.ObjectWrapper;
@@ -162,6 +163,13 @@ public class Configurable {
     /** Alias to the {@code ..._SNAKE_CASE} variation due to backward 
compatibility constraints. */
     public static final String TEMPLATE_EXCEPTION_HANDLER_KEY = 
TEMPLATE_EXCEPTION_HANDLER_KEY_SNAKE_CASE;
     
+    /** Legacy, snake case ({@code like_this}) variation of the setting name. 
@since 2.3.27 */
+    public static final String ATTEMPT_EXCEPTION_REPORTER_KEY_SNAKE_CASE = 
"attempt_exception_reporter";
+    /** Modern, camel case ({@code likeThis}) variation of the setting name. 
@since 2.3.27 */
+    public static final String ATTEMPT_EXCEPTION_REPORTER_KEY_CAMEL_CASE = 
"attemptExceptionReporter";
+    /** Alias to the {@code ..._SNAKE_CASE} variation due to backward 
compatibility constraints. */
+    public static final String ATTEMPT_EXCEPTION_REPORTER_KEY = 
ATTEMPT_EXCEPTION_REPORTER_KEY_SNAKE_CASE;
+    
     /** Legacy, snake case ({@code like_this}) variation of the setting name. 
@since 2.3.23 */
     public static final String ARITHMETIC_ENGINE_KEY_SNAKE_CASE = 
"arithmetic_engine";
     /** Modern, camel case ({@code likeThis}) variation of the setting name. 
@since 2.3.23 */
@@ -275,6 +283,7 @@ public class Configurable {
         // Must be sorted alphabetically!
         API_BUILTIN_ENABLED_KEY_SNAKE_CASE,
         ARITHMETIC_ENGINE_KEY_SNAKE_CASE,
+        ATTEMPT_EXCEPTION_REPORTER_KEY_SNAKE_CASE,
         AUTO_FLUSH_KEY_SNAKE_CASE,
         AUTO_IMPORT_KEY_SNAKE_CASE,
         AUTO_INCLUDE_KEY_SNAKE_CASE,
@@ -305,6 +314,7 @@ public class Configurable {
         // Must be sorted alphabetically!
         API_BUILTIN_ENABLED_KEY_CAMEL_CASE,
         ARITHMETIC_ENGINE_KEY_CAMEL_CASE,
+        ATTEMPT_EXCEPTION_REPORTER_KEY_CAMEL_CASE,
         AUTO_FLUSH_KEY_CAMEL_CASE,
         AUTO_IMPORT_KEY_CAMEL_CASE,
         AUTO_INCLUDE_KEY_CAMEL_CASE,
@@ -348,6 +358,7 @@ public class Configurable {
     private String falseStringValue;  // deduced from booleanFormat
     private Integer classicCompatible;
     private TemplateExceptionHandler templateExceptionHandler;
+    private AttemptExceptionReporter attemptExceptionReporter;
     private ArithmeticEngine arithmeticEngine;
     private ObjectWrapper objectWrapper;
     private String outputEncoding;
@@ -412,9 +423,10 @@ public class Configurable {
         classicCompatible = Integer.valueOf(0);
         properties.setProperty(CLASSIC_COMPATIBLE_KEY, 
classicCompatible.toString());
         
-        templateExceptionHandler = 
_TemplateAPI.getDefaultTemplateExceptionHandler(
-                incompatibleImprovements);
+        templateExceptionHandler = 
_TemplateAPI.getDefaultTemplateExceptionHandler(incompatibleImprovements);
         properties.setProperty(TEMPLATE_EXCEPTION_HANDLER_KEY, 
templateExceptionHandler.getClass().getName());
+
+        attemptExceptionReporter = 
_TemplateAPI.getDefaultAttemptExceptionReporter(incompatibleImprovements);
         
         arithmeticEngine = ArithmeticEngine.BIGDECIMAL_ENGINE;
         properties.setProperty(ARITHMETIC_ENGINE_KEY, 
arithmeticEngine.getClass().getName());
@@ -461,12 +473,8 @@ public class Configurable {
      */
     public Configurable(Configurable parent) {
         this.parent = parent;
-        locale = null;
-        numberFormat = null;
-        classicCompatible = null;
-        templateExceptionHandler = null;
         properties = new Properties(parent.properties);
-        customAttributes = new HashMap(0);
+        customAttributes = new HashMap<Object, Object>(0);
     }
     
     @Override
@@ -1313,7 +1321,16 @@ public class Configurable {
      * Neither is it meant to be used to roll back the printed output. These 
should be solved outside template
      * processing when the exception raises from {@link 
Template#process(Object, Writer) Template.process}.
      * {@link TemplateExceptionHandler} meant to be used if you want to 
include special content <em>in</em> the template
-     * output, or if you want to suppress certain exceptions. 
+     * output, or if you want to suppress certain exceptions. If you suppress 
an exception, and the
+     * {@link Environment#getLogTemplateExceptions()} returns {@code false}, 
then it's the responsibility of the
+     * {@link TemplateExceptionHandler} to log the exception (if you want it 
to be logged).  
+     *  
+     * <p>The {@link #setLogTemplateExceptions(boolean) 
log_template_exceptions} and
+     * {@link #setAttemptExceptionReporter(AttemptExceptionReporter) 
attempt_exception_reporter} settings take effect
+     * before the {@link TemplateExceptionHandler} is invoked, so these 
settings are technically independent, and deal
+     * with different aspects of exception handling.  
+     * 
+     * @see #setAttemptExceptionReporter(AttemptExceptionReporter)
      */
     public void setTemplateExceptionHandler(TemplateExceptionHandler 
templateExceptionHandler) {
         NullArgumentException.check("templateExceptionHandler", 
templateExceptionHandler);
@@ -1337,6 +1354,44 @@ public class Configurable {
     public boolean isTemplateExceptionHandlerSet() {
         return templateExceptionHandler != null;
     }
+    
+    /**
+     * Specifies how exceptions handled (and hence suppressed) by an {@code 
#attempt} blocks will be logged or otherwise
+     * reported. The default value is {@link 
AttemptExceptionReporter#LOG_ERROR_REPORTER}.
+     * 
+     * <p>Note that {@code #attempt} is not supposed to be a general purpose 
error handler mechanism, like {@code try}
+     * is in Java. It's for decreasing the impact of unexpected errors, by 
making it possible that only part of the
+     * page is going down, instead of the whole page. But it's still an error, 
something that someone should fix. So the
+     * error should be reported, not just ignored in a custom {@link 
AttemptExceptionReporter}-s.
+     * 
+     * <p>The {@link AttemptExceptionReporter} is invoked regardless of the 
value of the
+     * {@link #setLogTemplateExceptions(boolean) log_template_exceptions} 
setting.
+     * 
+     * @since 2.3.27
+     */
+    public void setAttemptExceptionReporter(AttemptExceptionReporter 
attemptExceptionReporter) {
+        NullArgumentException.check("attemptExceptionReporter", 
attemptExceptionReporter);
+        this.attemptExceptionReporter = attemptExceptionReporter;
+    }
+    
+    /**
+     * The getter pair of {@link 
#setAttemptExceptionReporter(AttemptExceptionReporter)}.
+     * 
+     * @since 2.3.27
+     */
+    public AttemptExceptionReporter getAttemptExceptionReporter() {
+        return attemptExceptionReporter != null
+                ? attemptExceptionReporter : 
parent.getAttemptExceptionReporter();
+    }
+    
+    /**
+     * Tells if this setting is set directly in this object or its value is 
coming from the {@link #getParent() parent}.
+     *  
+     * @since 2.3.27
+     */
+    public boolean isAttemptExceptionReporterSet() {
+        return attemptExceptionReporter != null;
+    }
 
     /**
      * Sets the arithmetic engine used to perform arithmetic operations.
@@ -1607,8 +1662,9 @@ public class Configurable {
      * written applications, because there the {@link TemplateException} 
thrown by the public FreeMarker API is also
      * logged by the caller (even if only as the cause exception of a higher 
level exception). Hence, in modern
      * applications it should be set to {@code false}. Note that this setting 
has no effect on the logging of exceptions
-     * caught by {@code #attempt}; those are always logged, no mater what 
(because those exceptions won't bubble up
-     * until the API caller).
+     * caught by {@code #attempt}; by default those are always logged as 
errors (because those exceptions won't bubble
+     * up to the API caller), however, that can be changed with the {@link
+     * #setAttemptExceptionReporter(AttemptExceptionReporter) 
attempt_exception_reporter} setting.
      * 
      * @since 2.3.22
      */
@@ -2020,8 +2076,17 @@ public class Configurable {
      *       {@code "rethrow"} (means {@link 
TemplateExceptionHandler#RETHROW_HANDLER}),
      *       {@code "debug"} (means {@link 
TemplateExceptionHandler#DEBUG_HANDLER}),
      *       {@code "html_debug"} (means {@link 
TemplateExceptionHandler#HTML_DEBUG_HANDLER}),
-     *       {@code "ignore"} (means {@link 
TemplateExceptionHandler#IGNORE_HANDLER}),
-     *       {@code "default"} (only allowed for {@link Configuration} 
instances) for the default.
+     *       {@code "ignore"} (means {@link 
TemplateExceptionHandler#IGNORE_HANDLER}), or
+     *       {@code "default"} (only allowed for {@link Configuration} 
instances) for the default value.
+     *       
+     *   <li><p>{@code "attempt_exception_reporter"}:
+     *       See {@link 
#setAttemptExceptionReporter(AttemptExceptionReporter)}.
+     *       <br>String value: If the value contains dot, then it's 
interpreted as an <a href="#fm_obe">object builder
+     *       expression</a>.
+     *       If the value does not contain dot, then it must be one of these 
predefined values (case insensitive):
+     *       {@code "log_error"} (means {@link 
AttemptExceptionReporter#LOG_ERROR_REPORTER}),
+     *       {@code "log_warn"} (means {@link 
AttemptExceptionReporter#LOG_WARN_REPORTER}), or
+     *       {@code "default"} (only allowed for {@link Configuration} 
instances) for the default value.
      *       
      *   <li><p>{@code "arithmetic_engine"}:
      *       See {@link #setArithmeticEngine(ArithmeticEngine)}.  
@@ -2457,6 +2522,24 @@ public class Configurable {
                     setTemplateExceptionHandler((TemplateExceptionHandler) 
_ObjectBuilderSettingEvaluator.eval(
                             value, TemplateExceptionHandler.class, false, 
_SettingEvaluationEnvironment.getCurrent()));
                 }
+            } else if (ATTEMPT_EXCEPTION_REPORTER_KEY_SNAKE_CASE.equals(name)
+                    || ATTEMPT_EXCEPTION_REPORTER_KEY_CAMEL_CASE.equals(name)) 
{
+                if (value.indexOf('.') == -1) {
+                    if ("log_error".equalsIgnoreCase(value) || 
"logError".equals(value)) {
+                        setAttemptExceptionReporter(
+                                AttemptExceptionReporter.LOG_ERROR_REPORTER);
+                    } else if ("log_warn".equalsIgnoreCase(value) || 
"logWarn".equals(value)) {
+                        setAttemptExceptionReporter(
+                                AttemptExceptionReporter.LOG_WARN_REPORTER);
+                    } else if (DEFAULT.equalsIgnoreCase(value) && this 
instanceof Configuration) {
+                        ((Configuration) this).unsetAttemptExceptionReporter();
+                    } else {
+                        throw invalidSettingValueException(name, value);
+                    }
+                } else {
+                    setAttemptExceptionReporter((AttemptExceptionReporter) 
_ObjectBuilderSettingEvaluator.eval(
+                            value, AttemptExceptionReporter.class, false, 
_SettingEvaluationEnvironment.getCurrent()));
+                }
             } else if (ARITHMETIC_ENGINE_KEY_SNAKE_CASE.equals(name) || 
ARITHMETIC_ENGINE_KEY_CAMEL_CASE.equals(name)) {
                 if (value.indexOf('.') == -1) { 
                     if ("bigdecimal".equalsIgnoreCase(value)) {

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/main/java/freemarker/core/Environment.java
----------------------------------------------------------------------
diff --git a/src/main/java/freemarker/core/Environment.java 
b/src/main/java/freemarker/core/Environment.java
index ce34a65..1fa59a0 100644
--- a/src/main/java/freemarker/core/Environment.java
+++ b/src/main/java/freemarker/core/Environment.java
@@ -839,20 +839,27 @@ public final class Environment extends Configurable {
         }
         lastThrowable = templateException;
 
-        // Log the exception, if logTemplateExceptions isn't false. However, 
even if it's false, if we are inside
-        // an #attempt block, it has to be logged, as it certainly won't 
bubble up to the caller of FreeMarker.
-        if (LOG.isErrorEnabled() && (isInAttemptBlock() || 
getLogTemplateExceptions())) {
+        if (getLogTemplateExceptions() && LOG.isErrorEnabled()
+                && !isInAttemptBlock() /* because then the 
AttemptExceptionReporter will report this */) {
             LOG.error("Error executing FreeMarker template", 
templateException);
         }
 
-        // Stop exception is not passed to the handler, but
-        // explicitly rethrown.
-        if (templateException instanceof StopException) {
-            throw templateException;
+        try {
+            // Stop exception is not passed to the handler, but
+            // explicitly rethrown.
+            if (templateException instanceof StopException) {
+                throw templateException;
+            }
+    
+            // Finally, pass the exception to the handler
+            
getTemplateExceptionHandler().handleTemplateException(templateException, this, 
out);
+        } catch (TemplateException e) {
+            // Note that if the TemplateExceptionHandler doesn't rethrow the 
exception, we don't get in there.
+            if (isInAttemptBlock()) {
+                this.getAttemptExceptionReporter().report(templateException, 
this);
+            }
+            throw e;
         }
-
-        // Finally, pass the exception to the handler
-        
getTemplateExceptionHandler().handleTemplateException(templateException, this, 
out);
     }
 
     @Override

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/main/java/freemarker/core/TemplateConfiguration.java
----------------------------------------------------------------------
diff --git a/src/main/java/freemarker/core/TemplateConfiguration.java 
b/src/main/java/freemarker/core/TemplateConfiguration.java
index 0569890..5ec5a02 100644
--- a/src/main/java/freemarker/core/TemplateConfiguration.java
+++ b/src/main/java/freemarker/core/TemplateConfiguration.java
@@ -234,6 +234,9 @@ public final class TemplateConfiguration extends 
Configurable implements ParserC
         if (tc.isTemplateExceptionHandlerSet()) {
             setTemplateExceptionHandler(tc.getTemplateExceptionHandler());
         }
+        if (tc.isAttemptExceptionReporterSet()) {
+            setAttemptExceptionReporter(tc.getAttemptExceptionReporter());
+        }
         if (tc.isTimeFormatSet()) {
             setTimeFormat(tc.getTimeFormat());
         }
@@ -349,6 +352,9 @@ public final class TemplateConfiguration extends 
Configurable implements ParserC
         if (isTemplateExceptionHandlerSet() && 
!template.isTemplateExceptionHandlerSet()) {
             
template.setTemplateExceptionHandler(getTemplateExceptionHandler());
         }
+        if (isAttemptExceptionReporterSet() && 
!template.isAttemptExceptionReporterSet()) {
+            
template.setAttemptExceptionReporter(getAttemptExceptionReporter());
+        }
         if (isTimeFormatSet() && !template.isTimeFormatSet()) {
             template.setTimeFormat(getTimeFormat());
         }
@@ -635,6 +641,7 @@ public final class TemplateConfiguration extends 
Configurable implements ParserC
                 || isShowErrorTipsSet()
                 || isSQLDateAndTimeTimeZoneSet()
                 || isTemplateExceptionHandlerSet()
+                || isAttemptExceptionReporterSet()
                 || isTimeFormatSet()
                 || isTimeZoneSet()
                 || isURLEscapingCharsetSet();

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/main/java/freemarker/template/AttemptExceptionReporter.java
----------------------------------------------------------------------
diff --git a/src/main/java/freemarker/template/AttemptExceptionReporter.java 
b/src/main/java/freemarker/template/AttemptExceptionReporter.java
new file mode 100644
index 0000000..c12076a
--- /dev/null
+++ b/src/main/java/freemarker/template/AttemptExceptionReporter.java
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package freemarker.template;
+
+import freemarker.core.Configurable;
+import freemarker.core.Environment;
+
+/**
+ * Used for the {@link 
Configurable#setAttemptExceptionReporter(AttemptExceptionReporter) 
attempt_exception_reported}
+ * configuration setting.
+ */
+public interface AttemptExceptionReporter {
+    
+    /**
+     * Logs the exception into the "freemarker.runtime" log category with 
"error" log level. This is the default
+     * {@link AttemptExceptionReporter}. The error message will explain that 
the error was handled by an
+     * {@code #attempt} block.
+     */
+    AttemptExceptionReporter LOG_ERROR_REPORTER = new 
LoggingAttemptExceptionReporter(false);
+
+    /**
+     * Like {@link #LOG_ERROR_REPORTER}, but it logs with "warn" log level.
+     */
+    AttemptExceptionReporter LOG_WARN_REPORTER = new 
LoggingAttemptExceptionReporter(true);
+    
+    /**
+     * Called to log or otherwise report the error that has occurred inside an 
{@code #attempt} block.  
+     */
+    void report(TemplateException te, Environment env);
+    
+}

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/main/java/freemarker/template/Configuration.java
----------------------------------------------------------------------
diff --git a/src/main/java/freemarker/template/Configuration.java 
b/src/main/java/freemarker/template/Configuration.java
index 42426e8..57b0121 100644
--- a/src/main/java/freemarker/template/Configuration.java
+++ b/src/main/java/freemarker/template/Configuration.java
@@ -510,6 +510,7 @@ public class Configuration extends Configurable implements 
Cloneable, ParserConf
     
     private boolean objectWrapperExplicitlySet;
     private boolean templateExceptionHandlerExplicitlySet;
+    private boolean attemptExceptionReporterExplicitlySet;
     private boolean logTemplateExceptionsExplicitlySet;
     private boolean localeExplicitlySet;
     private boolean defaultEncodingExplicitlySet;
@@ -961,6 +962,10 @@ public class Configuration extends Configurable implements 
Cloneable, ParserConf
     private TemplateExceptionHandler getDefaultTemplateExceptionHandler() {
         return 
getDefaultTemplateExceptionHandler(getIncompatibleImprovements());
     }
+
+    private AttemptExceptionReporter getDefaultAttemptExceptionReporter() {
+        return 
getDefaultAttemptExceptionReporter(getIncompatibleImprovements());
+    }
     
     private boolean getDefaultLogTemplateExceptions() {
         return getDefaultLogTemplateExceptions(getIncompatibleImprovements());
@@ -976,6 +981,11 @@ public class Configuration extends Configurable implements 
Cloneable, ParserConf
     }
 
     // Package visible as Configurable needs this to initialize the field 
defaults.
+    final static AttemptExceptionReporter 
getDefaultAttemptExceptionReporter(Version incompatibleImprovements) {
+        return AttemptExceptionReporter.LOG_ERROR_REPORTER;
+    }
+    
+    // Package visible as Configurable needs this to initialize the field 
defaults.
     final static boolean getDefaultLogTemplateExceptions(Version 
incompatibleImprovements) {
         return true;
     }
@@ -1703,6 +1713,36 @@ public class Configuration extends Configurable 
implements Cloneable, ParserConf
      */
     public boolean isTemplateExceptionHandlerExplicitlySet() {
         return templateExceptionHandlerExplicitlySet;
+    }
+    
+    @Override
+    public void setAttemptExceptionReporter(AttemptExceptionReporter 
attemptExceptionReporter) {
+        super.setAttemptExceptionReporter(attemptExceptionReporter);
+        attemptExceptionReporterExplicitlySet = true;
+    }
+
+    /**
+     * Resets the setting to its default, as if it was never set. This means 
that when you change the
+     * {@code incompatibe_improvements} setting later, the default will also 
change as appropriate. Also 
+     * {@link #isAttemptExceptionReporterExplicitlySet()} will return {@code 
false}.
+     * 
+     * @since 2.3.27
+     */
+    public void unsetAttemptExceptionReporter() {
+        if (attemptExceptionReporterExplicitlySet) {
+            setAttemptExceptionReporter(getDefaultAttemptExceptionReporter());
+            attemptExceptionReporterExplicitlySet = false;
+        }
+    }
+    
+    /**
+     * Tells if {@link #setAttemptExceptionReporter(AttemptExceptionReporter)} 
(or equivalent) was already called on
+     * this instance.
+     * 
+     * @since 2.3.27
+     */
+    public boolean isAttemptExceptionReporterExplicitlySet() {
+        return attemptExceptionReporterExplicitlySet;
     }    
     
     /**
@@ -1789,6 +1829,11 @@ public class Configuration extends Configurable 
implements Cloneable, ParserConf
                 templateExceptionHandlerExplicitlySet = true;
                 unsetTemplateExceptionHandler();
             }
+
+            if (!attemptExceptionReporterExplicitlySet) {
+                attemptExceptionReporterExplicitlySet = true;
+                unsetAttemptExceptionReporter();
+            }
             
             if (!logTemplateExceptionsExplicitlySet) {
                 logTemplateExceptionsExplicitlySet = true;

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/main/java/freemarker/template/LoggingAttemptExceptionReporter.java
----------------------------------------------------------------------
diff --git 
a/src/main/java/freemarker/template/LoggingAttemptExceptionReporter.java 
b/src/main/java/freemarker/template/LoggingAttemptExceptionReporter.java
new file mode 100644
index 0000000..80563ff
--- /dev/null
+++ b/src/main/java/freemarker/template/LoggingAttemptExceptionReporter.java
@@ -0,0 +1,29 @@
+package freemarker.template;
+
+import freemarker.core.Environment;
+import freemarker.log.Logger;
+
+/**
+ * Default {@link AttemptExceptionReporter} implementation, factored out from 
{@link AttemptExceptionReporter} so that
+ * we can have static field.
+ */
+class LoggingAttemptExceptionReporter implements AttemptExceptionReporter {
+    
+    private static final Logger LOG = Logger.getLogger("freemarker.runtime");
+    
+    private final boolean logAsWarn;
+    
+    public LoggingAttemptExceptionReporter(boolean logAsWarn) {
+        this.logAsWarn = logAsWarn;
+    }
+
+    public void report(TemplateException te, Environment env) {
+        String message = "Error executing FreeMarker template part in the 
#attempt block";
+        if (!logAsWarn) {
+            LOG.error(message, te);
+        } else {
+            LOG.warn(message, te);
+        }
+    }
+    
+}
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/main/java/freemarker/template/TemplateExceptionHandler.java
----------------------------------------------------------------------
diff --git a/src/main/java/freemarker/template/TemplateExceptionHandler.java 
b/src/main/java/freemarker/template/TemplateExceptionHandler.java
index 7d3744e..caf1e79 100644
--- a/src/main/java/freemarker/template/TemplateExceptionHandler.java
+++ b/src/main/java/freemarker/template/TemplateExceptionHandler.java
@@ -29,8 +29,8 @@ import freemarker.core.StopException;
 import freemarker.template.utility.StringUtil;
 
 /**
- * Used for the {@code template_exception_handler} configuration setting;
- * see {@link 
Configurable#setTemplateExceptionHandler(TemplateExceptionHandler)} for more.
+ * Used for the {@link 
Configurable#setTemplateExceptionHandler(TemplateExceptionHandler) 
template_exception_handler}
+ * configuration setting.
  */
 public interface TemplateExceptionHandler {
     
@@ -39,9 +39,9 @@ public interface TemplateExceptionHandler {
      * unless you want to suppress the exception.
      * 
      * <p>Note that you can check with {@link Environment#isInAttemptBlock()} 
if you are inside a {@code #attempt}
-     * block, which then will handle handle this exception and roll back the 
output generated inside it.
+     * block, which then will handle this exception and roll back the output 
generated inside it.
      * 
-     * <p>Note that {@link StopException}-s (raised by {@code #stop}) won't be 
captured.
+     * <p>Note that {@link StopException}-s (raised by {@code #stop}) won't be 
captured here.
      * 
      * <p>Note that you shouldn't log the exception in this method unless you 
suppress it. If there's a concern that the
      * exception might won't be logged after it bubbles up from {@link 
Template#process(Object, Writer)}, simply

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/main/java/freemarker/template/_TemplateAPI.java
----------------------------------------------------------------------
diff --git a/src/main/java/freemarker/template/_TemplateAPI.java 
b/src/main/java/freemarker/template/_TemplateAPI.java
index 737c528..373e0e6 100644
--- a/src/main/java/freemarker/template/_TemplateAPI.java
+++ b/src/main/java/freemarker/template/_TemplateAPI.java
@@ -81,6 +81,11 @@ public class _TemplateAPI {
         return 
Configuration.getDefaultTemplateExceptionHandler(incompatibleImprovements);
     }
 
+    public static AttemptExceptionReporter getDefaultAttemptExceptionReporter(
+            Version incompatibleImprovements) {
+        return 
Configuration.getDefaultAttemptExceptionReporter(incompatibleImprovements);
+    }
+    
     public static boolean getDefaultLogTemplateExceptions(Version 
incompatibleImprovements) {
         return 
Configuration.getDefaultLogTemplateExceptions(incompatibleImprovements);
     }

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/manual/en_US/book.xml
----------------------------------------------------------------------
diff --git a/src/manual/en_US/book.xml b/src/manual/en_US/book.xml
index 77b13cd..a4d63a1 100644
--- a/src/manual/en_US/book.xml
+++ b/src/manual/en_US/book.xml
@@ -2724,7 +2724,7 @@ baz
             <literal>book[test]</literal>. More examples; these are all
             equivalent: <literal>book.author.name</literal>,
             <literal>book["author"].name</literal>,
-            <literal>book.author.["name"]</literal>,
+            <literal>book.author["name"]</literal>,
             <literal>book["author"]["name"]</literal>.</para>
 
             <para>When you use the dot syntax, the same restrictions apply
@@ -18896,15 +18896,14 @@ Primary content continued</programlisting>
           linkend="dgui_template_exp_missing">missing value handler
           operators</link>). It can handle all kind of errors that occurs when
           the block is executed (i.e. not syntactical errors, which are
-          detected earlier). And it's meant to enclose bigger template
-          fragments, where error can occur at various points. For example, you
-          have a part in your template that deals with printing
-          advertisements, but that's not the primary content of the page, so
-          you don't want your whole page be down just because some error
-          occurs with the printing of the advertisements (say, because of a
-          temporal database server faliure). So you put the whole
-          advertisement printing into an <literal><replaceable>attempt
-          block</replaceable></literal>.</para>
+          detected earlier). It meant to enclose bigger template fragments,
+          where error can occur at various points. For example, you have a
+          part in your template that deals with printing advertisements, but
+          that's not the primary content of the page, so you don't want your
+          whole page be down just because some error occurs with the printing
+          of the advertisements (say, because of a database server outage). So
+          you put the whole advertisement printing into an
+          <literal><replaceable>attempt block</replaceable></literal>.</para>
 
           <para>In some environments programmers configure FreeMarker so that
           it doesn't abort template execution for certain errors, but
@@ -18921,11 +18920,17 @@ Primary content continued</programlisting>
           references to special variable are started with dot (for example:
           <literal>${.error}</literal>).</para>
 
-          <para><phrase role="forProgrammers">Errors occurring during template
-          execution are always <link
-          linkend="pgui_misc_logging">logged</link>, even if they occur inside
-          an <literal><replaceable>attempt
-          block</replaceable></literal>.</phrase></para>
+          <para><phrase role="forProgrammers">By default errors occurring
+          inside an <literal><replaceable>attempt
+          block</replaceable></literal> are logged as errors. This is because
+          <literal>attempt</literal> is not supposed to be a general purpose
+          error handler mechanism, like <literal>try</literal> is in Java.
+          It's for decreasing the impact of unexpected errors, by making it
+          possible that only part of the page is going down, instead of the
+          whole page. But it's still an error, something that someone should
+          fix. (The way this error is reported can be customized with the
+          <literal>attempt_exception_reporter</literal> configuration
+          setting.)</phrase></para>
         </section>
       </section>
 
@@ -26855,6 +26860,37 @@ TemplateModel x = env.getVariable("x");  // get 
variable x</programlisting>
 
           <itemizedlist>
             <listitem>
+              <para>Added new configuration setting,
+              <literal>attempt_exception_reporter</literal>
+              
(<literal>Configurable.setAttemptExceptionReporter(AttemptExceptionReporter)</literal>),
+              to allow the customization of how the exceptions handled (and
+              thus suppressed) by the <link
+              linkend="ref.directive.attempt"><literal>attempt</literal>
+              directive</link> are reported. The default
+              <literal>AttemptExceptionReporter</literal> logs the exception
+              as an error, just as it was in earlier versions, though now the
+              error message will indicate that the exception was thrown inside
+              an <literal>attempt</literal> directive block.</para>
+            </listitem>
+
+            <listitem>
+              <para>Bug fixed: When the
+              <literal>TemplateExceptionHandler</literal> suppresses (i.e.,
+              doesn't re-throw) an exception, the <link
+              linkend="ref.directive.attempt"><literal>attempt</literal>
+              directive</link> won't log it anymore. (To be precise, the
+              <literal>AttemptExceptionReporter</literal> won't be invoked for
+              it anymore; the default one logs as error.)</para>
+            </listitem>
+
+            <listitem>
+              <para>When logging error due to an error in an <link
+              linkend="ref.directive.attempt"><literal>attempt</literal>
+              directive</link> block, the log message now indicates that error
+              was inside an <literal>attempt</literal> block.</para>
+            </listitem>
+
+            <listitem>
               <para>Bug fixed (<link
               
xlink:href="https://issues.apache.org/jira/browse/FREEMARKER-52";>FREEMARKER-52</link>):
               When setting the <literal>output_format</literal> with

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/test/java/freemarker/core/AttemptLoggingTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/freemarker/core/AttemptLoggingTest.java 
b/src/test/java/freemarker/core/AttemptLoggingTest.java
new file mode 100644
index 0000000..edf3d18
--- /dev/null
+++ b/src/test/java/freemarker/core/AttemptLoggingTest.java
@@ -0,0 +1,77 @@
+package freemarker.core;
+
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+
+import java.io.IOException;
+import java.io.Writer;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.Test;
+
+import freemarker.template.AttemptExceptionReporter;
+import freemarker.template.TemplateException;
+import freemarker.template.TemplateExceptionHandler;
+import freemarker.test.TemplateTest;
+
+public class AttemptLoggingTest extends TemplateTest {
+
+    @Test
+    public void standardConfigTest() throws IOException, TemplateException {
+        assertOutput("<#attempt>${missingVar1}<#recover>r</#attempt>", "r");
+        // Here, we should have an ERROR entry in the log that refers to an 
exception in an #attempt block. But we can't
+        // easily assert that automatically, so it has to be checked 
manually...
+        
+        
getConfiguration().setAttemptExceptionReporter(AttemptExceptionReporter.LOG_WARN_REPORTER);
+        assertOutput("<#attempt>${missingVar2}<#recover>r</#attempt>", "r");
+        // Again, it must be checked manually if there's a WARN entry
+    }
+
+    @Test
+    public void customConfigTest() throws IOException, TemplateException {
+        List<String> reports = new ArrayList<String>();
+        getConfiguration().setAttemptExceptionReporter(new 
TestAttemptExceptionReporter(reports));
+        
+        assertOutput(
+                "<#attempt>${missingVar1}<#recover>r</#attempt>"
+                + "<#attempt>${missingVar2}<#recover>r</#attempt>",
+                "rr");
+        assertEquals(2, reports.size());
+        assertThat(reports.get(0), containsString("missingVar1"));
+        assertThat(reports.get(1), containsString("missingVar2"));
+    }
+
+    @Test
+    public void dontReportSuppressedExceptionsTest() throws IOException, 
TemplateException {
+        List<String> reports = new ArrayList<String>();
+        getConfiguration().setAttemptExceptionReporter(new 
TestAttemptExceptionReporter(reports));
+        
+        getConfiguration().setTemplateExceptionHandler(new 
TemplateExceptionHandler() {
+            public void handleTemplateException(TemplateException te, 
Environment env, Writer out) throws TemplateException {
+                try {
+                    out.write("[E]");
+                } catch (IOException e) {
+                    throw new TemplateException("Failed to write to the 
output", e, env);
+                }
+            }
+        });
+
+        assertOutput("<#attempt>${missingVar1}t<#recover>r</#attempt>", 
"[E]t");
+        
+        assertEquals(0, reports.size());
+    }
+    
+    private static final class TestAttemptExceptionReporter implements 
AttemptExceptionReporter {
+        private final List<String> reports;
+
+        private TestAttemptExceptionReporter(List<String> reports) {
+            this.reports = reports;
+        }
+
+        public void report(TemplateException te, Environment env) {
+            reports.add(te.getMessage());
+        }
+    }
+    
+}

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/test/java/freemarker/core/TemplateConfigurationTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/freemarker/core/TemplateConfigurationTest.java 
b/src/test/java/freemarker/core/TemplateConfigurationTest.java
index 837f6b6..bd397d4 100644
--- a/src/test/java/freemarker/core/TemplateConfigurationTest.java
+++ b/src/test/java/freemarker/core/TemplateConfigurationTest.java
@@ -48,6 +48,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 
 import freemarker.cache.StringTemplateLoader;
+import freemarker.template.AttemptExceptionReporter;
 import freemarker.template.Configuration;
 import freemarker.template.SimpleObjectWrapper;
 import freemarker.template.Template;
@@ -170,6 +171,7 @@ public class TemplateConfigurationTest {
         SETTING_ASSIGNMENTS.put("outputEncoding", "utf-16");
         SETTING_ASSIGNMENTS.put("showErrorTips", false);
         SETTING_ASSIGNMENTS.put("templateExceptionHandler", 
TemplateExceptionHandler.IGNORE_HANDLER);
+        SETTING_ASSIGNMENTS.put("attemptExceptionReporter", 
AttemptExceptionReporter.LOG_WARN_REPORTER);
         SETTING_ASSIGNMENTS.put("timeFormat", "@HH:mm");
         SETTING_ASSIGNMENTS.put("timeZone", NON_DEFAULT_TZ);
         SETTING_ASSIGNMENTS.put("arithmeticEngine", 
ArithmeticEngine.CONSERVATIVE_ENGINE);

http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/304df82a/src/test/java/freemarker/template/ConfigurationTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/freemarker/template/ConfigurationTest.java 
b/src/test/java/freemarker/template/ConfigurationTest.java
index 15ba915..2beeff0 100644
--- a/src/test/java/freemarker/template/ConfigurationTest.java
+++ b/src/test/java/freemarker/template/ConfigurationTest.java
@@ -1206,6 +1206,18 @@ public class ConfigurationTest extends TestCase {
         cfg.setSetting(Configurable.LOG_TEMPLATE_EXCEPTIONS_KEY, "false");
         assertEquals(false, cfg.getLogTemplateExceptions());
     }
+
+    public void testSetAttemptExceptionReporter() throws TemplateException {
+        Configuration cfg = new Configuration(Configuration.VERSION_2_3_0);
+        assertEquals(AttemptExceptionReporter.LOG_ERROR_REPORTER, 
cfg.getAttemptExceptionReporter());
+        assertFalse(cfg.isAttemptExceptionReporterExplicitlySet());
+        cfg.setSetting(Configurable.ATTEMPT_EXCEPTION_REPORTER_KEY, 
"log_warn");
+        assertEquals(AttemptExceptionReporter.LOG_WARN_REPORTER, 
cfg.getAttemptExceptionReporter());
+        assertTrue(cfg.isAttemptExceptionReporterExplicitlySet());
+        cfg.setSetting(Configurable.ATTEMPT_EXCEPTION_REPORTER_KEY, "default");
+        assertEquals(AttemptExceptionReporter.LOG_ERROR_REPORTER, 
cfg.getAttemptExceptionReporter());
+        assertFalse(cfg.isAttemptExceptionReporterExplicitlySet());
+    }
     
     public void testSharedVariables() throws TemplateModelException {
         Configuration cfg = new Configuration();


Reply via email to