codeconsole commented on code in PR #15564:
URL: https://github.com/apache/grails-core/pull/15564#discussion_r3172367725
##########
grails-bootstrap/src/main/groovy/org/grails/exceptions/reporting/DefaultStackTraceFilterer.java:
##########
@@ -81,32 +89,32 @@ public Throwable filter(Throwable source, boolean
recursive) {
if (recursive) {
Throwable current = source;
while (current != null) {
- current = filter(current);
+ filter(current);
current = current.getCause();
}
+ return source;
}
return filter(source);
}
public Throwable filter(Throwable source) {
- if (shouldFilter) {
- StackTraceElement[] trace = source.getStackTrace();
- List<StackTraceElement> newTrace = filterTraceWithCutOff(trace,
cutOffPackage);
+ if (!shouldFilter) {
+ return source;
+ }
+ StackTraceElement[] trace = source.getStackTrace();
+ List<StackTraceElement> newTrace = filterTraceWithCutOff(trace,
cutOffPackage);
- if (newTrace.isEmpty()) {
- // filter with no cut-off so at least there is some trace
- newTrace = filterTraceWithCutOff(trace, null);
- }
+ if (newTrace.isEmpty()) {
+ // filter with no cut-off so at least there is some trace
+ newTrace = filterTraceWithCutOff(trace, null);
+ }
- // Only trim the trace if there was some application trace on the
stack
- // if not we will just skip sanitizing and leave it as is
- if (!newTrace.isEmpty()) {
- // We don't want to lose anything, so log it
- STACK_LOG.error(FULL_STACK_TRACE_MESSAGE, source);
Review Comment:
Consulted AI and it thinks it is a really bad idea:
can you just introduce another config value and default it to false?
It would work, but the mess comes back for opt-in users. Let me trace it.
## Setup
- Keep `logFullStackTrace` where it is (resolver emits once per request
exception when on).
- Add `logFullStackTraceOnFilter` (default `false`). When `true`,
`filter(Throwable)` emits to `STACK_LOG` as it did pre-PR.
## Output matrix for a 3-cause request exception
| logFullStackTrace | logFullStackTraceOnFilter | STACK_LOG entries |
Resolver entries | Total |
|---|---|---|---|---|
| `false` (default) | `false` (default) | 0 | 1 | 1 |
| `true` | `false` | 1 | 1 | 2 |
| `false` | `true` | 3 (one per cause via filter loop) | 1 | 4 |
| `true` | `true` | 4 (1 from resolver + 3 from filter loop) | 1 | 5 |
The bottom two rows are exactly the per-cause duplication the PR fixes.
The user who opts into `logFullStackTraceOnFilter` for
non-resolver coverage pays for it with 3+ `STACK_LOG` entries every time a
request exception happens.
## Non-resolver sanitize path
| logFullStackTraceOnFilter | STACK_LOG entries from one
`sanitizeRootCause(ex)` call |
|---|---|
| `false` | 0 |
| `true` | 1 |
This is the only clean win. But it only kicks in when the flag is on, at
which point the user is also paying the 4-entry penalty on
every request exception.
## Plus the layering problem
`DefaultStackTraceFilterer` lives in `grails-bootstrap`, which doesn't
depend on `grails-core` where `Config` lives. Options:
1. **Pull `Config` into bootstrap** — module cycle.
2. **Setter on the filterer** — resolver reads the flag and calls
`filterer.setLogFullStackTraceOnFilter(flag)` at construction.
Every other caller (`new DefaultStackTraceFilterer()` in
`TestStacktraceSanitizer`, user subclasses, plugin code) defaults to
`false` because nobody wires the flag for them.
3. **System property**
(`Boolean.getBoolean("grails.full.stacktrace.onFilter")`) — matches the
existing
`SYS_PROP_DISPLAY_FULL_STACKTRACE` pattern in this file. Avoids the module
coupling. Costs YAML-config discoverability.
Option 3 is the cleanest plumbing-wise but means users opt in via JVM flag
rather than `application.yml`. A bit inconsistent with
the other `grails.exceptionresolver.*` settings, all of which live in
YAML.
## Cleaner alternative
Document one line in the upgrade guide for teams relying on non-resolver
sanitize coverage:
```java
// before: relied on side-effect log
Throwable cleaned = GrailsUtil.sanitizeRootCause(ex);
log.error("job failed", cleaned);
// after: explicit
Throwable cleaned = GrailsUtil.sanitizeRootCause(ex);
DefaultStackTraceFilterer.STACK_LOG.error(StackTraceFilterer.FULL_STACK_TRACE_MESSAGE,
cleaned);
log.error("job failed", cleaned);
```
No new flag. No layering workaround. No duplicate-per-cause regression for
users who happen to need non-resolver coverage.
My call
If you want a concrete non-resolver path you want covered, better the
two-line migration. If the project wants blanket pre-PR compatibility, the
second flag works but I'd go the system-property route (option 3) to avoid the
layering mess, and I'd document clearly that turning it on without also
turning logFullStackTrace off produces N+1 entries per exception.
Honestly though: the second flag is solving the problem by re-exposing the
old mechanism. The cleaner fix is the two-line code change at the actual call
sites that want the behavior. That's explicit, discoverable at the code site,
and doesn't involve tuning two interacting flags.
--
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]
