Skip to content
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

VaultException thrown without "cause" hides important information #713

Closed
CharlieReitzel opened this issue Jul 13, 2022 · 7 comments
Closed
Labels
type: bug A general bug
Milestone

Comments

@CharlieReitzel
Copy link

In general, exception handlers should preserve the original exception so eventually, a full stack trace to the original offending code can be logged. This gives developer the ability to quickly zero in on the precise issue and time to resolve problems is dramatically reduced.

This principle is described by a SonarQube issue RSPEC-1166.

I am finding that, in a number of places, the spring-vault project is suppressing the original exception. The original message is usually logged, but the exact location where the exception was thrown is lost.

The fix is to simply include the original exception with VaultException - or derived type - as the "cause".

@mp911de
Copy link
Member

mp911de commented Jul 14, 2022

How about submitting a pull request so we can continue the discussion over actual code changes?

@CharlieReitzel
Copy link
Author

Fair enough. Let me get something together to at least look at.

My issue that triggered this ticket is that Apache httpcomponents is throwing a ClientProtocolException. Tbh, I'm not 100% sure what that means. :--) But, if I could find the line of code where it gets thrown, I'm sure I could figure it out quickly. As it is, I'm going through the Apache sources trying to understand all the places it might come from, etc.

@CharlieReitzel
Copy link
Author

Looks I am just lucky. I found only 3 cases. But I'm encountering 1 of them. Such is life.

Here are links to the lines of code. Does that work?

  1. VaultTokenAdapter
  2. VaultLoginException
  3. AbstractResult

@CharlieReitzel
Copy link
Author

CharlieReitzel commented Jul 15, 2022

I created a local build that adds the local exception variable as "cause" argument to the constructor. Very minimal changes vs. 2.3.2 worked when running locally.

@mp911de
Copy link
Member

mp911de commented Jul 18, 2022

HttpStatusCodeException is (or should be) an error case that should not retain any stack details within the client because the error indication is a HTTP status code that isn't expected. That is, an already correctly parsed response where only the status code doesn't match our expectations. The other two exceptions should be indeed reworked to host the cause.

@mp911de mp911de added the type: bug A general bug label Mar 17, 2023
@mp911de mp911de modified the milestones: 3.0.2, 2.3.3 Mar 17, 2023
mp911de added a commit that referenced this issue Mar 17, 2023
mp911de added a commit that referenced this issue Mar 17, 2023
@vm13814
Copy link

vm13814 commented Sep 22, 2023

There is another case : ReactiveVaultTemplate should work the same as VaultTemplate

@mp911de
Copy link
Member

mp911de commented Sep 22, 2023

How exactly do you intend to extract a cause from a HTTP status code that isn't communicated as exception?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants