OSIS-166: preserve Vault server faults (5xx) instead of masking them as 400#169
OSIS-166: preserve Vault server faults (5xx) instead of masking them as 400#169anurag4DSB wants to merge 1 commit into
Conversation
|
LGTM — the 5xx passthrough logic is correct. The toResponseException helper properly checks for VaultServiceException with a 5xx status before falling back to 400, and the inline variant in updateTenant correctly preserves errorCode/reason for 4xx while still surfacing genuine server faults. Tests pin both create and update paths. |
| }); | ||
|
|
||
| assertEquals(500, exception.getStatus().value(), | ||
| "a genuine Vault server fault must not be downgraded to 400"); |
There was a problem hiding this comment.
nitpick: I prefer when the tests simply state what they check, rather than focus on the regression that it's fixing, as the user may not have the context to understand the reason of "must not be downgraded to 400" (unless the test is tailored to a particular regression which is not the case here as the test has value more generally, outside of the bug it's fixing), I'd rather say something like "a genuine Vault server fault must be thrown as its original 500 error code".
Just a little suggestion, but up to you!
| // A client-side Vault rejection maps to 400; a genuine Vault server fault (5xx) is | ||
| // surfaced unchanged instead of being masked as a client 400. |
There was a problem hiding this comment.
For the same reason I commented in https://github.com/scality/osis/pull/169/changes#r3482873683, I think it's best not to talk about past regressions in fixed code to avoid confusing the reader. May not warrant a comment at all actually, I think the code is self-explanatory. Or simply state the list of possible error codes and their mapping if it helps understanding.
Same comment for the other occurrences of a similar comment.
3 participants
Intent: why does this change exist?
Several create and update methods caught any failure and rethrew it as 400 BAD_REQUEST, which masked a genuine Vault server fault (5xx) as a client error and hid real outages behind a misleading status.
System impact: what's affected, including downstream?
The error status surfaced by createTenant, createUser, createS3Credential, updateTenant, updateCredentialStatus, and updateUser. VaultServiceException extends Spring's ResponseStatusException, so the carried status is what the caller sees.
Preserved behavior: what explicitly stays the same?
Client-side rejections still map to 400 with the original kept as the cause (updateTenant keeps its errorCode and reason), and all existing error logging is unchanged. Only genuine 5xx faults change.
Intended change: what's different after this PR?
A VaultServiceException carrying a 5xx status is rethrown unchanged so the real server status reaches the caller, instead of being downgraded to 400. A small toResponseException helper centralizes the rule; updateTenant gets an inline 5xx passthrough to keep its errorCode and reason contract. Tests pin that a 500 from Vault stays 500 on create and update.
Verification: how do we know this worked, or how would we know if it didn't?
Ran the full osis-core test module including the JaCoCo gate: green. The new tests fail if a Vault 5xx is downgraded to 400.