freemarker-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ddek...@apache.org
Subject incubator-freemarker git commit: Improved #attempt error reporting:
Date Tue, 27 Jun 2017 23:42:42 GMT
Repository: incubator-freemarker
Updated Branches:
  refs/heads/2.3-gae 4b5b7e871 -> 304df82a7


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-gae
Commit: 304df82a7501f66386b2c5be740d377961f93dc5
Parents: 4b5b7e8
Author: ddekany <ddekany@apache.org>
Authored: Wed Jun 28 01:42:10 2017 +0200
Committer: ddekany <ddekany@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();



Mime
View raw message