Move verb+body validation into an isolated Channel #292
Conversation
Fix: channel.execute must not throw, a future should be returned. Follow-up: implement an outer channel to catch RuntimeExceptions and Errors thrown and convert them into immediate failed futures.
|
WIP: Needs its own tests, AbstractChannelTest.*failsWhenBodyIsGiven should be removed as we don't need to implement this validation in every client. |
|
ACK on the future-vs-throw change. |
I started hacking on channel implementations for a couple other http clients for a direct comparison. This makes it easier to write a new client by reducing the required code duplication. |
|
#289 has a similar motivation since not all clients support content compression by default. |
| @@ -84,7 +84,6 @@ public static HttpChannel of(HttpClient client, URL baseUrl, Duration requestTim | |||
| // Fill request body and set HTTP method | |||
| switch (endpoint.httpMethod()) { | |||
| case GET: | |||
| Preconditions.checkArgument(!request.body().isPresent(), "GET endpoints must not have a request body"); | |||
There was a problem hiding this comment.
+1 to rfink's suggestion (co-locating the GET related validation with the actual mention of GET in this switch seems nice), could we just pop a line in here saying:
if (request.body().isPresent()) {
return Futures.immediateFailedFuture(
new SafeIllegalArgumentException("GET endpoint must not have a request body"));
}I know java's control flow with async stuff is a bit crap (would love rust's ? operator here), but maybe the verbosity is fine?
There was a problem hiding this comment.
This means every client must re-implement the same validation, I’d like to generalize as much as possible to avoid similar issues to our okhttp mess. happy to leave both in place here, but I think the generalized handler makes it easier to build additional client implementations with equivalent semantics.
There was a problem hiding this comment.
To rephrase; I don’t think this validation is a concern of client implementations, but a general dialogue concept.
There was a problem hiding this comment.
As an alternative, could we consider stronger types that avoid optional fields where they don't make sense semantically?
There was a problem hiding this comment.
as long as we don't have to include derive4j ;)
There was a problem hiding this comment.
Always in favor of moving validation to the type system :-)
That would be a larger break than the proposed request change, is that something we’re amenable to?
There was a problem hiding this comment.
I think it's now or never.
There was a problem hiding this comment.
Ack, before we change this I'd like to put some thought into the request body API. I put together quick netty client implementation (very, very naive at this point), but it exposes some api limitations that are worth considering before we lock things down.
|
Closing, DELETE validation is unnecessary because we can send bodies. |
Fix: channel.execute must not throw, a future should be returned.
Follow-up: implement an outer channel to catch RuntimeExceptions
and Errors thrown and convert them into immediate failed futures.