search

spring-projects / spring-vault

Provides familiar Spring abstractions for HashiCorp Vault
https://spring.io/projects/spring-vault
Apache License 2.0
283 stars 186 forks source link

VaultException thrown without "cause" hides important information #713

Closed CharlieReitzel closed 1 year ago

CharlieReitzel commented 2 years ago

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 commented 2 years ago

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

CharlieReitzel commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

vm13814 commented 1 year ago

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

mp911de commented 1 year ago

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