codeconsole commented on code in PR #15666: URL: https://github.com/apache/grails-core/pull/15666#discussion_r3653346847
########## grails-doc/src/en/guide/upgrading/upgrading71x.adoc: ########## @@ -849,3 +849,11 @@ 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. + +Both `grails.logging.stackTraceFiltererClass` and `grails.exceptionresolver.logFullStackTraceOnFilter` now Review Comment: Moved to `upgrading80x.adoc` §38. 7.1 §2.13 is back to 7.1-only content. ########## grails-doc/src/en/guide/conf/config/logging/loggingFullStackTraces.adoc: ########## @@ -97,6 +97,14 @@ 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: Both `grails.logging.stackTraceFiltererClass` and `grails.exceptionresolver.logFullStackTraceOnFilter` +apply outside the exception resolver too — including GSP view-render exceptions — so this property controls +emission everywhere the filterer is used, not just for resolver-driven requests. Before the application +context is initialized (CLI usage, tests that don't boot a context, plain `main()`), the pre-7.1 fallback Review Comment: Both fixed — now "any context not started through `SpringApplication`", plus a sentence that the filterer is JVM-global and last-booted wins. ########## grails-test-examples/app2/src/integration-test/groovy/app2/GrailsUtilStackFiltererIntegrationSpec.groovy: ########## @@ -0,0 +1,49 @@ +/* + * 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 app2 + +import grails.plugin.geb.ContainerGebSpec +import grails.testing.mixin.integration.Integration +import org.springframework.test.context.TestPropertySource + +/** + * Boots the full application with {@code grails.logging.stackTraceFiltererClass} set, then drives a + * non-resolver code path ({@link grails.util.GrailsUtil#deepSanitize}) to prove the bootstrap wiring in + * {@code org.apache.grails.core.GrailsBootstrapRegistryInitializer} actually installs the configured + * filterer in a real running app -- the piece {@code GrailsUtilStackFiltererSpec} and + * {@code GrailsBootstrapRegistryInitializerSpec} can't prove on their own, since they exercise the + * classes directly rather than through a real Spring Boot bootstrap. + * + * <p>The {@link TestPropertySource} on this class gives it a merged context configuration distinct from + * {@link ErrorsControllerSpec} and {@link NotFoundHandlerSpec}, so Spring boots a separate application + * context for it and the other specs in this module are unaffected by the custom filterer class. Review Comment: Paragraph rewritten — it said the opposite of what happens. Went with documenting the single-context assumption rather than a `ContextClosedEvent` reset, since resetting on close would break shutdown-time logging. `cleanupSpec` restores the default so it doesn't leak into the fork. ########## grails-test-examples/app2/src/integration-test/groovy/app2/GrailsUtilStackFiltererIntegrationSpec.groovy: ########## @@ -0,0 +1,49 @@ +/* + * 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 app2 + +import grails.plugin.geb.ContainerGebSpec +import grails.testing.mixin.integration.Integration +import org.springframework.test.context.TestPropertySource + +/** + * Boots the full application with {@code grails.logging.stackTraceFiltererClass} set, then drives a + * non-resolver code path ({@link grails.util.GrailsUtil#deepSanitize}) to prove the bootstrap wiring in + * {@code org.apache.grails.core.GrailsBootstrapRegistryInitializer} actually installs the configured + * filterer in a real running app -- the piece {@code GrailsUtilStackFiltererSpec} and + * {@code GrailsBootstrapRegistryInitializerSpec} can't prove on their own, since they exercise the + * classes directly rather than through a real Spring Boot bootstrap. + * + * <p>The {@link TestPropertySource} on this class gives it a merged context configuration distinct from + * {@link ErrorsControllerSpec} and {@link NotFoundHandlerSpec}, so Spring boots a separate application + * context for it and the other specs in this module are unaffected by the custom filterer class. + */ +@Integration(applicationClass = Application) +@TestPropertySource(properties = ['grails.logging.stackTraceFiltererClass=app2.RecordingStackTraceFilterer']) Review Comment: Added in `GrailsBootstrapRegistryInitializerSpec` as you suggested — `MapPropertySource` holding the `Class`, no second context. Positive case plus a `Class` that doesn't implement the interface. ########## grails-core/src/test/groovy/grails/util/GrailsUtilStackFiltererSpec.groovy: ########## @@ -0,0 +1,173 @@ +/* + * 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 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(StackTraceFilterer)} installs the given + * filterer for {@link GrailsUtil#deepSanitize}, {@link GrailsUtil#sanitizeRootCause} and + * {@link GrailsUtil#printSanitizedStackTrace}, and that the pre-initialization fallback (a + * {@link DefaultStackTraceFilterer}) is used until then. Config-driven resolution (the configured + * class + {@code logFullStackTraceOnFilter}) now happens in + * {@code org.apache.grails.core.GrailsBootstrapRegistryInitializer}, covered separately by + * {@code GrailsBootstrapRegistryInitializerSpec}. + */ +class GrailsUtilStackFiltererSpec extends Specification { + + StackTraceFilterer previous + + def setup() { + previous = currentFilterer() + setFilterer(fallbackFilterer()) + } + + def cleanup() { + setFilterer(previous) + } + + def 'deepSanitize does not throw before initializeStackFilterer is called'() { + when: + GrailsUtil.deepSanitize(new RuntimeException('boom')) + + then: + noExceptionThrown() + } + + def 'initializeStackFilterer is a no-op when filterer is null'() { + when: + GrailsUtil.initializeStackFilterer(null) + + then: + currentFilterer().is(fallbackFilterer()) + } + + def 'initializeStackFilterer installs the given filterer'() { + given: + def filterer = new RecordingStackTraceFilterer() + + when: + GrailsUtil.initializeStackFilterer(filterer) + GrailsUtil.deepSanitize(new RuntimeException('boom')) + + then: + currentFilterer().is(filterer) + filterer.recursiveCalls == 1 + } + + def 'last initializeStackFilterer call wins when invoked more than once'() { + given: + def first = new RecordingStackTraceFilterer() + def second = new RecordingStackTraceFilterer() + + when: + GrailsUtil.initializeStackFilterer(first) + GrailsUtil.initializeStackFilterer(second) + + then: + currentFilterer().is(second) + } + + def 'installed DefaultStackTraceFilterer honours logFullStackTraceOnFilter=false'() { + given: 'captured System.err' + def originalErr = System.err + def baos = new ByteArrayOutputStream() + System.setErr(new PrintStream(baos, true)) + + and: 'a filterer with the side-effect emission disabled' + def quietFilterer = new DefaultStackTraceFilterer() + quietFilterer.logFullStackTraceOnFilter = false + + when: + GrailsUtil.initializeStackFilterer(quietFilterer) + GrailsUtil.deepSanitize(exceptionWithApplicationFrame()) + + then: "no 'Full Stack Trace:' entry is emitted" + System.err.flush() + !baos.toString().contains(StackTraceFilterer.FULL_STACK_TRACE_MESSAGE) + + cleanup: + System.setErr(originalErr) + } + + def 'installed DefaultStackTraceFilterer emits Full Stack Trace by default'() { + given: 'captured System.err' + def originalErr = System.err + def baos = new ByteArrayOutputStream() + System.setErr(new PrintStream(baos, true)) + + and: 'a filterer with the default (enabled) side-effect emission' + def loudFilterer = new DefaultStackTraceFilterer() + + when: + GrailsUtil.initializeStackFilterer(loudFilterer) + GrailsUtil.deepSanitize(exceptionWithApplicationFrame()) + + then: "a 'Full Stack Trace:' entry is emitted -- the positive control proving the negative case above is meaningful" + System.err.flush() + baos.toString().contains(StackTraceFilterer.FULL_STACK_TRACE_MESSAGE) + + cleanup: + System.setErr(originalErr) + } + + private static RuntimeException exceptionWithApplicationFrame() { + def exception = new RuntimeException('boom') + exception.stackTrace = [ + new StackTraceElement('test.FooController', 'show', 'FooController.groovy', 6), + new StackTraceElement('java.lang.reflect.Method', 'invoke', 'Method.java', 580) + ] as StackTraceElement[] + exception + } + + private static StackTraceFilterer currentFilterer() { + filtererField().get(null) as StackTraceFilterer + } + + private static void setFilterer(StackTraceFilterer filterer) { + filtererField().set(null, filterer) + } + + private static StackTraceFilterer fallbackFilterer() { + Field field = GrailsUtil.getDeclaredField('FALLBACK_FILTERER') Review Comment: Reflection gone from both specs. Assertions go through `deepSanitize` / `sanitizeRootCause` / `printSanitizedStackTrace`; `initializeStackFilterer` is its own reset. Added coverage for the latter two, which weren't exercised. -- 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]
