Uh oh!
There was an error while loading. Please reload this page.
JCL-402: Low-level client throws enriched exceptions - #1157
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bec2793 to
2341769Compareb67d8e1 to
208424dCompareda62735 to
aa53b0eCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
92358fa to
c41758eCompareaa53b0e to
6d28358Compareb13bc8d to
fda4bb3Compare8b193a9 to
71d6302Comparefda4bb3 to
d722196Compare| headers.firstValue(USER_AGENT).ifPresent(agent -> builder.setHeader(USER_AGENT, agent)); | ||
| final Request request = builder.build(); | ||
| return client.send(request, Response.BodyHandlers.ofByteArray()) |
There was a problem hiding this comment.
This looks like a large change, but essentially the error branch of the initial if has been moved to the wrapping throwOnError
| )// FIXME I don't understand why the following is required. | ||
| .thenAccept(o -> { /* no-op */ }); |
There was a problem hiding this comment.
I don't know that this is the correct approach
There was a problem hiding this comment.
It's because the method signature is (async) void.
Previously (when error handling was here) the response from the body handler was mapped (thenApply) to nothing.
NSeydoux
commented
Apr 17, 2024
You'll notice that unlike what we discussed yesterday, I didn't end up using the |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f73c900 to
c03c1d9Compare40a80c1 to
642a8eaCompare| * @param statusCode the HTTP response status code | ||
| * @return true if the status code is in the success range, namely [200, 299]. | ||
| */ | ||
| static boolean isSuccess(final int statusCode) { |
There was a problem hiding this comment.
Do we really need this method?
There was a problem hiding this comment.
It avoids some duplication, and my impression was that it is a quite common pattern to have this affordance on HTTP response object: JS Response has a .ok getter, and OkHttp has isSuccessful. I'm not strongly opposed to removing this, but I find it reads slightly better than checking the status range each time.
Uh oh!
There was an error while loading. Please reload this page.
`SolidClient` now parses HTTP error responses if they are compliant with RFC9457 in order to throw a structured exception. In case when no JSON parser is available in the classpath, the default behavior is to build a ProblemDetails object that only relies on the data available in the HTTP response status code.
It will be used in Jena and RDF4J modules too
HTTP body handlers are applied by the low-level HTTP client, so they should only ever map to an HTTP response. The processing of that response is then the responsibility of the caller.
a03f434 to
9e0967dCompare
This is based on #1160, which should be reviewed first.
The
SolidClientnow uses thethrowOnErrorbody mapper to handle error responses, and provides a custom exception mapper to throw the appropriate specialized exception.