Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move verb+body validation into an isolated Channel #292

Closed
wants to merge 1 commit into from
Closed

Move verb+body validation into an isolated Channel #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractChannelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import java.io.ByteArrayInputStream;
Expand Down Expand Up @@ -204,9 +205,9 @@ public void encodesQueryParameters() throws Exception {
public void get_failsWhenBodyIsGiven() {
endpoint.method = HttpMethod.GET;
when(request.body()).thenReturn(Optional.of(body));
assertThatThrownBy(() -> channel.execute(endpoint, request))
.isInstanceOf(SafeIllegalArgumentException.class)
.hasMessage("GET endpoints must not have a request body");
assertThatThrownBy(() -> Futures.getDone(channel.execute(endpoint, request)))
.hasCauseInstanceOf(SafeIllegalArgumentException.class)
.hasMessageContaining("must not have a request body");
}

@Test
Expand All @@ -227,9 +228,9 @@ public void put_okWhenNoBodyIsGiven() {
public void delete_failsWhenBodyIsGiven() {
endpoint.method = HttpMethod.DELETE;
when(request.body()).thenReturn(Optional.of(body));
assertThatThrownBy(() -> channel.execute(endpoint, request))
.isInstanceOf(SafeIllegalArgumentException.class)
.hasMessage("DELETE endpoints must not have a request body");
assertThatThrownBy(() -> Futures.getDone(channel.execute(endpoint, request)))
.hasCauseInstanceOf(SafeIllegalArgumentException.class)
.hasMessageContaining("must not have a request body");
}

@Test
Expand Down
1 change: 1 addition & 0 deletions dialogue-core/src/main/java/com/palantir/dialogue/core/Channels.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public static Channel create(
// Instrument inner-most channel with metrics so that we measure only the over-the-wire-time
.map(channel -> new InstrumentedChannel(channel, metrics))
.map(channel -> new DeprecationWarningChannel(channel, metrics))
.map(MethodRequestBodyValidatingChannel::new)
.map(channel -> new TracedChannel(channel, "Concurrency-Limited Dialogue Request"))
.map(TracedRequestChannel::new)
.map(ConcurrencyLimitedChannel::create)
Expand Down
31 changes: 31 additions & 0 deletions ...gue-core/src/main/java/com/palantir/dialogue/core/MethodRequestBodyValidatingChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.palantir.dialogue.core;

import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.palantir.dialogue.Channel;
import com.palantir.dialogue.Endpoint;
import com.palantir.dialogue.HttpMethod;
import com.palantir.dialogue.Request;
import com.palantir.dialogue.Response;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;

/** Validates that <code>GET</code> and <code>DELETE</code> requests do not contain bodies. */
final class MethodRequestBodyValidatingChannel implements Channel {

private final Channel delegate;

MethodRequestBodyValidatingChannel(Channel delegate) {
this.delegate = delegate;
}

@Override
public ListenableFuture<Response> execute(Endpoint endpoint, Request request) {
HttpMethod method = endpoint.httpMethod();
if ((method == HttpMethod.DELETE || method == HttpMethod.GET)
&& request.body().isPresent()) {
return Futures.immediateFailedFuture(
new SafeIllegalArgumentException("GET and DELETE endpoints must not have a request body"));
}
return delegate.execute(endpoint, request);
}
}
3 changes: 0 additions & 3 deletions dialogue-java-client/src/main/java/com/palantir/dialogue/HttpChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ public ListenableFuture<Response> execute(Endpoint endpoint, Request request) {
// 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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To rephrase; I don’t think this validation is a concern of client implementations, but a general dialogue concept.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative, could we consider stronger types that avoid optional fields where they don't make sense semantically?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as we don't have to include derive4j ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's now or never.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

httpRequest.GET();
break;
case POST:
Expand All @@ -94,8 +93,6 @@ public ListenableFuture<Response> execute(Endpoint endpoint, Request request) {
httpRequest.PUT(toBody(request));
break;
case DELETE:
Preconditions.checkArgument(
!request.body().isPresent(), "DELETE endpoints must not have a request body");
httpRequest.DELETE();
break;
}
Expand Down