-
-
Notifications
You must be signed in to change notification settings - Fork 973
GrailsUtil: honor stackTraceFiltererClass and logFullStackTraceOnFilter #15666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 7.1.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,11 @@ | |
| import org.apache.commons.logging.Log; | ||
| import org.apache.commons.logging.LogFactory; | ||
|
|
||
| import org.springframework.beans.BeanUtils; | ||
|
|
||
| import grails.config.Config; | ||
| import grails.config.Settings; | ||
| import grails.core.GrailsApplication; | ||
| import org.grails.exceptions.reporting.DefaultStackTraceFilterer; | ||
| import org.grails.exceptions.reporting.StackTraceFilterer; | ||
|
|
||
|
|
@@ -36,7 +41,15 @@ public class GrailsUtil { | |
|
|
||
| private static final Log LOG = LogFactory.getLog(GrailsUtil.class); | ||
| private static final boolean LOG_DEPRECATED = Boolean.valueOf(System.getProperty("grails.log.deprecated", String.valueOf(Environment.isDevelopmentMode()))); | ||
| private static final StackTraceFilterer stackFilterer = new DefaultStackTraceFilterer(); | ||
|
|
||
| /** | ||
| * Lazily-resolved filterer used by {@link #printSanitizedStackTrace}, {@link #sanitizeRootCause} | ||
| * and {@link #deepSanitize}. Cached once a {@link GrailsApplication} is discoverable via | ||
| * {@link Holders#findApplication()} so the config-driven class and emission flag are read | ||
| * exactly once. Volatile to publish the cached value safely; double-checked init in | ||
| * {@link #resolveStackFilterer()}. | ||
| */ | ||
| private static volatile StackTraceFilterer stackFilterer; | ||
|
|
||
| private GrailsUtil() { | ||
| } | ||
|
|
@@ -106,7 +119,7 @@ public static void warn(String message) { | |
| } | ||
|
|
||
| public static void printSanitizedStackTrace(Throwable t, PrintWriter p) { | ||
| printSanitizedStackTrace(t, p, stackFilterer); | ||
| printSanitizedStackTrace(t, p, resolveStackFilterer()); | ||
| } | ||
|
|
||
| public static void printSanitizedStackTrace(Throwable t, PrintWriter p, StackTraceFilterer stackTraceFilterer) { | ||
|
|
@@ -144,7 +157,7 @@ public static Throwable extractRootCause(Throwable t) { | |
| * @return The root cause exception instance, with its stace trace modified to filter out grails runtime classes | ||
| */ | ||
| public static Throwable sanitizeRootCause(Throwable t) { | ||
| return stackFilterer.filter(extractRootCause(t)); | ||
| return resolveStackFilterer().filter(extractRootCause(t)); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -154,7 +167,84 @@ public static Throwable sanitizeRootCause(Throwable t) { | |
| * @return The root cause exception instances, with stack trace modified to filter out grails runtime classes | ||
| */ | ||
| public static Throwable deepSanitize(Throwable t) { | ||
| return stackFilterer.filter(t, true); | ||
| return resolveStackFilterer().filter(t, true); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the {@link StackTraceFilterer} used by this class, lazily initialised from the | ||
| * Grails application config when one is discoverable. Honours | ||
| * {@link Settings#SETTING_LOGGING_STACKTRACE_FILTER_CLASS} (the filterer class — same key | ||
| * the exception resolver consults) and propagates | ||
| * {@link Settings#SETTING_LOG_FULL_STACKTRACE_ON_FILTER} to instances of | ||
| * {@link DefaultStackTraceFilterer}. | ||
| * | ||
| * <p>While no {@link GrailsApplication} is available (early-init paths, plain {@code main} | ||
| * usage, tests that don't wire one up) a fresh {@link DefaultStackTraceFilterer} is returned | ||
| * and <em>not</em> cached — so once the application context boots, the next call resolves | ||
| * the configured filterer for real. After that the value is cached for the lifetime of the | ||
| * JVM, matching the historical behaviour of the previous {@code static final} field. | ||
| */ | ||
| private static StackTraceFilterer resolveStackFilterer() { | ||
| StackTraceFilterer cached = stackFilterer; | ||
| if (cached != null) { | ||
| return cached; | ||
| } | ||
| GrailsApplication application = findApplicationQuietly(); | ||
| if (application == null) { | ||
| // No application discoverable yet — return an uncached default. A later call, | ||
| // once the context is up, will run through the configured-resolution branch | ||
| // and populate the cache. | ||
| return new DefaultStackTraceFilterer(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What scenario is this occuring? Now we're initializing this every access until the app is online. Why aren't we just setting this value earlier in the bootstrap context and then referencing outside of the bean scope? |
||
| } | ||
| synchronized (GrailsUtil.class) { | ||
| cached = stackFilterer; | ||
| if (cached != null) { | ||
| return cached; | ||
| } | ||
| stackFilterer = createConfiguredFilterer(application); | ||
| return stackFilterer; | ||
| } | ||
| } | ||
|
|
||
| private static GrailsApplication findApplicationQuietly() { | ||
| try { | ||
| return Holders.findApplication(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this throws it's likely trying to fetch this prior to the application being initialized. It seems like this would be a major issue because before this functioned regardless of the application being initialized. |
||
| } | ||
| catch (Throwable ignored) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private static StackTraceFilterer createConfiguredFilterer(GrailsApplication application) { | ||
| Class<? extends StackTraceFilterer> filtererClass = DefaultStackTraceFilterer.class; | ||
| boolean logOnFilter = true; | ||
| try { | ||
| Config config = application.getConfig(); | ||
| if (config != null) { | ||
| filtererClass = config.getProperty( | ||
| Settings.SETTING_LOGGING_STACKTRACE_FILTER_CLASS, | ||
| Class.class, DefaultStackTraceFilterer.class); | ||
| logOnFilter = config.getProperty( | ||
| Settings.SETTING_LOG_FULL_STACKTRACE_ON_FILTER, | ||
| Boolean.class, true); | ||
| } | ||
| } | ||
| catch (Throwable t) { | ||
| LOG.warn("Unable to resolve StackTraceFilterer config; using default: " + t.getMessage()); | ||
| } | ||
| StackTraceFilterer instance; | ||
| try { | ||
| instance = BeanUtils.instantiateClass(filtererClass, StackTraceFilterer.class); | ||
| } | ||
| catch (Throwable t) { | ||
| LOG.warn("Problem instantiating configured StackTraceFilterer [" + filtererClass.getName() + | ||
| "], falling back to default: " + t.getMessage()); | ||
| instance = new DefaultStackTraceFilterer(); | ||
| } | ||
| if (instance instanceof DefaultStackTraceFilterer) { | ||
| ((DefaultStackTraceFilterer) instance).setLogFullStackTraceOnFilter(logOnFilter); | ||
| } | ||
| return instance; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| /* | ||
| * 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 | ||
| * | ||
| * https://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 grails.util | ||
|
|
||
| import grails.config.Config | ||
| import grails.core.GrailsApplication | ||
| import org.grails.exceptions.reporting.DefaultStackTraceFilterer | ||
| import org.grails.exceptions.reporting.StackTraceFilterer | ||
| import spock.lang.Specification | ||
|
|
||
| /** | ||
| * Verifies that {@link GrailsUtil#deepSanitize}, {@link GrailsUtil#sanitizeRootCause} and | ||
| * {@link GrailsUtil#printSanitizedStackTrace} honour the same config keys as | ||
| * {@code GrailsExceptionResolver} — {@code grails.logging.stackTraceFiltererClass} and | ||
| * {@code grails.exceptionresolver.logFullStackTraceOnFilter}. | ||
| * | ||
| * The cached filterer is reset between scenarios via reflection so each test sees a | ||
| * fresh lookup against its own {@link GrailsApplication}. | ||
| */ | ||
| class GrailsUtilStackFiltererSpec extends Specification { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you point me to an integration test that exercises this logic? |
||
|
|
||
| GrailsApplication previousApplication | ||
|
|
||
| def setup() { | ||
| previousApplication = Holders.findApplication() | ||
| resetCachedFilterer() | ||
| } | ||
|
|
||
| def cleanup() { | ||
| Holders.setGrailsApplication(previousApplication) | ||
| resetCachedFilterer() | ||
| } | ||
|
|
||
| def 'falls back to a DefaultStackTraceFilterer when no GrailsApplication is discoverable'() { | ||
| given: | ||
| Holders.setGrailsApplication(null) | ||
|
|
||
| when: | ||
| def ex = new RuntimeException('boom') | ||
| GrailsUtil.deepSanitize(ex) | ||
|
|
||
| then: | ||
| noExceptionThrown() | ||
| } | ||
|
|
||
| def 'honours grails.logging.stackTraceFiltererClass'() { | ||
| given: | ||
| def application = Mock(GrailsApplication) | ||
| def config = Mock(Config) | ||
| config.getProperty('grails.logging.stackTraceFiltererClass', Class, DefaultStackTraceFilterer) >> RecordingStackTraceFilterer | ||
| config.getProperty('grails.exceptionresolver.logFullStackTraceOnFilter', Boolean, true) >> true | ||
| application.getConfig() >> config | ||
| Holders.setGrailsApplication(application) | ||
|
|
||
| when: | ||
| def ex = new RuntimeException('boom') | ||
| GrailsUtil.deepSanitize(ex) | ||
|
|
||
| then: | ||
| RecordingStackTraceFilterer.lastInstance != null | ||
| RecordingStackTraceFilterer.lastInstance.recursiveCalls == 1 | ||
| } | ||
|
|
||
| def 'propagates logFullStackTraceOnFilter to DefaultStackTraceFilterer instances'() { | ||
| given: | ||
| def application = Mock(GrailsApplication) | ||
| def config = Mock(Config) | ||
| config.getProperty('grails.logging.stackTraceFiltererClass', Class, DefaultStackTraceFilterer) >> DefaultStackTraceFilterer | ||
| config.getProperty('grails.exceptionresolver.logFullStackTraceOnFilter', Boolean, true) >> false | ||
| application.getConfig() >> config | ||
| Holders.setGrailsApplication(application) | ||
|
|
||
| and: 'captured StackTrace logger output' | ||
| def originalErr = System.err | ||
| def baos = new ByteArrayOutputStream() | ||
| System.setErr(new PrintStream(baos, true)) | ||
|
|
||
| when: | ||
| GrailsUtil.deepSanitize(new RuntimeException('boom')) | ||
|
|
||
| then: | ||
| System.err.flush() | ||
| !baos.toString().contains('ERROR StackTrace') | ||
|
|
||
| cleanup: | ||
| System.setErr(originalErr) | ||
| } | ||
|
|
||
| private static void resetCachedFilterer() { | ||
| def field = GrailsUtil.getDeclaredField('stackFilterer') | ||
| field.accessible = true | ||
| field.set(null, null) | ||
| } | ||
|
|
||
| static class RecordingStackTraceFilterer implements StackTraceFilterer { | ||
| static RecordingStackTraceFilterer lastInstance | ||
| int singleCalls = 0 | ||
| int recursiveCalls = 0 | ||
|
|
||
| RecordingStackTraceFilterer() { | ||
| lastInstance = this | ||
| } | ||
|
|
||
| Throwable filter(Throwable source) { | ||
| singleCalls++ | ||
| return source | ||
| } | ||
|
|
||
| Throwable filter(Throwable source, boolean recursive) { | ||
| recursiveCalls++ | ||
| return source | ||
| } | ||
|
|
||
| void addInternalPackage(String name) {} | ||
| void setCutOffPackage(String cutOffPackage) {} | ||
| void setShouldFilter(boolean shouldFilter) {} | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before it always used one instance, but now it's using multiple potentially. This seems backwards.