From 0ee8de21c1a64bef8ec5558a7f434c99af2f7885 Mon Sep 17 00:00:00 2001 From: Olga MaciaszekSharma Date: Wed, 20 Mar 2024 19:02:29 +0100 Subject: [PATCH] Fix merging default headers and params. Fixes gh-1001. --- .../openfeign/FeignClientFactoryBean.java | 21 ++- ...eignClientFactoryBeanIntegrationTests.java | 156 ++++++++++++++++++ .../resources/application-defaultstest.yml | 31 ++++ 3 files changed, 201 insertions(+), 7 deletions(-) create mode 100644 spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryBeanIntegrationTests.java create mode 100644 spring-cloud-openfeign-core/src/test/resources/application-defaultstest.yml diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java index 5fc73f80e..3b768145a 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2023 the original author or authors. + * Copyright 2013-2024 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. @@ -324,19 +324,26 @@ protected void configureUsingProperties(FeignClientProperties.FeignClientConfigu protected void configureDefaultRequestElements(FeignClientProperties.FeignClientConfiguration defaultConfig, FeignClientProperties.FeignClientConfiguration clientConfig, Feign.Builder builder) { - Map> defaultRequestHeaders = defaultConfig != null - ? defaultConfig.getDefaultRequestHeaders() : new HashMap<>(); + Map> defaultRequestHeaders = new HashMap<>(); + if (defaultConfig != null) { + defaultConfig.getDefaultRequestHeaders() + .forEach((k, v) -> defaultRequestHeaders.put(k, new ArrayList<>(v))); + } if (clientConfig != null) { - defaultRequestHeaders.putAll(clientConfig.getDefaultRequestHeaders()); + clientConfig.getDefaultRequestHeaders().forEach((k, v) -> defaultRequestHeaders.put(k, new ArrayList<>(v))); } if (!defaultRequestHeaders.isEmpty()) { addDefaultRequestHeaders(defaultRequestHeaders, builder); } - Map> defaultQueryParameters = defaultConfig != null - ? defaultConfig.getDefaultQueryParameters() : new HashMap<>(); + Map> defaultQueryParameters = new HashMap<>(); + if (defaultConfig != null) { + defaultConfig.getDefaultQueryParameters() + .forEach((k, v) -> defaultQueryParameters.put(k, new ArrayList<>(v))); + } if (clientConfig != null) { - defaultQueryParameters.putAll(clientConfig.getDefaultQueryParameters()); + clientConfig.getDefaultQueryParameters() + .forEach((k, v) -> defaultQueryParameters.put(k, new ArrayList<>(v))); } if (!defaultQueryParameters.isEmpty()) { addDefaultQueryParams(defaultQueryParameters, builder); diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryBeanIntegrationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryBeanIntegrationTests.java new file mode 100644 index 000000000..5091754d0 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/FeignClientFactoryBeanIntegrationTests.java @@ -0,0 +1,156 @@ +/* + * Copyright 2013-2024 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; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +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.boot.test.web.server.LocalServerPort; +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.loadbalancer.annotation.LoadBalancerClient; +import org.springframework.cloud.loadbalancer.annotation.LoadBalancerClients; +import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSupplier; +import org.springframework.cloud.loadbalancer.support.ServiceInstanceListSuppliers; +import org.springframework.cloud.openfeign.test.NoSecurityConfiguration; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Import; +import org.springframework.http.HttpHeaders; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RequestHeader; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; + +import static java.util.Map.entry; +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; + +/** + * Integration tests for {@link FeignClientFactoryBean}. + * + * @author Olga Maciaszek-Sharma + */ +@SpringBootTest(classes = FeignClientFactoryBeanIntegrationTests.Application.class, + webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ActiveProfiles("defaultstest") +class FeignClientFactoryBeanIntegrationTests { + + @Autowired + TestClientA testClientA; + + @Autowired + TestClientB testClientB; + + @Test + void shouldProcessDefaultRequestHeadersPerClient() { + assertThat(testClientA.headers()).isNotNull() + .contains(entry("x-custom-header-2", List.of("2 from default")), + entry("x-custom-header", List.of("from client A"))); + assertThat(testClientB.headers()).isNotNull() + .contains(entry("x-custom-header-2", List.of("2 from default")), + entry("x-custom-header", List.of("from client B"))); + } + + @Test + void shouldProcessDefaultQueryParamsPerClient() { + assertThat(testClientA.params()).isNotNull() + .contains(entry("customParam2", "2 from default"), + entry("customParam1", "from client A")); + assertThat(testClientB.params()).isNotNull() + .contains(entry("customParam2", "2 from default"), + entry("customParam1", "from client B")); + } + + @FeignClient("testClientA") + public interface TestClientA { + + @GetMapping("/headers") + Map> headers(); + + @GetMapping("/params") + Map params(); + + } + + @FeignClient("testClientB") + public interface TestClientB { + + @GetMapping("/headers") + Map> headers(); + + @GetMapping("/params") + Map params(); + + } + + @EnableAutoConfiguration + @EnableFeignClients(clients = {TestClientA.class, TestClientB.class}) + @RestController + @LoadBalancerClients({@LoadBalancerClient(name = "testClientA", configuration = TestClientAConfiguration.class), + @LoadBalancerClient(name = "testClientB", configuration = TestClientBConfiguration.class)}) + @RequestMapping + @Import(NoSecurityConfiguration.class) + protected static class Application { + + @GetMapping(value = "/headers", produces = APPLICATION_JSON_VALUE) + public Map> headers(@RequestHeader HttpHeaders headers) { + return new HashMap<>(headers); + } + + @GetMapping(value = "/params", produces = APPLICATION_JSON_VALUE) + public Map headersB(@RequestParam Map params) { + return params; + } + + } + + // LoadBalancer with fixed server list for "testClientA" pointing to localhost + static class TestClientAConfiguration { + + @LocalServerPort + private int port = 0; + + @Bean + public ServiceInstanceListSupplier testClientAServiceInstanceListSupplier() { + return ServiceInstanceListSuppliers.from("testClientA", + new DefaultServiceInstance("local-1", "testClientA", "localhost", port, false)); + } + + } + + // LoadBalancer with fixed server list for "testClientB" pointing to localhost + static class TestClientBConfiguration { + + @LocalServerPort + private int port = 0; + + @Bean + public ServiceInstanceListSupplier testClientBServiceInstanceListSupplier() { + return ServiceInstanceListSuppliers.from("testClientB", + new DefaultServiceInstance("local-1", "testClientB", "localhost", port, false)); + } + + } + +} diff --git a/spring-cloud-openfeign-core/src/test/resources/application-defaultstest.yml b/spring-cloud-openfeign-core/src/test/resources/application-defaultstest.yml new file mode 100644 index 000000000..5e11b73a5 --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/resources/application-defaultstest.yml @@ -0,0 +1,31 @@ +spring: + cloud: + openfeign: + client: + config: + default: + logger-level: FULL + default-request-headers: + x-custom-header: + - "default" + x-custom-header-2: + - "2 from default" + default-query-parameters: + customParam1: + - "default" + customParam2: + - "2 from default" + testClientA: + default-request-headers: + x-custom-header: + - "from client A" + default-query-parameters: + customParam1: + - "from client A" + testClientB: + default-request-headers: + x-custom-header: + - "from client B" + default-query-parameters: + customParam1: + - "from client B"