JCL-402: Data classes for HTTP Problem Details support #1160

NSeydoux · 2024-04-14T20:49:49Z

This creates the ProblemDetails class, which represents the content of an RFC9457-compliant HTTP error response. To help setting defaults for ProblemDetails, this also adds HttpStatus to the public API of the api module. Only the HTTP status are part of the API (they will be reused by error-specific exceptions), and the HttpStatus class also defines a package-private way to build a default Problem Details title based on the HTTP status code.

ProblemDetails encapsulates the SPI lookup required to discover a JSON service used to deserialize RFC9457 error responses. If no JSON parser is available, ProblemDetails falls back to providing a minimal object with all its fields nulled except for the type set to a default value and the error code set to the value associated with the response. Unlike what was suggested in the initial design, no default is applied to the title based on the HTTP status phrase, because not all the HTTP client libraries we support expose sur HTTP status phrase.

The tests for ProblemDetails parsing 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.

The Response interface from the api module now also exposes a new BodyHandler that will throw on HTTP error. This body handler is composable with another body handler that handles success cases, and projects the HTTP response to the desirable object. Both the exception mapper and the success criterion are configurable. This new body handler throws a ClientHttpException, which is a new Exception type parent of SolidClientException. The HTTP-specific members of SolidClientException have been lifted up to ClientHttpException.

langsamu · 2024-04-15T10:53:03Z

api/src/main/java/com/inrupt/client/HttpStatus.java

+ static String getStatusMessage(final int statusCode) {
+ return Arrays.stream(StatusMessages.values())
+                .filter(status -> status.code == statusCode)
+                .findFirst()
+                .map(knownStatus -> knownStatus.message)
+                .orElseGet(() -> {
+ // If the status is unknown, default to 400 for client errors and 500 for server errors
+ if (statusCode >= 400 && statusCode <= 499) {
+ return BAD_REQUEST.message;
+                    }
+ return INTERNAL_SERVER_ERROR.message;
+                });
+        }
+    }


I don't like this logic.
While I think I understand why it would seem helpful, I disagree with it.

Other implementations of HTTP status classes that I know of choose either to return nulls or to throw when they encounter unknown statuses.

The default chosen here is very opinionated and I don't think that a library like this one, a fairly low-level HTTP client library, should make such decisions about responses from the server.

I might even argue the it is always the server who is responsible for the content of HTTP status codes and phrases, so our library should just honor the ones generated by the server.

Another example of prior art is the JBoss RESTEasy implementation of JAX-RS (I think), that has a special HTTP status phrase 'Unknown Code' for some cases.

Do we really need to do this status codes mapping at all or could you just pass the data on.

BAD_REQUEST(HttpStatus.BAD_REQUEST, "Bad Request"), UNAUTHORIZED(HttpStatus.UNAUTHORIZED, "Unauthorized"), FORBIDDEN(HttpStatus.FORBIDDEN, "Forbidden"), ...

Unless you do all status codes there are going to be problems like this. Will your HttpStatus be used as API?

I might even argue the it is always the server who is responsible for the content of HTTP status codes and phrases, so our library should just honor the ones generated by the server.

I would have preferred that as well, however not all HTTP libraries expose the HTTP status message, in particular java.net.http.HttpResponse does not, so the com.inrupt.client.ResponseInfo interface implements the smaller common denominator and doesn't expose the status message returned by the server.

From there, I don't think it is possible to expose what the server returned. The current behavior is aligned with https://www.rfc-editor.org/rfc/rfc9110.html#name-status-codes:

HTTP status codes are extensible. A client is not required to understand the meaning of all registered status codes, though such understanding is obviously desirable. However, a client MUST understand the class of any status code, as indicated by the first digit, and treat an unrecognized status code as being equivalent to the x00 status code of that class.

Based on this, I would say it is more correct to default to 400 and 500 for client and server status message respectively rather than returning a unique "Unknown" status message.

langsamu · 2024-04-15T10:56:42Z

api/src/main/java/com/inrupt/client/ProblemDetails.java

+ final URI instance = Optional.ofNullable((String) pdData.get("instance"))
+                    .map(URI::create)
+                    .orElse(null);
+ // Note that the status code is disregarded from the body, and reused from the HTTP response directly,


We could say that the mentioned requirement is for the server.
If the server decides to err, I think we should return what the server said.

With the typed JSON deserialization, we no longer enforce the problem details status is the same as the HTTP response status, except in case of invalid status in the first place.

langsamu · 2024-04-15T10:58:04Z

api/src/main/java/com/inrupt/client/ProblemDetails.java

