From d419826ca0d17c94ffd8829442096c1d7fbf90b9 Mon Sep 17 00:00:00 2001 From: Ram Anaswara Date: Sun, 15 Nov 2020 15:39:57 +0000 Subject: [PATCH] Fix for Path variables with / are not url encoded # Conflicts: # spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientProperties.java # spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java # spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractIntegrationTests.java # spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java --- .../openfeign/FeignClientProperties.java | 21 +++- .../openfeign/FeignClientsConfiguration.java | 8 +- .../openfeign/support/SpringMvcContract.java | 16 +++ ...ractSpringMvcContractIntegrationTests.java | 116 ++++++++++++++++++ .../SpringMvcContractIntegrationTests.java | 84 ++----------- ...ContractSlashEncodingIntegrationTests.java | 49 ++++++++ .../support/SpringMvcContractTests.java | 87 ++++++++++++- 7 files changed, 301 insertions(+), 80 deletions(-) create mode 100644 spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/AbstractSpringMvcContractIntegrationTests.java create mode 100644 spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractSlashEncodingIntegrationTests.java diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientProperties.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientProperties.java index c656c4a3..3d81d6f2 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientProperties.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientProperties.java @@ -36,6 +36,7 @@ import org.springframework.boot.context.properties.ConfigurationProperties; /** * @author Eko Kurniawan Khannedy * @author Ilia Ilinykh + * @author Ram Anaswara */ @ConfigurationProperties("feign.client") public class FeignClientProperties { @@ -46,6 +47,12 @@ public class FeignClientProperties { private Map config = new HashMap<>(); + /** + * Feign clients do not encode slash `/` characters by default. To change this + * behavior, set the `decodeSlash` to `false`. + */ + private boolean decodeSlash = true; + public boolean isDefaultToProperties() { return this.defaultToProperties; } @@ -70,6 +77,14 @@ public class FeignClientProperties { this.config = config; } + public boolean isDecodeSlash() { + return decodeSlash; + } + + public void setDecodeSlash(boolean decodeSlash) { + this.decodeSlash = decodeSlash; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -81,12 +96,14 @@ public class FeignClientProperties { FeignClientProperties that = (FeignClientProperties) o; return this.defaultToProperties == that.defaultToProperties && Objects.equals(this.defaultConfig, that.defaultConfig) - && Objects.equals(this.config, that.config); + && Objects.equals(this.config, that.config) + && Objects.equals(this.decodeSlash, that.decodeSlash); } @Override public int hashCode() { - return Objects.hash(this.defaultToProperties, this.defaultConfig, this.config); + return Objects.hash(this.defaultToProperties, this.defaultConfig, this.config, + this.decodeSlash); } /** diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java index ee55359d..46cf1474 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java @@ -82,6 +82,9 @@ public class FeignClientsConfiguration { @Autowired(required = false) private SpringDataWebProperties springDataWebProperties; + @Autowired(required = false) + private FeignClientProperties feignClientProperties; + @Bean @ConditionalOnMissingBean public Decoder feignDecoder() { @@ -118,7 +121,10 @@ public class FeignClientsConfiguration { @Bean @ConditionalOnMissingBean public Contract feignContract(ConversionService feignConversionService) { - return new SpringMvcContract(this.parameterProcessors, feignConversionService); + boolean decodeSlash = feignClientProperties == null + || feignClientProperties.isDecodeSlash(); + return new SpringMvcContract(this.parameterProcessors, feignConversionService, + decodeSlash); } @Bean diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java index cdaa9b73..a589eb69 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java @@ -78,6 +78,7 @@ import static org.springframework.core.annotation.AnnotatedElementUtils.findMerg * @author Aaron Whiteside * @author Artyom Romanenko * @author Darren Foong + * @author Ram Anaswara */ public class SpringMvcContract extends Contract.BaseContract implements ResourceLoaderAware { @@ -104,6 +105,8 @@ public class SpringMvcContract extends Contract.BaseContract private ResourceLoader resourceLoader = new DefaultResourceLoader(); + private boolean decodeSlash; + public SpringMvcContract() { this(Collections.emptyList()); } @@ -116,6 +119,12 @@ public class SpringMvcContract extends Contract.BaseContract public SpringMvcContract( List annotatedParameterProcessors, ConversionService conversionService) { + this(annotatedParameterProcessors, conversionService, true); + } + + public SpringMvcContract( + List annotatedParameterProcessors, + ConversionService conversionService, boolean decodeSlash) { Assert.notNull(annotatedParameterProcessors, "Parameter processors can not be null."); Assert.notNull(conversionService, "ConversionService can not be null."); @@ -126,6 +135,7 @@ public class SpringMvcContract extends Contract.BaseContract annotatedArgumentProcessors = toAnnotatedArgumentProcessorMap(processors); this.conversionService = conversionService; convertingExpanderFactory = new ConvertingExpanderFactory(conversionService); + this.decodeSlash = decodeSlash; } private static TypeDescriptor createTypeDescriptor(Method method, int paramIndex) { @@ -183,6 +193,9 @@ public class SpringMvcContract extends Contract.BaseContract pathValue = "/" + pathValue; } data.template().uri(pathValue); + if (data.template().decodeSlash() != decodeSlash) { + data.template().decodeSlash(decodeSlash); + } } } } @@ -247,6 +260,9 @@ public class SpringMvcContract extends Contract.BaseContract pathValue = "/" + pathValue; } data.template().uri(pathValue, true); + if (data.template().decodeSlash() != decodeSlash) { + data.template().decodeSlash(decodeSlash); + } } } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/AbstractSpringMvcContractIntegrationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/AbstractSpringMvcContractIntegrationTests.java new file mode 100644 index 00000000..ea7d5cb2 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/AbstractSpringMvcContractIntegrationTests.java @@ -0,0 +1,116 @@ +/* + * Copyright 2013-2020 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 + * + * https://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.openfeign.support; + +import java.nio.charset.Charset; + +import feign.Response; +import feign.codec.Decoder; +import feign.codec.Encoder; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; + +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.cloud.openfeign.EnableFeignClients; +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.cloud.openfeign.test.NoSecurityConfiguration; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Import; +import org.springframework.http.HttpHeaders; +import org.springframework.util.SocketUtils; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestHeader; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; + +/** + * Abstract class for the integration tests for {@link SpringMvcContract}. + * + * @author Ram Anaswara + */ +public class AbstractSpringMvcContractIntegrationTests { + + @BeforeAll + public static void beforeClass() { + System.setProperty("server.port", + String.valueOf(SocketUtils.findAvailableTcpPort())); + } + + @AfterAll + public static void afterClass() { + System.clearProperty("server.port"); + } + + protected String getUrlQueryParam(Response response) { + return response.request().requestTemplate().queries().get("url").stream() + .findFirst().orElseThrow(IllegalStateException::new); + } + + @FeignClient(name = "test", url = "http://localhost:${server.port}/", + configuration = NoCodecsFeignConfiguration.class) + interface TestClient { + + @PostMapping("/test") + Object sendMessage(@RequestBody String message, + @RequestHeader(HttpHeaders.CONTENT_TYPE) String acceptHeader); + + @GetMapping("/get") + Object getMessage(@RequestParam String url); + + } + + @Configuration(proxyBeanMethods = false) + @EnableFeignClients(clients = TestClient.class) + @EnableAutoConfiguration + @RestController + @Import(NoSecurityConfiguration.class) + protected static class Config { + + @PostMapping("/test") + Object sendMessage(@RequestBody String message, + @RequestHeader(HttpHeaders.CONTENT_TYPE) String acceptHeader) { + return message; + } + + @GetMapping("/get") + Object getMessage(@RequestParam String url) { + return url; + } + + } + + // Avoid feign.codec.EncodeException - this feature works for users that override + // Encoder + protected static class NoCodecsFeignConfiguration { + + @Bean + public Decoder decoder() { + return (response, type) -> response; + } + + @Bean + public Encoder encoder() { + return (object, bodyType, request) -> request + .body(object.toString().getBytes(), Charset.defaultCharset()); + } + + } + +} diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractIntegrationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractIntegrationTests.java index cc2e9607..d786739c 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractIntegrationTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractIntegrationTests.java @@ -16,101 +16,41 @@ package org.springframework.cloud.openfeign.support; -import java.nio.charset.Charset; - -import feign.codec.Decoder; -import feign.codec.Encoder; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.BeforeAll; +import feign.Response; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.cloud.openfeign.EnableFeignClients; -import org.springframework.cloud.openfeign.FeignClient; -import org.springframework.cloud.openfeign.test.NoSecurityConfiguration; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.Import; -import org.springframework.http.HttpHeaders; -import org.springframework.util.SocketUtils; -import org.springframework.web.bind.annotation.PostMapping; -import org.springframework.web.bind.annotation.RequestBody; -import org.springframework.web.bind.annotation.RequestHeader; -import org.springframework.web.bind.annotation.RestController; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; /** - * Integration tests for {@link SpringMvcContract} + * Integration tests for {@link SpringMvcContract}. * * @author Olga Maciaszek-Sharma + * @author Ram Anaswara */ -@SpringBootTest(classes = SpringMvcContractIntegrationTests.Config.class, +@SpringBootTest(classes = AbstractSpringMvcContractIntegrationTests.Config.class, webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT) -public class SpringMvcContractIntegrationTests { +public class SpringMvcContractIntegrationTests + extends AbstractSpringMvcContractIntegrationTests { @Autowired private TestClient client; - @BeforeAll - public static void beforeClass() { - System.setProperty("server.port", - String.valueOf(SocketUtils.findAvailableTcpPort())); - } - - @AfterAll - public static void afterClass() { - System.clearProperty("server.port"); - } - @Test public void shouldNotThrowInvalidMediaTypeExceptionWhenContentTypeTemplateUsed() { assertThatCode(() -> client.sendMessage("test", "text/markdown")) .doesNotThrowAnyException(); } - @FeignClient(name = "test", url = "http://localhost:${server.port}/", - configuration = NoCodecsFeignConfiguration.class) - interface TestClient { - - @PostMapping("/test") - Object sendMessage(@RequestBody String message, - @RequestHeader(HttpHeaders.CONTENT_TYPE) String acceptHeader); - - } - - @Configuration(proxyBeanMethods = false) - @EnableFeignClients(clients = TestClient.class) - @EnableAutoConfiguration - @RestController - @Import(NoSecurityConfiguration.class) - protected static class Config { - - @PostMapping("/test") - Object sendMessage(@RequestBody String message, - @RequestHeader(HttpHeaders.CONTENT_TYPE) String acceptHeader) { - return message; - } - - } - - // avoid feign.codec.EncodeException - this feature works for users that override - // Encoder - protected static class NoCodecsFeignConfiguration { - - @Bean - public Decoder decoder() { - return (response, type) -> response; - } - - @Bean - public Encoder encoder() { - return (object, bodyType, request) -> request - .body(object.toString().getBytes(), Charset.defaultCharset()); - } + @Test + public void feignClientShouldPreserveSlash() { + Response response = (Response) client.getMessage("https://www.google.com"); + String urlQueryParam = getUrlQueryParam(response); + assertThat(urlQueryParam).isEqualTo("https%3A//www.google.com"); } } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractSlashEncodingIntegrationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractSlashEncodingIntegrationTests.java new file mode 100644 index 00000000..4f36af18 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractSlashEncodingIntegrationTests.java @@ -0,0 +1,49 @@ +/* + * Copyright 2013-2020 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 + * + * https://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.openfeign.support; + +import feign.Response; +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Integration tests for {@link SpringMvcContract}. + * + * @author Ram Anaswara + */ +@SpringBootTest(classes = SpringMvcContractSlashEncodingIntegrationTests.Config.class, + webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT, + properties = { "feign.client.decodeSlash=false" }) +public class SpringMvcContractSlashEncodingIntegrationTests + extends AbstractSpringMvcContractIntegrationTests { + + @Autowired + private TestClient client; + + @Test + public void feignClientShouldNotDecodeEncodedSlash() { + Response response = (Response) client.getMessage("https://www.google.com"); + + String urlQueryParam = getUrlQueryParam(response); + assertThat(urlQueryParam).isEqualTo("https%3A%2F%2Fwww.google.com"); + } + +} diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java index 983743b3..757bcc69 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java @@ -118,11 +118,7 @@ public class SpringMvcContractTests { @Before public void setup() { - FormattingConversionServiceFactoryBean conversionServiceFactoryBean = new FormattingConversionServiceFactoryBean(); - conversionServiceFactoryBean.afterPropertiesSet(); - ConversionService conversionService = conversionServiceFactoryBean.getObject(); - - contract = new SpringMvcContract(Collections.emptyList(), conversionService); + contract = new SpringMvcContract(Collections.emptyList(), getConversionService()); } @Test @@ -136,6 +132,23 @@ public class SpringMvcContractTests { assertThat(data.template().method()).isEqualTo("GET"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); + + assertThat(data.template().decodeSlash()).isTrue(); + } + + @Test + public void testProcessAnnotationOnMethod_Simple_SlashEncoded() throws Exception { + contract = new SpringMvcContract(Collections.emptyList(), getConversionService(), + false); + + Method method = TestTemplate_Simple.class.getDeclaredMethod("getTest", + String.class); + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertThat(data.template().url()).isEqualTo("/test/{id}"); + + assertThat(data.template().decodeSlash()).isFalse(); } @Test @@ -194,6 +207,24 @@ public class SpringMvcContractTests { assertThat(data.template().method()).isEqualTo("GET"); assertThat(data.indexToName().get(0).iterator().next()).isEqualTo("classId"); + + assertThat(data.template().decodeSlash()).isTrue(); + } + + @Test + public void testProcessAnnotations_Class_AnnotationsGetAllTests_EncodeSlash() + throws Exception { + contract = new SpringMvcContract(Collections.emptyList(), getConversionService(), + false); + + Method method = TestTemplate_Class_Annotations.class + .getDeclaredMethod("getAllTests", String.class); + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertThat(data.template().url()).isEqualTo("/prepend/{classId}"); + + assertThat(data.template().decodeSlash()).isFalse(); } @Test @@ -213,6 +244,30 @@ public class SpringMvcContractTests { assertThat(data.indexToName().get(0).iterator().next()) .isEqualTo(data.indexToName().get(0).iterator().next()); + assertThat(data.indexToName().get(0).iterator().next()) + .isEqualTo(data.indexToName().get(0).iterator().next()); + assertThat(data.template().decodeSlash()).isTrue(); + } + + @Test + public void testProcessAnnotations_ExtendedInterface_EncodeSlash() throws Exception { + contract = new SpringMvcContract(Collections.emptyList(), getConversionService(), + false); + + Method extendedMethod = TestTemplate_Extended.class.getMethod("getAllTests", + String.class); + MethodMetadata extendedData = contract.parseAndValidateMetadata( + extendedMethod.getDeclaringClass(), extendedMethod); + + Method method = TestTemplate_Class_Annotations.class + .getDeclaredMethod("getAllTests", String.class); + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertThat(data.template().url()).isEqualTo(extendedData.template().url()); + assertThat(data.template().method()).isEqualTo(extendedData.template().method()); + + assertThat(data.template().decodeSlash()).isFalse(); } @Test @@ -390,6 +445,22 @@ public class SpringMvcContractTests { assertThat(data.template().method()).isEqualTo("GET"); assertThat(data.template().headers().get("Accept").iterator().next()) .isEqualTo(MediaType.APPLICATION_JSON_VALUE); + assertThat(data.template().decodeSlash()).isTrue(); + } + + @Test + public void testProcessAnnotations_Advanced3_DecodeSlashFlagNotModified() + throws Exception { + contract = new SpringMvcContract(Collections.emptyList(), getConversionService(), + false); + + Method method = TestTemplate_Simple.class.getDeclaredMethod("getTest"); + MethodMetadata data = contract + .parseAndValidateMetadata(method.getDeclaringClass(), method); + + assertThat(data.template().url()).isEqualTo("/"); + + assertThat(data.template().decodeSlash()).isTrue(); } @Test @@ -608,6 +679,12 @@ public class SpringMvcContractTests { assertThat(data.formParams()).contains("file", "id"); } + private ConversionService getConversionService() { + FormattingConversionServiceFactoryBean conversionServiceFactoryBean = new FormattingConversionServiceFactoryBean(); + conversionServiceFactoryBean.afterPropertiesSet(); + return conversionServiceFactoryBean.getObject(); + } + public interface TestTemplate_Simple { @RequestMapping(value = "/test/{id}", method = RequestMethod.GET,