JCL-460: Initial support for RFC9457 Problem Details - #1138
NSeydoux wants to merge 18 commits into
Conversation
Sorry, something went wrong.
| */ | ||
| 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?
Sorry, something went wrong.
There was a problem hiding this comment.
I missed the fact that they do in fact already derive.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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
Sorry, something went wrong.
There was a problem hiding this comment.
Done in d568238
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
There was a problem hiding this comment.
In my opinion, no.
Except when you want to communicate something important to ourselves.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
The exception are now built taking a ProblemDetails in.
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.
Sorry, something went wrong.
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 ProblemDetails class, and updates the exception class hierarchy so that the SolidClientException child classes take a ProblemDetails instance in their constructor. The HttpStatus enum will be used to provide a default title to the ProblemDetails derived from a non-RFC9457 compliant response.