JCL-460: Initial support for RFC9457 Problem Details #1138
Conversation
0375861 to
c9e8814
Compare
| */ | ||
| public BadRequestException( | ||
| final String message, | ||
| final ProblemDetails pd, |
There was a problem hiding this comment.
All of these exceptions that represent HTTP error statuses now have problem details.
But there is also an exception dedicated to carrying problem details introduced above.
Would it make sense to derive these HTTP error exceptions from the new ClientHttpException?
There was a problem hiding this comment.
I missed the fact that they do in fact already derive.
| import java.util.Optional; | ||
|
|
||
| enum HttpStatus { | ||
| BAD_REQUEST(BadRequestException.STATUS_CODE, "Bad Request"), |
There was a problem hiding this comment.
Perhaps it would make sense to flip this on its head:
That this enum has the actual status numbers and the exceptions refer to it.
Instead of the exception classes possessing the actual values.
There was a problem hiding this comment.
Agreed, that's what I wanted to do initially, but then the SolidClientException::handle switch statement complains that a union member variable (e.g. HttpStatus.BAD_REQUEST.code) is not a constant. I preferred to keep that switch statement rather than a large if for readability, but I don't have a strong opinion one way or another.
There was a problem hiding this comment.
Actually I'm now revisiting this and I'm going to do as you suggest because I'm refactoring this to the api module so that it can be used in the jena and rdf4j packages too
| public ClientHttpException(ProblemDetails problemDetails, String message) { | ||
| super(message); | ||
| this.problemDetails = problemDetails; | ||
| } |
There was a problem hiding this comment.
I prepended the additional parameter here to keep the message, cause sequence in the overloaded constructor, but I notice none of the SolidClientExceptions expose a constructor taking a cause. Should the overloaded constructor be removed, and the order of parameters reversed?
| import java.util.Arrays; | ||
| import java.util.Optional; | ||
|
|
||
| enum HttpStatus { |
There was a problem hiding this comment.
Should a package-private enum have a Javadoc entry?
There was a problem hiding this comment.
In my opinion, no.
Except when you want to communicate something important to ourselves.
| import java.util.Optional; | ||
|
|
||
| enum HttpStatus { | ||
| BAD_REQUEST(BadRequestException.STATUS_CODE, "Bad Request"), |
There was a problem hiding this comment.
Agreed, that's what I wanted to do initially, but then the SolidClientException::handle switch statement complains that a union member variable (e.g. HttpStatus.BAD_REQUEST.code) is not a constant. I preferred to keep that switch statement rather than a large if for readability, but I don't have a strong opinion one way or another.
d7df055 to
ad3551c
Compare
The exception are now built taking a ProblemDetails in.
b67d8e1 to
208424d
Compare
The tests are in the solid-client module, because it will need the additional json service anyways in the test dependencies for testing the rest of this feature, and putting these tests in the api module creates a circular dependency.
This adds boilerplate for the Problem Details consumption. This PR intentionally doesn't update
SolidClient, and rather gets the underlying infrastructure in place first, so that the more meaningful changes to the behavior are focused and not drowned in setup code.Specifically, this creates the
ProblemDetailsclass, and updates the exception class hierarchy so that theSolidClientExceptionchild classes take aProblemDetailsinstance in their constructor. TheHttpStatusenum will be used to provide a default title to the ProblemDetails derived from a non-RFC9457 compliant response.