From 15e01dfe5d6242ac8a6ac61363d6287568275857 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Mon, 6 Mar 2017 20:11:03 -0700 Subject: [PATCH] Don't remove content-length header in feign ribbon. Previously, when feign used the Ribbon RestClient, an exception was thrown if Content-Length was set. Over time, feign has moved away from RestClient, yet removing the header still happened. Fixes gh-1705 --- .../feign/ribbon/FeignLoadBalancer.java | 2 - .../netflix/feign/valid/FeignClientTests.java | 8 +- .../feign/valid/FeignHttpClientTests.java | 11 +- .../netflix/feign/valid/FeignOkHttpTests.java | 192 ++++++++++++++++++ 4 files changed, 208 insertions(+), 5 deletions(-) create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignOkHttpTests.java diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancer.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancer.java index f4ba1f08..5b5ca69f 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancer.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancer.java @@ -120,8 +120,6 @@ public class FeignLoadBalancer extends private Request toRequest(Request request) { Map> headers = new LinkedHashMap<>( request.headers()); - // Apache client barfs if you set the content length - headers.remove(Util.CONTENT_LENGTH); return Request.create(request.method(),getUri().toASCIIString(),headers,request.body(),request.charset()); } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientTests.java index 6550984b..76518ab2 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientTests.java @@ -62,6 +62,7 @@ import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; @@ -455,8 +456,11 @@ public class FeignClientTests { } @RequestMapping(method = RequestMethod.POST, consumes = "application/vnd.io.spring.cloud.test.v1+json", produces = "application/vnd.io.spring.cloud.test.v1+json", path = "/complex") - String complex(String body) { - return "{\"value\":\"OK\"}"; + String complex(@RequestBody String body, @RequestHeader("Content-Length") int contentLength) { + if (contentLength <= 0) { + throw new IllegalArgumentException("Invalid Content-Length "+ contentLength); + } + return body; } @RequestMapping(method = RequestMethod.GET, path = "/tostring") diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignHttpClientTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignHttpClientTests.java index 03758b2f..27aa31b9 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignHttpClientTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignHttpClientTests.java @@ -42,6 +42,8 @@ import org.springframework.http.ResponseEntity; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.RestController; @@ -110,7 +112,14 @@ public class FeignHttpClientTests { } @RequestMapping(method = RequestMethod.PATCH, value = "/hellop") - public ResponseEntity patchHello() { + public ResponseEntity patchHello(@RequestBody Hello hello, + @RequestHeader("Content-Length") int contentLength) { + if (contentLength <= 0) { + throw new IllegalArgumentException("Invalid Content-Length "+ contentLength); + } + if (!hello.getMessage().equals("foo")) { + throw new IllegalArgumentException("Invalid Hello: " + hello.getMessage()); + } return ResponseEntity.ok().header("X-Hello", "hello world patch").build(); } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignOkHttpTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignOkHttpTests.java new file mode 100644 index 00000000..5b2d917f --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignOkHttpTests.java @@ -0,0 +1,192 @@ +/* + * Copyright 2013-2017 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.springframework.cloud.netflix.feign.valid; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.context.SpringBootTest.WebEnvironment; +import org.springframework.cloud.netflix.feign.EnableFeignClients; +import org.springframework.cloud.netflix.feign.FeignClient; +import org.springframework.cloud.netflix.feign.ribbon.LoadBalancerFeignClient; +import org.springframework.cloud.netflix.ribbon.RibbonClient; +import org.springframework.cloud.netflix.ribbon.StaticServerList; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.http.ResponseEntity; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestHeader; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RestController; + +import com.netflix.loadbalancer.Server; +import com.netflix.loadbalancer.ServerList; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; + +import feign.Client; +import lombok.AllArgsConstructor; +import lombok.Data; +import lombok.NoArgsConstructor; + +/** + * @author Spencer Gibb + */ +@RunWith(SpringJUnit4ClassRunner.class) +@SpringBootTest(classes = FeignOkHttpTests.Application.class, webEnvironment = WebEnvironment.RANDOM_PORT, value = { + "spring.application.name=feignclienttest", "feign.hystrix.enabled=false", + "feign.okhttp.enabled=true" }) +@DirtiesContext +public class FeignOkHttpTests { + + @Value("${local.server.port}") + private int port = 0; + + @Autowired + private TestClient testClient; + + @Autowired + private Client feignClient; + + @Autowired + private UserClient userClient; + + @FeignClient("localapp") + protected interface TestClient extends BaseTestClient { + } + + protected interface BaseTestClient { + @RequestMapping(method = RequestMethod.GET, value = "/hello") + Hello getHello(); + + @RequestMapping(method = RequestMethod.PATCH, value = "/hellop", consumes = "application/json") + ResponseEntity patchHello(Hello hello); + } + + protected interface UserService { + @RequestMapping(method = RequestMethod.GET, value = "/users/{id}") + User getUser(@PathVariable("id") long id); + } + + @FeignClient("localapp") + protected interface UserClient extends UserService { + } + + @Configuration + @EnableAutoConfiguration + @RestController + @EnableFeignClients(clients = { TestClient.class, UserClient.class }) + @RibbonClient(name = "localapp", configuration = LocalRibbonClientConfiguration.class) + protected static class Application implements UserService { + + @RequestMapping(method = RequestMethod.GET, value = "/hello") + public Hello getHello() { + return new Hello("hello world 1"); + } + + @RequestMapping(method = RequestMethod.PATCH, value = "/hellop") + public ResponseEntity patchHello(@RequestBody Hello hello, + @RequestHeader("Content-Length") int contentLength) { + if (contentLength <= 0) { + throw new IllegalArgumentException("Invalid Content-Length "+ contentLength); + } + if (!hello.getMessage().equals("foo")) { + throw new IllegalArgumentException("Invalid Hello: " + hello.getMessage()); + } + return ResponseEntity.ok().header("X-Hello", "hello world patch").build(); + } + + @Override + public User getUser(@PathVariable("id") long id) { + return new User("John Smith"); + } + + } + + @Test + public void testSimpleType() { + Hello hello = this.testClient.getHello(); + assertNotNull("hello was null", hello); + assertEquals("first hello didn't match", new Hello("hello world 1"), hello); + } + + @Test + public void testPatch() { + ResponseEntity response = this.testClient.patchHello(new Hello("foo")); + assertThat(response, is(notNullValue())); + String header = response.getHeaders().getFirst("X-Hello"); + assertThat(header, equalTo("hello world patch")); + } + + @Test + public void testFeignClientType() throws IllegalAccessException { + assertThat(this.feignClient, is(instanceOf(LoadBalancerFeignClient.class))); + LoadBalancerFeignClient client = (LoadBalancerFeignClient) this.feignClient; + Client delegate = client.getDelegate(); + assertThat(delegate, is(instanceOf(feign.okhttp.OkHttpClient.class))); + } + + @Test + public void testFeignInheritanceSupport() { + assertNotNull("UserClient was null", this.userClient); + final User user = this.userClient.getUser(1); + assertNotNull("Returned user was null", user); + assertEquals("Users were different", user, new User("John Smith")); + } + + @Data + @AllArgsConstructor + @NoArgsConstructor + public static class Hello { + private String message; + } + + @Data + @AllArgsConstructor + @NoArgsConstructor + public static class User { + private String name; + } + + // Load balancer with fixed server list for "local" pointing to localhost + @Configuration + static class LocalRibbonClientConfiguration { + + @Value("${local.server.port}") + private int port = 0; + + @Bean + public ServerList ribbonServerList() { + return new StaticServerList<>(new Server("localhost", this.port)); + } + + } +}