feat: improve error response messages #614
Conversation
emlaver
force-pushed
the
s1002-error-augmentation
branch
from
a5e8c90 to
79ba202
Compare
ricellis
force-pushed
the
s1002-error-augmentation
branch
from
79ba202 to
f0cf7fd
Compare
| public Response intercept(Chain chain) throws IOException { | ||
| // Don't modify the request, but get the response | ||
| Response response = chain.proceed(chain.request()); | ||
| if (!"HEAD".equals(chain.request().method()) // skip HEAD requests because they have no body |
There was a problem hiding this comment.
This is redundant with response.body() != null below
There was a problem hiding this comment.
I would keep it if there's an event where the response of a non-HEAD request fails and the body doesn't exist.
There was a problem hiding this comment.
I mean it's other way around - we don't need HEAD check if we already have check for body not been null 😄
There was a problem hiding this comment.
I think @eiri means that we could skip the HEAD check and just use the body check, but if we want to do that we need to be sure on the subtlety any of empty string vs null
There was a problem hiding this comment.
Ok, I'm open to removing HEAD check. If the body is not null and just empty, the GSON deserialize code should returns a null response. Here's the doc for fromJson:
Returns:
an object of type T from the string. Returns null if json is null or if json is empty.
If we don't want it to reach that code then we can add a response.body().toString().isEmpty() check.
Edited: Tried out the change in a branch and seems to work fine. Also added a test (to verify) for GET request with no body and that passes.
| if (errorBody != null) { | ||
| // Don't augment if there is already a trace present | ||
| if (!errorBody.has("trace")) { | ||
| String error = Optional.ofNullable(errorBody.get("error")).map(JsonElement::getAsString).orElse(null); |
There was a problem hiding this comment.
A nit, but logically those two lines fit better after if (!errorBody.has("errors")) { check
Add ErrorTransformInterceptor. Modify CloudantBaseService to apply interceptor. New tests in ErrorTransformInterceptorTest and CloudantErrorInterceptorTest. Co-authored-by: Rich Ellis <[email protected]>
emlaver
force-pushed
the
s1002-error-augmentation
branch
from
8411bd3 to
1369e17
Compare
PR summary
Add interceptor for error response augmentation
Fixes: s1002
Note: An existing issue is required before opening a PR.
PR Checklist
Please make sure that your PR fulfills the following requirements:
Angular Commit Message Guidelines.
PR Type
What is the current behavior?
Cloudant/CouchDB error models are handled by the core with only the
errorfield being processed. Obtaining e.g. thereasonfield requires additional user code.What is the new behavior?
The error response is augmented with
errorsandtraceproperties.This improves the way the core handles the error response and enhances the user facing exception string to include the
reason.The
traceis the CouchDB request ID which can be used to correlate with logs.Does this PR introduce a breaking change?
The existing error model fields are retained, the new fields are backwards compatible.
Exception strings will be different, but the types remain the same.
Other information
Add
ErrorTransformInterceptor.Modify
CloudantBaseServiceto apply interceptor.New tests in
ErrorTransformInterceptorTestandCloudantErrorInterceptorTest.