spring-vault icon indicating copy to clipboard operation
spring-vault copied to clipboard

VaultException thrown without "cause" hides important information

Open CharlieReitzel opened this issue 2 years ago • 5 comments

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".

CharlieReitzel avatar Jul 13 '22 18:07 CharlieReitzel

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

mp911de avatar Jul 14 '22 06:07 mp911de

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 avatar Jul 15 '22 18:07 CharlieReitzel

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 avatar Jul 15 '22 19:07 CharlieReitzel

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.

CharlieReitzel avatar Jul 15 '22 20:07 CharlieReitzel

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 avatar Jul 18 '22 13:07 mp911de

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

vm13814 avatar Sep 22 '23 09:09 vm13814

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

mp911de avatar Sep 22 '23 13:09 mp911de