Propagate error info to caller#1174
Conversation
| RequestUrls requestUrls; | ||
| try { | ||
| requestUrls = buildRequestedUrls(requestToProxy); | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
this catching Throwable doesn't give us much info on what could be happening.
Provide InvalidTokenException with some error codes (probably we should do something like this so we can have more info on our end on what can be happening in order to help the customer)
| // InvalidTokenException extends RuntimeException | ||
| if (e instanceof ReversibleTokenizationStrategy.InvalidTokenException ite) { | ||
| return HttpEventResponse.builder() | ||
| // should this be a 500? Maybe 422 Unprocessable Content? |
There was a problem hiding this comment.
500 will make us retry, and same call would yield same results.
I think 422 makes more sense in this case and we can handle globally on Worklytics' side
| super(message, cause); | ||
|
|
||
| @Getter | ||
| public enum ErrorCode { |
There was a problem hiding this comment.
maybe not the best approach, but for certain cases (rules exceptions, this, and access errors) could be helpful to exactly know what's going on rather than a generic error
There was a problem hiding this comment.
Pull request overview
This PR adds structured error details to reversible-token decryption failures so callers can distinguish specific FAILED_TO_BUILD_URL / token reversal failure modes.
Changes:
- Introduces
InvalidTokenException.ErrorCodeonReversibleTokenizationStrategy.InvalidTokenExceptionand updates AES token reversal to throw typed error codes. - Updates
ApiDataRequestHandlerto surface token reversal error information to callers when request URL building fails.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
java/gateway-core/src/main/java/com/avaulta/gateway/tokens/impl/AESReversibleTokenizationStrategy.java |
Switches decryption exception handling to throw InvalidTokenException with structured ErrorCode values. |
java/gateway-core/src/main/java/com/avaulta/gateway/tokens/ReversibleTokenizationStrategy.java |
Adds ErrorCode enum + stores it on InvalidTokenException so callers can inspect a stable identifier. |
java/core/src/main/java/co/worklytics/psoxy/gateway/impl/ApiDataRequestHandler.java |
Returns additional error detail when URL building fails due to token reversal errors, and attempts to propagate the error code in responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // should this be a 500? Maybe 422 Unprocessable Content? | ||
| .statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR) | ||
| .header(ProcessedDataMetadataFields.ERROR.getHttpHeader(), | ||
| ErrorCauses.FAILED_TO_BUILD_URL.name()) |
There was a problem hiding this comment.
This InvalidTokenException branch returns a 500 with FAILED_TO_BUILD_URL, but the same exception type is handled later as a client/token issue (TOKENIZED_REQUEST_PARAMETER_INVALID with a 409). That inconsistency will make it harder for callers to correctly react to token problems; consider mapping this branch to the same status + error cause as the later catch (or otherwise keep the semantics consistent across both paths).
| // should this be a 500? Maybe 422 Unprocessable Content? | |
| .statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR) | |
| .header(ProcessedDataMetadataFields.ERROR.getHttpHeader(), | |
| ErrorCauses.FAILED_TO_BUILD_URL.name()) | |
| .statusCode(HttpStatus.SC_CONFLICT) | |
| .header(ProcessedDataMetadataFields.ERROR.getHttpHeader(), | |
| ErrorCauses.TOKENIZED_REQUEST_PARAMETER_INVALID.name()) |
There was a problem hiding this comment.
yeah, I agree not a 500 ... whatever 4xx seems most semantically correct for the case.
| // InvalidTokenException extends RuntimeException | ||
| if (e instanceof ReversibleTokenizationStrategy.InvalidTokenException ite) { | ||
| return HttpEventResponse.builder() | ||
| // should this be a 500? Maybe 422 Unprocessable Content? | ||
| .statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR) | ||
| .header(ProcessedDataMetadataFields.ERROR.getHttpHeader(), | ||
| ErrorCauses.FAILED_TO_BUILD_URL.name()) | ||
| .body("Error parsing request URL. Error " + ite.getErrorCode().getCode()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
The newly added behavior that surfaces InvalidTokenException details during URL building (and the associated status/header/body mapping) doesn’t appear to be covered by tests. Please add/extend ApiDataRequestHandlerTest to assert the response status and X-Psoxy-Error header (and any new detail header/body) for this failure mode, so future refactors don’t silently change the contract.
eschultink
left a comment
There was a problem hiding this comment.
i think just use X-Psoxy-Error that exists; no need to create something else that basically duplicates that.
and return whole error enum name or exception name. No need to add extra levels of indirection to save a few chars, but require us to grep the code to determine the actual error
| /** | ||
| * a specific error code while processing the request, complements ERROR (optional) | ||
| */ | ||
| ERROR_CODE("Error-Code"), |
There was a problem hiding this comment.
i think Error is a value from an enum ... so is specific
There was a problem hiding this comment.
so if Error is a strict code -- I think eliminate this, just use Error; but if it's freeform message text, OK.
|
|
||
| @Getter | ||
| public enum ErrorCode { | ||
| ALGORITHM_PARAMETER_ERROR("ITE01", "Failed to decrypt token; some algorithm parameter, such as iv, is wrong"), |
There was a problem hiding this comment.
no need to be so clever here; just return whole enum names ... don't care about a few extra chars in error case
…ersibleTokenizationStrategy.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Use X-Psoxy-Error with full InvalidTokenException.ErrorCode enum names, return 422 for invalid token failures, and remove the redundant Error-Code header. Co-authored-by: Cursor <cursoragent@cursor.com>
Try to propagate some info to the caller on the FAILED_TO_BUILD_URL error case.
Change implications
CHANGELOG.mdanything that will show up interraform plan/applythat isn'tobviously a no-op? no
alpha, requires major versionchange -> no