jdaugherty commented on code in PR #15666:
URL: https://github.com/apache/grails-core/pull/15666#discussion_r3610658937
##########
grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java:
##########
@@ -131,6 +132,7 @@ public void setServletContext(ServletContext
servletContext) {
public void setGrailsApplication(GrailsApplication grailsApplication) {
this.grailsApplication = grailsApplication;
createStackFilterer();
+ GrailsUtil.initializeStackFilterer(grailsApplication);
Review Comment:
This still couples `grails-core` static state to a `grails-web-mvc` bean:
any context where this resolver isn't wired (a non-web app, or an app that
replaces the `exceptionHandler` bean with a different resolver) silently stays
on the fallback with no warning.
Isn't this what `GrailsBootstrapRegistryInitializer` is meant to solve? Its
close listener fires at the end of `prepareContext()` — environment fully
bound, before `refresh()` — so `grails-core` could initialize its own utility
there for every app type, and the configured filterer would be active before
any bean instantiates (startup failures would honour the config too). It could
also promote the filterer as a singleton (the way `PluginDiscovery.BEAN_NAME`
is promoted) so this resolver consumes the same instance instead of
instantiating a second copy of a possibly custom filterer class.
##########
grails-doc/src/en/guide/conf/config/logging/loggingFullStackTraces.adoc:
##########
@@ -97,6 +97,13 @@ log record. It means non-resolver code paths (for example, a
scheduled job that
`GrailsUtil.sanitizeRootCause(ex)` before logging via its own logger) continue
to populate the `StackTrace`
appender without an explicit emission call.
+NOTE: `GrailsUtil` honours the same config keys as the exception resolver
Review Comment:
This NOTE bakes internal wiring into the user guide —
`GrailsExceptionResolver` and `GroovyPageView` are both `org.grails.*` internal
packages, and we shouldn't reference internal APIs in user-facing docs. Can we
state the behaviour only, e.g. "`GrailsUtil` honours the same config keys once
the application has started, so this property controls both resolver-driven and
`GrailsUtil`-driven emission (including GSP view-rendering errors)"? Then the
docs also don't need to change when the wiring does.
##########
grails-core/src/test/groovy/grails/util/GrailsUtilStackFiltererSpec.groovy:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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
+
+import java.lang.reflect.Field
+
+/**
+ * Verifies that {@link GrailsUtil#initializeStackFilterer} resolves the
configured filterer class
+ * from the application's config and propagates {@code
grails.exceptionresolver.logFullStackTraceOnFilter}
+ * to {@link DefaultStackTraceFilterer} instances. Before initialization the
FALLBACK_FILTERER
+ * (a {@link DefaultStackTraceFilterer} singleton) is used so CLI/test/main
paths work unchanged.
+ */
+class GrailsUtilStackFiltererSpec extends Specification {
+
+ StackTraceFilterer previous
+
+ def setup() {
+ previous = currentFilterer()
+ setFilterer(fallbackFilterer())
+ }
+
+ def cleanup() {
+ setFilterer(previous)
+ }
+
+ def 'deepSanitize uses the fallback filterer before
initializeStackFilterer is called'() {
+ when:
+ GrailsUtil.deepSanitize(new RuntimeException('boom'))
+
+ then:
+ noExceptionThrown()
+ currentFilterer().is(fallbackFilterer())
+ }
+
+ def 'initializeStackFilterer is a no-op when application is null'() {
+ when:
+ GrailsUtil.initializeStackFilterer(null)
+
+ then:
+ currentFilterer().is(fallbackFilterer())
+ }
+
+ def 'initializeStackFilterer wires the class declared by
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
+
+ when:
+ GrailsUtil.initializeStackFilterer(application)
+ GrailsUtil.deepSanitize(new RuntimeException('boom'))
+
+ then:
+ currentFilterer() instanceof RecordingStackTraceFilterer
+ RecordingStackTraceFilterer.lastInstance.recursiveCalls == 1
+ }
+
+ def 'initializeStackFilterer 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
+
+ and: 'captured StackTrace logger output'
+ def originalErr = System.err
+ def baos = new ByteArrayOutputStream()
+ System.setErr(new PrintStream(baos, true))
+
+ when:
+ GrailsUtil.initializeStackFilterer(application)
+ GrailsUtil.deepSanitize(new RuntimeException('boom'))
+
+ then:
+ System.err.flush()
+ !baos.toString().contains('ERROR StackTrace')
Review Comment:
`StackTraceFiltererSpec` asserts on the `'Full Stack Trace:'` marker;
`'ERROR StackTrace'` depends on the logging pattern, so this negative assertion
can pass vacuously even when emission occurs. Please use the same marker, and
consider adding the positive control (flag `true` + initialized application →
emission does happen through `GrailsUtil.deepSanitize`) so the negative case is
proven meaningful.
##########
grails-core/src/test/groovy/grails/util/GrailsUtilStackFiltererSpec.groovy:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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
+
+import java.lang.reflect.Field
+
+/**
+ * Verifies that {@link GrailsUtil#initializeStackFilterer} resolves the
configured filterer class
+ * from the application's config and propagates {@code
grails.exceptionresolver.logFullStackTraceOnFilter}
+ * to {@link DefaultStackTraceFilterer} instances. Before initialization the
FALLBACK_FILTERER
+ * (a {@link DefaultStackTraceFilterer} singleton) is used so CLI/test/main
paths work unchanged.
+ */
+class GrailsUtilStackFiltererSpec extends Specification {
+
+ StackTraceFilterer previous
+
+ def setup() {
+ previous = currentFilterer()
+ setFilterer(fallbackFilterer())
+ }
+
+ def cleanup() {
+ setFilterer(previous)
+ }
+
+ def 'deepSanitize uses the fallback filterer before
initializeStackFilterer is called'() {
Review Comment:
`setup()` installs `FALLBACK_FILTERER` via reflection, so this feature
asserts the state the test itself just created rather than the class's initial
state. As written it can't fail. Either drop it or assert only the behaviour
(`deepSanitize` succeeds with no application wired).
##########
grails-doc/src/en/guide/upgrading/upgrading71x.adoc:
##########
@@ -849,3 +849,14 @@ Set to `false` to disable the side-effect emission and
rely solely on `logFullSt
output. The two flags interact — if both are enabled, a request exception with
N causes produces N+1 `StackTrace`
records (one resolver-driven plus one per throwable visited by the recursive
filter walk). The Logging Full
Stack Traces section of the user guide includes a matrix of behaviours for the
four flag combinations.
+
+`GrailsUtil` honours both `grails.logging.stackTraceFiltererClass` and
Review Comment:
Same doc concern as the user-guide NOTE: this names
`GrailsExceptionResolver.setGrailsApplication`,
`GroovyPageView.handleException`, and `GrailsUtil.initializeStackFilterer`
wiring details. For the upgrade notes, what a user needs is: (1) the two keys
now reach `GrailsUtil`-driven paths including GSP render errors, (2) before the
application context is initialized `GrailsUtil` falls back to the default
filterer (unchanged behaviour), and (3) apps that silenced the `StackTrace`
logger in logback purely to suppress GSP-render noise can now use
`logFullStackTraceOnFilter: false` instead. The internal method references
should be dropped.
##########
grails-core/src/main/groovy/grails/util/GrailsUtil.java:
##########
@@ -36,11 +41,51 @@ 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();
+
+ /**
+ * Default filterer used before {@link
#initializeStackFilterer(GrailsApplication)} runs (CLI,
+ * tests that don't boot a context, plain {@code main()} usage). Preserves
the pre-PR behaviour
+ * of a single hardcoded {@link DefaultStackTraceFilterer} instance for
the JVM lifetime when no
+ * application is wired.
+ */
+ private static final StackTraceFilterer FALLBACK_FILTERER = new
DefaultStackTraceFilterer();
+
+ /**
+ * Active filterer for {@link #printSanitizedStackTrace}, {@link
#sanitizeRootCause} and
+ * {@link #deepSanitize}. Starts as {@link #FALLBACK_FILTERER} and is
replaced with a
+ * config-driven instance when {@link
#initializeStackFilterer(GrailsApplication)} runs during
+ * Grails bootstrap. Volatile so the bootstrap-time write publishes safely
to the request
+ * threads that read it later.
+ */
+ private static volatile StackTraceFilterer stackFilterer =
FALLBACK_FILTERER;
private GrailsUtil() {
}
+ /**
+ * Installs a {@link StackTraceFilterer} resolved from the given
application's config, replacing
+ * the default fallback. Reads {@link
Settings#SETTING_LOGGING_STACKTRACE_FILTER_CLASS} for the
+ * filterer class and propagates {@link
Settings#SETTING_LOG_FULL_STACKTRACE_ON_FILTER} to
+ * instances of {@link DefaultStackTraceFilterer}. Called by {@code
GrailsExceptionResolver}
+ * during Spring bean wiring (which is the same point the resolver
consults these keys for its
+ * own filterer), so request-time callers of the static {@code
sanitize}/{@code deepSanitize}
+ * methods see the configured instance.
+ *
+ * <p>No-ops when {@code application} is null. Safe to call more than once
— the last successful
+ * invocation wins.
+ *
+ * @since 7.1.5
+ */
+ public static void initializeStackFilterer(GrailsApplication application) {
Review Comment:
If initialization moves to the bootstrap-registry phase, this should take
the `Environment` (or simply the resolved class + boolean) rather than
`GrailsApplication` — the `grailsApplication` bean doesn't exist until refresh,
while `Environment.getProperty(key, Class.class, default)` performs the same
String→Class conversion `Config` does. That keeps `GrailsUtil` free of any
dependency on the application object and lets the filterer be configured before
the context refreshes.
##########
grails-core/src/test/groovy/grails/util/GrailsUtilStackFiltererSpec.groovy:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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
+
+import java.lang.reflect.Field
+
+/**
+ * Verifies that {@link GrailsUtil#initializeStackFilterer} resolves the
configured filterer class
+ * from the application's config and propagates {@code
grails.exceptionresolver.logFullStackTraceOnFilter}
+ * to {@link DefaultStackTraceFilterer} instances. Before initialization the
FALLBACK_FILTERER
+ * (a {@link DefaultStackTraceFilterer} singleton) is used so CLI/test/main
paths work unchanged.
+ */
+class GrailsUtilStackFiltererSpec extends Specification {
Review Comment:
Following up on the earlier integration-test thread: yes, please add the
`@Integration` spec — `app2` is the right home since it already exercises
exception handling. Boot with `grails.logging.stackTraceFiltererClass` set in
config, call `GrailsUtil.deepSanitize` in the running app, and assert the
configured class is used. That's the piece the unit spec can't prove: that the
bootstrap wiring actually fires in a real app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
