1677 return stack trace for authenticated users with specific roles#1683
1677 return stack trace for authenticated users with specific roles#1683
Conversation
… behavior and keeping it consistent
|
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 Vs String |
MikeNeilson
left a comment
There was a problem hiding this comment.
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.
|
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. |
| Stack-trace exposure is controlled through the Togglz feature flag | ||
| `INCLUDE_ERROR_STACK_TRACES`. | ||
|
|
||
| For shared environments, enable that feature in the active `features.properties` and the API will |
There was a problem hiding this comment.
Wouldn't this be an environment variable? not editing the features.properties which just declares the defaults?
There was a problem hiding this comment.
I wondered the same but to my knowledge we do not have the vars available yet?
@MikeNeilson suggested we use togglz
Attempted it here
b359370#diff-17e15a1833942c6fa1e1473e638b835b8b3b98c9a5534d2e591a047c3979e343R76
But has been removed. Unless togglz could somehow target the env and that's what Mike meant?
There was a problem hiding this comment.
The idea behind putting all of the "features" behind togglz is that it gives the primary code a single place to ask "should/can I do this" will punting off the actual acquisition of that option to something else.
For example this is an easy one to put into at at_properties, or something.
There was a problem hiding this comment.
the features.properties file is just a default location.
MikeNeilson
left a comment
There was a problem hiding this comment.
Looks good. However, there is one challenge you need to do.
An integration test that actually renders a stacktrace. NOTE: you may need to be a bit contrived here, except that reality.
| Set.of(new Role(ExceptionTraceSupport.SHOW_STACK_TRACE_ROLE_NAME))); | ||
|
|
||
| Map<String, Serializable> details = ExceptionTraceSupport.buildDetails(Map.of(), | ||
| new IllegalStateException("boom"), |
There was a problem hiding this comment.
"... wouldn't it be if it was: boom, boom, and then kaboom?"
There was a problem hiding this comment.
let me hear you say wayo
| Stack-trace exposure is controlled through the Togglz feature flag | ||
| `INCLUDE_ERROR_STACK_TRACES`. | ||
|
|
||
| For shared environments, enable that feature in the active `features.properties` and the API will |
There was a problem hiding this comment.
The idea behind putting all of the "features" behind togglz is that it gives the primary code a single place to ask "should/can I do this" will punting off the actual acquisition of that option to something else.
For example this is an easy one to put into at at_properties, or something.
| Stack-trace exposure is controlled through the Togglz feature flag | ||
| `INCLUDE_ERROR_STACK_TRACES`. | ||
|
|
||
| For shared environments, enable that feature in the active `features.properties` and the API will |
There was a problem hiding this comment.
the features.properties file is just a default location.
Add conditional stack traces for local and development debugging and keeps the existing
incidentIdentifierbehavior used for log discovery.Stack traces are now included in
CdaError.details.stackTracewhen the request isexplicitly configured for trace output:
OR
CWMS User AdminsAND indevenvironment (env.lower().includes('dev'))The implementation keeps
CdaErroras a response DTO and places the request-aware policyin
ErrorTraceSupport.To make that behavior apply consistently, this PR also removes several redundant
controller-level
catch (Exception)wrappers that were bypassingApiServlet, andupdates remaining endpoint
500handlers to make their responses throughErrorTraceSupport.Partly fixes some of
but not the ConnectionPreparingDataSource
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.