Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 813FD200CB5 for ; Wed, 28 Jun 2017 01:42:56 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 7FE4B160BE9; Tue, 27 Jun 2017 23:42:56 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id CE458160BDC for ; Wed, 28 Jun 2017 01:42:54 +0200 (CEST) Received: (qmail 2774 invoked by uid 500); 27 Jun 2017 23:42:54 -0000 Mailing-List: contact notifications-help@freemarker.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@freemarker.incubator.apache.org Delivered-To: mailing list notifications@freemarker.incubator.apache.org Received: (qmail 2765 invoked by uid 99); 27 Jun 2017 23:42:53 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 27 Jun 2017 23:42:53 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id E60D5185EB6 for ; Tue, 27 Jun 2017 23:42:52 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: -4.221 X-Spam-Level: X-Spam-Status: No, score=-4.221 tagged_above=-999 required=6.31 tests=[KAM_ASCII_DIVIDERS=0.8, RCVD_IN_DNSWL_HI=-5, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, RP_MATCHES_RCVD=-0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id vp4vJAyXMpOH for ; Tue, 27 Jun 2017 23:42:45 +0000 (UTC) Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with SMTP id EF29C60CD9 for ; Tue, 27 Jun 2017 23:42:43 +0000 (UTC) Received: (qmail 2449 invoked by uid 99); 27 Jun 2017 23:42:42 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 27 Jun 2017 23:42:42 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 75826DFAF5; Tue, 27 Jun 2017 23:42:42 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: ddekany@apache.org To: notifications@freemarker.incubator.apache.org Message-Id: <1463fcfb149d4d1898de521b6c90471a@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: incubator-freemarker git commit: Improved #attempt error reporting: Date: Tue, 27 Jun 2017 23:42:42 +0000 (UTC) archived-at: Tue, 27 Jun 2017 23:42:56 -0000 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 Authored: Wed Jun 28 01:42:10 2017 +0200 Committer: ddekany 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(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 in 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). + * + *

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}. + * + *

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. + * + *

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. + * + *

  • {@code "attempt_exception_reporter"}: + * See {@link #setAttemptExceptionReporter(AttemptExceptionReporter)}. + *
    String value: If the value contains dot, then it's interpreted as an object builder + * expression. + * 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. * *

  • {@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. * *

    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. * - *

    Note that {@link StopException}-s (raised by {@code #stop}) won't be captured. + *

    Note that {@link StopException}-s (raised by {@code #stop}) won't be captured here. * *

    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 book[test]. More examples; these are all equivalent: book.author.name, book["author"].name, - book.author.["name"], + book.author["name"], book["author"]["name"]. When you use the dot syntax, the same restrictions apply @@ -18896,15 +18896,14 @@ Primary content continued linkend="dgui_template_exp_missing">missing value handler operators). 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 attempt - block. + 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 + attempt block. 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 references to special variable are started with dot (for example: ${.error}). - Errors occurring during template - execution are always logged, even if they occur inside - an attempt - block. + By default errors occurring + inside an attempt + block are logged as errors. This is because + attempt is not supposed to be a general purpose + error handler mechanism, like 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. (The way this error is reported can be customized with the + attempt_exception_reporter configuration + setting.) @@ -26855,6 +26860,37 @@ TemplateModel x = env.getVariable("x"); // get variable x + 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.) + + + + When logging error due to an error in an attempt + directive block, the log message now indicates that error + was inside an attempt block. + + + Bug fixed (FREEMARKER-52): When setting the output_format 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", "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", "r"); + // Again, it must be checked manually if there's a WARN entry + } + + @Test + public void customConfigTest() throws IOException, TemplateException { + List reports = new ArrayList(); + getConfiguration().setAttemptExceptionReporter(new TestAttemptExceptionReporter(reports)); + + assertOutput( + "<#attempt>${missingVar1}<#recover>r" + + "<#attempt>${missingVar2}<#recover>r", + "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 reports = new ArrayList(); + 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", "[E]t"); + + assertEquals(0, reports.size()); + } + + private static final class TestAttemptExceptionReporter implements AttemptExceptionReporter { + private final List reports; + + private TestAttemptExceptionReporter(List 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();