GrailsUtil: honor stackTraceFiltererClass and logFullStackTraceOnFilter#15666
GrailsUtil: honor stackTraceFiltererClass and logFullStackTraceOnFilter#15666codeconsole wants to merge 3 commits into
Conversation
GrailsUtil held a hardcoded `private static final DefaultStackTraceFilterer` that ignored grails.logging.stackTraceFiltererClass and the new grails.exceptionresolver.logFullStackTraceOnFilter flag. Non-resolver callers of the filterer — most visibly GroovyPageView.handleException via GrailsUtil.deepSanitize on GSP view-render exceptions, plus scheduled jobs and custom code calling sanitizeRootCause/deepSanitize directly — produced StackTrace logger emissions that no config key could suppress. The only workaround was silencing the StackTrace logger in logback, which is too blunt and is called out in the user guide as a fallback rather than the intended control surface. Resolve the filterer lazily from Holders.findApplication().getConfig() on first use. Cache the resolved instance once an application is discoverable; pre-context callers (early init, plain main, tests) get a fresh uncached default so a later call after the context boots can still populate the cache. Propagate the on-filter flag to DefaultStackTraceFilterer instances the same way GrailsExceptionResolver.applyLogFullStackTraceOnFilter does, leaving custom StackTraceFilterer implementations responsible for their own logging policy. Default behavior is unchanged — unset config yields a DefaultStackTraceFilterer with logFullStackTraceOnFilter=true, matching the previous static field. All exception paths and instantiation failures fall back to the default with a logged warning so a bad config value can't break GrailsUtil callers. Adds GrailsUtilStackFiltererSpec covering the three branches (no app, custom class, on-filter propagation). Existing GrailsUtilTests and StackTraceFiltererSpec unchanged and passing. Documents the change in the Logging Full Stack Traces guide and the 7.1.x upgrade notes.
|
|
||
| private static GrailsApplication findApplicationQuietly() { | ||
| try { | ||
| return Holders.findApplication(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed — Holders.findApplication() doesn't actually throw (Holders.java:124-132 iterates strategies and returns null), and "regardless of the application being initialized" is the right invariant. Switched the whole approach in cdffb8f to a bootstrap hook: GrailsExceptionResolver.setGrailsApplication calls a new public GrailsUtil.initializeStackFilterer(application) during Spring bean wiring, so GrailsUtil no longer reaches into Holders at all. Hot path is now a single volatile read.
|
The CSV file contains only 1 row of actual data (row 2), which is empty. There are no review comments available to analyze. |
| * 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 { |
There was a problem hiding this comment.
Can you point me to an integration test that exercises this logic?
There was a problem hiding this comment.
Currently only the unit spec; same coverage level the parent PR #15564 shipped with for the resolver side of these keys. Happy to add an @Integration spec in grails-test-examples/app2 that boots a real context, sets grails.logging.stackTraceFiltererClass in application.yml, calls GrailsUtil.deepSanitize from inside the running app, and asserts the configured class was used. Prefer app2 (it already exercises exception handling) or a dedicated minimal app like config-report?
| if (cached != null) { | ||
| return cached; | ||
| } | ||
| GrailsApplication application = findApplicationQuietly(); |
There was a problem hiding this comment.
Before it always used one instance, but now it's using multiple potentially. This seems backwards.
There was a problem hiding this comment.
Right, that was a real regression of the prior invariant. Fixed in cdffb8f with a static final FALLBACK_FILTERER sentinel: the volatile stackFilterer field is initialised to the sentinel so CLI/test/main() paths see one shared instance for the JVM lifetime, matching the pre-PR static final shape. The bootstrap hook (C4 thread) then overwrites it once during Spring bean wiring.
| // 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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good point — switched to the bootstrap-hook approach in cdffb8f. GrailsExceptionResolver.setGrailsApplication now calls a new public GrailsUtil.initializeStackFilterer(application) immediately after createStackFilterer(), mirroring how the resolver consumes the same two config keys. The static final FALLBACK_FILTERER sentinel is the field's initial value so CLI/test/main() paths work unchanged, but in any Grails web app the configured filterer is pinned during bean wiring — no per-access initialization, no Holders lookup at runtime, hot path is one volatile read.
Address review feedback on apache#15666: - Replace the lazy resolveStackFilterer/findApplicationQuietly pattern with a deterministic bootstrap-hook: GrailsExceptionResolver.setGrailsApplication calls the new public GrailsUtil.initializeStackFilterer(application) during Spring bean wiring. Hot path collapses to a single volatile read. - Restore the single-instance invariant of the prior static final field via a static final FALLBACK_FILTERER sentinel. The volatile stackFilterer is initialised to FALLBACK_FILTERER so CLI, tests that don't boot a context, and plain main() usage continue to work with one shared instance. - Drop findApplicationQuietly and the surrounding try/catch. Holders no longer needs to be consulted from GrailsUtil at runtime; the resolver pushes the application in during its own wiring. GrailsUtilStackFiltererSpec rewritten to cover the new shape: fallback before init, null-application no-op, configured-class wiring, logFullStackTraceOnFilter propagation, and last-write-wins on repeat init. Existing GrailsExceptionResolverSpec, StackTraceFiltererSpec and GrailsUtilTests continue to pass; checkstyle clean. Docs in upgrading71x.adoc and loggingFullStackTraces.adoc updated to describe the bootstrap-hook wiring instead of the prior lazy resolution.
✅ All tests passed ✅Test Summary
🏷️ Commit: cdffb8f Learn more about TestLens at testlens.app. |
2 participants
grails.logging.stackTraceFiltererClassconfig value is not honored byGrailsUtil.deepSanitizeLet's say you customize the
stackTraceFiltererClassvia yml or groovyapplication.groovyGroovyPageView.java:103:
GrailsUtil.deepSanitizedumps a massive exception to your console and there is no way around it even if you defined a customstackTraceFiltererClass. Any gap error results in massive spam.This PR uses Holders to route to the appropriate
stackTraceFiltererClassSummary
Follow-up to #15564.
GrailsUtilheld a hardcodedprivate static final DefaultStackTraceFiltererthat ignored bothgrails.logging.stackTraceFiltererClass(the class-swap key thatGrailsExceptionResolverhonors) and the newgrails.exceptionresolver.logFullStackTraceOnFilterflag introduced by #15564.Non-resolver callers of the filterer — most visibly
GroovyPageView.handleException→GrailsUtil.deepSanitizeon GSP view-render exceptions, plus scheduled jobs and custom code callingsanitizeRootCause/deepSanitizedirectly — producedStackTracelogger emissions that no config key could suppress. The only workaround was silencing theStackTracelogger in logback, which the user guide already calls out as a fallback rather than the intended control surface.This PR closes that gap:
GrailsUtilnow resolves its filterer lazily on first use fromHolders.findApplication().getConfig(), honoring the same two keys the resolver honors and propagatinglogFullStackTraceOnFiltertoDefaultStackTraceFiltererinstances exactly the wayGrailsExceptionResolver.applyLogFullStackTraceOnFilter()does.Why this matters
Pre-7.1, applications could set
grails.logging.stackTraceFiltererClassto a custom filterer and reach every framework caller — includingGrailsUtil. The transition through #15564 and the years prior leftGrailsUtilas the one path the key never reached, which only becomes visible when GSP rendering throws (because that's the path that callsGrailsUtil.deepSanitizedirectly, bypassing the resolver). Apps hitting this today see 2 unfilterableStackTracerecords per render-time exception in 7.1.x (3 in 7.0.x, before #15564 dropped the redundant trailingfilter(source)), with no Grails-level knob.After this PR, setting
grails.exceptionresolver.logFullStackTraceOnFilter: falsesilences every caller of the filterer — resolver andGrailsUtilalike — without logback intervention.Design
main, tests that don't wire an application) get a fresh uncachedDefaultStackTraceFiltererso a later call after the context boots can still populate the cache. Post-context callers resolve from config and cache for JVM lifetime, matching the historical semantics of the previousstatic finalfield.DefaultStackTraceFiltererwithlogFullStackTraceOnFilter=true— identical to the previous hardcoded value. No behavior change for apps that don't set the new keys.Throwableand falls back toDefaultStackTraceFiltererwith a logged warning. A bad config value can't breakGrailsUtilcallers.setLogFullStackTraceOnFilteronly applied when the resolved instance is aDefaultStackTraceFilterer(or subclass) — matchingGrailsExceptionResolver.applyLogFullStackTraceOnFilter()exactly. CustomStackTraceFiltererimplementations remain responsible for their own logging policy.Tests
New
GrailsUtilStackFiltererSpeccovers three branches:DefaultStackTraceFiltererwhen noGrailsApplicationis discoverablegrails.logging.stackTraceFiltererClass(verifies a custom filterer is instantiated and invoked)logFullStackTraceOnFilter=falsetoDefaultStackTraceFiltererinstances (verifies noStackTraceemission)Existing
GrailsUtilTestsandStackTraceFiltererSpecare unchanged and continue to pass. Full:grails-core:testand:grails-web-mvc:testsuites green locally.Docs
loggingFullStackTraces.adoc: NOTE block clarifying thatGrailsUtilnow participates in the same emission policy as the resolver, so the matrix applies toGrailsUtil-driven paths too (including the GSP render path).upgrading71x.adoc§2.13: short paragraph notingGrailsUtilnow honors both keys and that apps previously silencing theStackTracelogger in logback purely to suppress GSP-render noise can now uselogFullStackTraceOnFilter: falseinstead.Test plan
grails.exceptionresolver.logFullStackTraceOnFilter: false: GSP-render exceptions produce zeroStackTracerecords (manually verified against a repro app on 7.2.0-SNAPSHOT after applying the patch)grails.logging.stackTraceFiltererClass:GrailsUtil.deepSanitizeroutes through the custom class (verified via custom-class test)