Support protected resources with RDF body handlers - #1248
acoburn wants to merge 3 commits into
Conversation
Sorry, something went wrong.
| return builder.build(); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
Could you reduce suppression from method scope to assignment scope.
Sorry, something went wrong.
| if (ex.getStatusCode() == UNAUTHORIZED) { | ||
| return new NoBodyResponse<T>(ex.getUri(), ex.getStatusCode(), ex.getHeaders()); | ||
| } |
There was a problem hiding this comment.
Duplicate of above. Might be worth extracting for readability.
Sorry, something went wrong.
|
|
||
| @SuppressWarnings("unchecked") | ||
| static <T, R extends Throwable> Response<T> recover(final Response<T> res, final Throwable err) throws R { | ||
| if (err != null) { |
There was a problem hiding this comment.
Inverting this conditional would reduce indentation of whole method.
Sorry, something went wrong.
| .map(token -> httpClient.send(upgradeRequest(request, token), responseBodyHandler)) | ||
| // Otherwise perform the regular HTTP authorization dance | ||
| .orElseGet(() -> httpClient.send(request, responseBodyHandler) | ||
| .handle((res, err) -> recover(res, err)) |
There was a problem hiding this comment.
Could recover be a method reference or are they not available in 8?
Sorry, something went wrong.
There was a problem hiding this comment.
I also think an inline comment here would help, just like the explanatory notes above.
Sorry, something went wrong.
| if (ex.getStatusCode() == UNAUTHORIZED) { | ||
| return new NoBodyResponse<T>(ex.getUri(), ex.getStatusCode(), ex.getHeaders()); | ||
| } | ||
| } else if (err.getCause() instanceof ClientHttpException) { |
There was a problem hiding this comment.
The error and its cause are inpected, but the hierarchy is not traversed further.
I'm sure this is accurate vis a vis our architecture, but it feels off.
I think an explanation with inline comments would be good.
Sorry, something went wrong.
| static <T, R extends Throwable> Response<T> recover(final Response<T> res, final Throwable err) throws R { | ||
| if (err != null) { | ||
| if (err instanceof ClientHttpException) { | ||
| final ClientHttpException ex = (ClientHttpException) err; | ||
| if (ex.getStatusCode() == UNAUTHORIZED) { | ||
| return new NoBodyResponse<T>(ex.getUri(), ex.getStatusCode(), ex.getHeaders()); | ||
| } | ||
| } else if (err.getCause() instanceof ClientHttpException) { | ||
| final ClientHttpException ex = (ClientHttpException) err.getCause(); | ||
| if (ex.getStatusCode() == UNAUTHORIZED) { | ||
| return new NoBodyResponse<T>(ex.getUri(), ex.getStatusCode(), ex.getHeaders()); | ||
| } | ||
| } | ||
| throw (R) err; | ||
| } | ||
| return res; | ||
| } |
There was a problem hiding this comment.
This new functionality is quite complex and branching.
Would it not make sense to add tests that highlight its behavior specifically?
Sorry, something went wrong.
|
An alternative approach is implemented in #1253 |
Sorry, something went wrong.
|
This is not directly related to this PR in particular, but should we update the integration tests so that this would have been caught before getting merged? |
Sorry, something went wrong.
The issue is that there were new methods introduced. Those new methods were not part of the integration tests, and it was only when I changed the old methods in those integration tests to the new methods that I discovered the implementation problem. |
Sorry, something went wrong.
With the current code, it is not possible to use the ofJenaModel (and similar) method on protected resources, because the code will throw an exception at the first 401, interfering with the reactive style of the JCL