New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes issue #768 to conditionally include stacktraces for DataFormatExceptions in OperationOutcomes when using AbstractJaxRsProviders #769

Merged
merged 2 commits into from Nov 23, 2017

Conversation

Projects
None yet
2 participants
@InfiniteLoop90
Contributor

InfiniteLoop90 commented Oct 22, 2017

When DataFormatExceptions are thrown when using AbstractJaxRsProviders, the stacktrace will be included in the OperationOutcome details only if AbstractJaxRsProvider#withStrackTrace() is configured to return true.

InfiniteLoop90 added some commits Oct 22, 2017

Fixes issue #768 so that when DataFormatExceptions are thrown when us…
…ing AbstractJaxRsProviders, the stacktrace will be included in the OperationOutcome details only if AbstractJaxRsProvider#withStrackTrace() is configured to return true
Fixed issue #768 in a different way by wrapping the DataFormatExcepti…
…on as an InvalidRequestException so that it still gets sent as a 400 response but now DataFormatException stacktraces can now be conditionally sent based on the ExceptionHandlingInterceptor#setReturnStackTracesForExceptionTypes(Class<?>...) configuration
@InfiniteLoop90

This comment has been minimized.

Show comment
Hide comment
@InfiniteLoop90

InfiniteLoop90 Oct 23, 2017

Contributor

@jamesagnew In my second commit I took a different approach to solving the problem. Let me know if you have any concerns with it or any preferred recommendations on a better way to handle the problem. Thanks!

Contributor

InfiniteLoop90 commented Oct 23, 2017

@jamesagnew In my second commit I took a different approach to solving the problem. Let me know if you have any concerns with it or any preferred recommendations on a better way to handle the problem. Thanks!

@InfiniteLoop90 InfiniteLoop90 changed the title from Fixes issue #768 to conditionally include stacktraces in OperationOutcome when using AbstractJaxRsProviders to Fixes issue #768 to conditionally include stacktraces for DataFormatExceptions in OperationOutcome when using AbstractJaxRsProviders Oct 23, 2017

@InfiniteLoop90 InfiniteLoop90 changed the title from Fixes issue #768 to conditionally include stacktraces for DataFormatExceptions in OperationOutcome when using AbstractJaxRsProviders to Fixes issue #768 to conditionally include stacktraces for DataFormatExceptions in OperationOutcomes when using AbstractJaxRsProviders Oct 23, 2017

@InfiniteLoop90

This comment has been minimized.

Show comment
Hide comment
@InfiniteLoop90

InfiniteLoop90 Oct 30, 2017

Contributor

@jamesagnew This is ready for review. Thanks!

Contributor

InfiniteLoop90 commented Oct 30, 2017

@jamesagnew This is ready for review. Thanks!

if (!(theException instanceof BaseServerResponseException)) {
if (theException instanceof DataFormatException) {
// Wrapping the DataFormatException as an InvalidRequestException so that it gets sent back to the client as a 400 response.
retVal = new InvalidRequestException(theException);

This comment has been minimized.

@jamesagnew

jamesagnew Nov 23, 2017

Owner

I'm trying to decide if this makes sense.. It seems a bit odd to me, but then again I can't think of any valid reasons for a server to be throwing DataFormatException that wouldn't be caused by client error. And I especially don't see any reason that this would be worse than the current 500 error. So I guess it is better. :)

@jamesagnew

jamesagnew Nov 23, 2017

Owner

I'm trying to decide if this makes sense.. It seems a bit odd to me, but then again I can't think of any valid reasons for a server to be throwing DataFormatException that wouldn't be caused by client error. And I especially don't see any reason that this would be worse than the current 500 error. So I guess it is better. :)

jamesagnew added a commit that referenced this pull request Nov 23, 2017

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Nov 23, 2017

Owner

Looks great, merging now!

Owner

jamesagnew commented Nov 23, 2017

Looks great, merging now!

@jamesagnew jamesagnew merged commit 6293bb1 into jamesagnew:master Nov 23, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment