From 32fb20c4b82461c457bf960371298a29d3492e27 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 5 Feb 2020 20:00:45 -0500 Subject: [PATCH] Move verb+body validation into an isolated Channel 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. --- .../dialogue/AbstractChannelTest.java | 13 ++++---- .../com/palantir/dialogue/core/Channels.java | 1 + .../MethodRequestBodyValidatingChannel.java | 31 +++++++++++++++++++ .../com/palantir/dialogue/HttpChannel.java | 3 -- 4 files changed, 39 insertions(+), 9 deletions(-) create mode 100644 dialogue-core/src/main/java/com/palantir/dialogue/core/MethodRequestBodyValidatingChannel.java diff --git a/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractChannelTest.java b/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractChannelTest.java index 89d3b8364..bf0e7881d 100644 --- a/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractChannelTest.java +++ b/dialogue-client-test-lib/src/main/java/com/palantir/dialogue/AbstractChannelTest.java @@ -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; @@ -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 @@ -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 diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/Channels.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/Channels.java index 0520494ae..620537c4e 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/Channels.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/Channels.java @@ -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) diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/MethodRequestBodyValidatingChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/MethodRequestBodyValidatingChannel.java new file mode 100644 index 000000000..6560dbcd0 --- /dev/null +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/MethodRequestBodyValidatingChannel.java @@ -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 GET and DELETE requests do not contain bodies. */ +final class MethodRequestBodyValidatingChannel implements Channel { + + private final Channel delegate; + + MethodRequestBodyValidatingChannel(Channel delegate) { + this.delegate = delegate; + } + + @Override + public ListenableFuture 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); + } +} diff --git a/dialogue-java-client/src/main/java/com/palantir/dialogue/HttpChannel.java b/dialogue-java-client/src/main/java/com/palantir/dialogue/HttpChannel.java index 9bf1143e2..d05324b17 100644 --- a/dialogue-java-client/src/main/java/com/palantir/dialogue/HttpChannel.java +++ b/dialogue-java-client/src/main/java/com/palantir/dialogue/HttpChannel.java @@ -84,7 +84,6 @@ public ListenableFuture 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"); httpRequest.GET(); break; case POST: @@ -94,8 +93,6 @@ public ListenableFuture 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; }