Skip to content

1677 return stack trace for authenticated users with specific roles#1683

Open
krowvin wants to merge 6 commits intodevelopfrom
1677-return-stack-trace-for-authenticated-users-with-specific-roles
Open

1677 return stack trace for authenticated users with specific roles#1683
krowvin wants to merge 6 commits intodevelopfrom
1677-return-stack-trace-for-authenticated-users-with-specific-roles

Conversation

@krowvin
Copy link
Copy Markdown
Collaborator

@krowvin krowvin commented Apr 9, 2026

Add conditional stack traces for local and development debugging and keeps the existing incidentIdentifier behavior used for log discovery.

Stack traces are now included in CdaError.details.stackTrace when the request is
explicitly configured for trace output:

  • request is on localhost (docker compose)
    OR
  • request is authenticated as a CWMS User Admins AND in dev environment (env.lower().includes('dev'))

The implementation keeps CdaError as a response DTO and places the request-aware policy
in ErrorTraceSupport.

To make that behavior apply consistently, this PR also removes several redundant
controller-level catch (Exception) wrappers that were bypassing ApiServlet, and
updates remaining endpoint 500 handlers to make their responses through
ErrorTraceSupport.

Partly fixes some of

Other potentially related issues include:

Trying to centralize general errors where possible and reduce fragmentation

Not trying to solve a new base exception direction, or a better inheritance model mainly because @MikeNeilson said to discuss more before doing that in another issue.

@krowvin krowvin requested a review from MikeNeilson April 9, 2026 03:16
@krowvin krowvin linked an issue Apr 9, 2026 that may be closed by this pull request
@krowvin
Copy link
Copy Markdown
Collaborator Author

krowvin commented Apr 9, 2026

Something to note is that it returns JSON and a stack trace in JSON is not the prettiest of things with all the \n in the JSON string.

The last commit shows an alternative where we could keep both, or pick one, of the options. Putting the traces in a list split by the newline (Could probably do \n\t too) does render nicely in the browser at least and possible in a fetch/output of a downstream client.

List
image

Vs

String
image

Copy link
Copy Markdown
Contributor

@MikeNeilson MikeNeilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using System.getProperty or System.getenv directly, integrate into the Togglz feature system.

But otherwise I don't hate it. Do want to chew on it some more, but overall seems reasonable.

@krowvin krowvin requested a review from MikeNeilson April 10, 2026 03:32
@krowvin
Copy link
Copy Markdown
Collaborator Author

krowvin commented Apr 10, 2026

Spoke with Mike. Lets go with stackTraceLines and remove the string.

Need to fix the OpenAPI spec. Latest Schema can break if Oracle out of sync (but check). But search for FAILED all caps and first instance all caps FAILED should have your error for the other failed tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Return stack trace for authenticated users with specific role(s)

2 participants