+ // ProblemDetails doesn't have a default constructor, and we can't use JSON mapping annotations because
+ // the JSON service is an abstraction over JSON-B and Jackson, so we deserialize the JSON object in a Map
+ // and build the ProblemDetails from the Map values.


Would it not be possible to use typed JSON deserialization if this class had a default constructor and conventionally named property methods?

I would prefer that to this Map-based logic.

Let me give this a try

Done in 7a59d45 via adding a package-private mutable data class used to initialize the immutable one.

langsamu · 2024-04-15T10:59:05Z

api/src/test/java/com/inrupt/client/HttpStatusTest.java

+ assertEquals(
+ HttpStatus.StatusMessages.getStatusMessage(418),
+ HttpStatus.StatusMessages.BAD_REQUEST.message
+        );


This test is a perfect example for why I disagree with the current defaulting logic.

That 418 is most certainly not a Bad Request.

api/src/main/java/com/inrupt/client/ProblemDetails.java

api/src/main/java/com/inrupt/client/ClientHttpException.java

NSeydoux · 2024-04-16T13:25:00Z

api/src/main/java/com/inrupt/client/ProblemDetails.java

+ return this.instance;
+    };
+
+ public static class ProblemDetailsData {


Unfortunately JSONB demands things be public. I think my previous tests failed to capture this because of lack of correct recompilation

I lean towards keeping this over the previous Map-based approach: 490a1f3, but it does add some surface to the public API.

I do prefer this approach over the map-based one, even if it adds superfluous API.
It being an inner class is hiding enough for my taste.

I'd call it simply 'Data'.
And I'd annotate it with Javadocs explain intended usage (none/deserialization).
Some @SuppressWarnings might be in order if PMD/Sonar complain about a mutable class.

Initial ClientHttpException class Create HTTP status enum Add some tests Add javadoc Refactor HttpStatus Add specialized builder to ProblemDetails Add missing header Make problem details parsing more resilient Add tests for problem details parsing 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. License header

the JSON was invalid, causing the intended code path not to be used.

The exception are now built taking a ProblemDetails in. Use HttpStatus from API module as reference Lint

This reverts commit 499164d.

That is required by jsonb

NSeydoux · 2024-04-17T07:26:05Z

api/src/main/java/com/inrupt/client/Response.java

+         * @return the body handler
+         * @param <T> the type of the body handler
+         */
+ public static <T> Response.BodyHandler<T> throwOnError(


This body handler can be used in the solid module, where we already have a specific mapping from HTTP errors to exceptions

NSeydoux · 2024-04-17T07:26:44Z

api/src/main/java/com/inrupt/client/Response.java

+         * @return the body handler
+         * @param <T> the type of the body handler
+         */
+ public static <T> Response.BodyHandler<T> throwOnError(


This handler can be used in the jena and rdf4j modules, where we want to throw a generic HTTP exception.

langsamu · 2024-04-17T11:44:18Z

api/src/main/java/com/inrupt/client/ProblemDetails.java

+ return this.instance;
+    };
+
+ public static class ProblemDetailsData {


I do prefer this approach over the map-based one, even if it adds superfluous API.
It being an inner class is hiding enough for my taste.

I'd call it simply 'Data'.
And I'd annotate it with Javadocs explain intended usage (none/deserialization).
Some @SuppressWarnings might be in order if PMD/Sonar complain about a mutable class.

langsamu · 2024-04-17T11:53:13Z

api/src/main/java/com/inrupt/client/ProblemDetails.java

+ public URI instance;
+    }
+
+ private static JsonService getJsonService() {


This (and its two fields) might be simplified using an Optional field.

If the field is nut null then return its value (which might be null).
Otherwise try to get a service and set the field to an Optional.ofNullable with the service as value.

Might be nicer or not, I don't really know.
This looks OK otherwise.

I thought it would be nicer, but after having implemented some of it, it looks quite the same. One of my assumptions which causes the code to look like what it does here is that the SPI lookup is a relatively expensive operation that you only want to do once, so I want to have a clear separation between "this is null because we haven't initialized it" and "this is null because the lookup failed", so that I don't do a systematic lookup in the latter case assuming we might be in the former.

If I have understood what you are describing correctly, having the Optional doesn't change much to the overall structure, in particular because a lookup failure results in an exception, and not in null, so the try/catch needs to be there even when initializing the Optional (I think)

langsamu · 2024-04-17T11:54:49Z

api/src/main/java/com/inrupt/client/ProblemDetails.java

+ status,
+ pdData.instance
+            );
+        } catch (IOException e) {


Any chance we have a more specific exception for JSON parsing errors?

Unfortunately no, more specific exceptions are Jackson or JSON-B specific. Our abstraction layer doesn't define a specific exception beyond IOException (it might be a future improvement though, in the spirit of the ClientHttpException)

api/src/main/java/com/inrupt/client/Response.java

NSeydoux force-pushed the JCL-460/problem-details-class-http-status branch from c4eb65c to 16a6143 Compare April 14, 2024 20:58

NSeydoux changed the title ~~Data classes for HTTP Problem Details support~~ JCL-402: Data classes for HTTP Problem Details support Apr 14, 2024

This was referenced Apr 14, 2024

JCL-460: Initial support for RFC9457 Problem Details #1138

Closed

JCL-402: Low-level client throws enriched exceptions #1157

Merged

NSeydoux marked this pull request as ready for review April 14, 2024 22:07

NSeydoux requested a review from a team as a code owner April 14, 2024 22:07

langsamu reviewed Apr 15, 2024

View reviewed changes

NSeydoux commented Apr 15, 2024

View reviewed changes

api/src/main/java/com/inrupt/client/ProblemDetails.java Outdated Show resolved Hide resolved

edwardsph reviewed Apr 16, 2024

View reviewed changes

api/src/main/java/com/inrupt/client/ClientHttpException.java Show resolved Hide resolved

NSeydoux mentioned this pull request Apr 16, 2024

JCL-402: Refactor HTTP exception constructors #1161

Closed

NSeydoux commented Apr 16, 2024

View reviewed changes

NSeydoux added 19 commits April 16, 2024 22:46

Use typed JSON deserialization

1147b3b

Move JSON service discovery in ProblemDetails²

050f4a6

Make JSON service singleton

374880e

fixup! Move JSON service discovery in ProblemDetails²

914c153

Change default ProblemDetails titles

f753ff2

Fix ProblemDetails parsing tests

67f6124

the JSON was invalid, causing the intended code path not to be used.

Lift SolidClientExceptions members to parent

766eb95

Fix typo in default HTTP status

9cbf5e3

Lint

ba883b7

Refactor specialized exception constructors

17b8ce2

The exception are now built taking a ProblemDetails in. Use HttpStatus from API module as reference Lint

fixup! Change default ProblemDetails titles

6c8c766

Remove default ProblemDetails title

97f39c7

fixup! Remove default ProblemDetails title

662ad83

fixup! fixup! Remove default ProblemDetails title

540fe12

fixup! fixup! fixup! Remove default ProblemDetails title

2452b49

Replace ProblemDetailsData with private setters

80be1ac

Revert "Replace ProblemDetailsData w prv. setters"

ca829b2

This reverts commit 499164d.

Make data class public

1832898

That is required by jsonb

NSeydoux added 3 commits April 16, 2024 22:46

Add body handler to throw on HTTP error

4638882

fixup! Add body handler to throw on HTTP error

6412895

Lint

71d6302

NSeydoux force-pushed the JCL-460/problem-details-class-http-status branch from 8b193a9 to 71d6302 Compare April 16, 2024 20:47

NSeydoux added 4 commits April 16, 2024 22:53

fixup! fixup! Add body handler to throw on HTTP error

d75f6e5

Rephrase comment

d91de68

fixup! fixup! fixup! Remove default ProblemDetails title

600bfdc

Add exception mapper to throwing body handler

5a1bd37

NSeydoux commented Apr 17, 2024

View reviewed changes

NSeydoux mentioned this pull request Apr 17, 2024

JCL-402: HTTP error handling in JenaBodyHandlers #1159

Merged

langsamu reviewed Apr 17, 2024

View reviewed changes

jholleran reviewed Apr 17, 2024

View reviewed changes

api/src/main/java/com/inrupt/client/Response.java Outdated Show resolved Hide resolved

NSeydoux added 2 commits April 17, 2024 15:54

Add Javadoc to inner ProblemDetails.Data class

aff5c70

Remove response URI from exception message

3a5f981

jholleran approved these changes Apr 18, 2024

View reviewed changes

NSeydoux merged commit 121498d into main Apr 18, 2024

NSeydoux deleted the JCL-460/problem-details-class-http-status branch April 18, 2024 11:53

JCL-402: Data classes for HTTP Problem Details support #1160

JCL-402: Data classes for HTTP Problem Details support #1160

Uh oh!

Conversation

NSeydoux commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NSeydoux commented Apr 14, 2024 •

edited

Loading