Copilot commented on code in PR #15564:
URL: https://github.com/apache/grails-core/pull/15564#discussion_r3052627642
##########
grails-bootstrap/src/main/groovy/org/grails/exceptions/reporting/DefaultStackTraceFilterer.java:
##########
@@ -90,27 +90,38 @@ public Throwable filter(Throwable source, boolean
recursive) {
public Throwable filter(Throwable source) {
if (shouldFilter) {
- 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);
- }
-
- // 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
+ boolean modified = doFilter(source);
+ if (modified) {
+ // Log the full stack trace once for the top-level exception
(includes causes)
STACK_LOG.error(FULL_STACK_TRACE_MESSAGE, source);
- StackTraceElement[] clean = new
StackTraceElement[newTrace.size()];
- newTrace.toArray(clean);
- source.setStackTrace(clean);
}
Review Comment:
`filter(Throwable source)` currently calls `doFilter(source)` (which mutates
`source` via `setStackTrace`) before logging `FULL_STACK_TRACE_MESSAGE`. That
means the "Full Stack Trace" log is no longer the full/original trace, but the
already-filtered trace. To preserve prior behavior and the PR description, log
the exception before applying the filtered stack trace (e.g., have `doFilter`
compute/return the cleaned trace without mutating, then apply after logging, or
temporarily save/restore the original trace around the log call).
##########
grails-bootstrap/src/main/groovy/org/grails/exceptions/reporting/DefaultStackTraceFilterer.java:
##########
@@ -90,27 +90,38 @@ public Throwable filter(Throwable source, boolean
recursive) {
public Throwable filter(Throwable source) {
if (shouldFilter) {
- 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);
- }
-
- // 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
+ boolean modified = doFilter(source);
+ if (modified) {
+ // Log the full stack trace once for the top-level exception
(includes causes)
STACK_LOG.error(FULL_STACK_TRACE_MESSAGE, source);
- StackTraceElement[] clean = new
StackTraceElement[newTrace.size()];
- newTrace.toArray(clean);
- source.setStackTrace(clean);
}
Review Comment:
This change modifies when/how often the `StackTrace` logger logs
(`STACK_LOG.error(...)`) during recursive filtering, but existing specs (e.g.,
`grails-core/src/test/.../StackTraceFiltererSpec.groovy`) only assert the
sanitized stack trace contents and don't cover log emission/count. Adding a
test that captures the `StackTrace` logger output and asserts a single "Full
Stack Trace" entry for `filter(e, true)` would help prevent regressions.
--
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]
