From 87258a03c73a13c1cd254412602c314c7e7966ed Mon Sep 17 00:00:00 2001 From: spring-builds Date: Thu, 13 Mar 2025 13:08:54 +0000 Subject: [PATCH 1/2] Bumping versions --- README.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.adoc b/README.adoc index 380a8fbf..5d9ed438 100644 --- a/README.adoc +++ b/README.adoc @@ -365,7 +365,7 @@ Spring Cloud Build brings along the `basepom:duplicate-finder-maven-plugin`, th [[duplicate-finder-configuration]] === Duplicate Finder configuration -Duplicate finder is *enabled by default* and will run in the `verify` phase of your Maven build, but it will only take effect in your project if you add the `duplicate-finder-maven-plugin` to the `build` section of the projecst's `pom.xml`. +Duplicate finder is *enabled by default* and will run in the `verify` phase of your Maven build, but it will only take effect in your project if you add the `duplicate-finder-maven-plugin` to the `build` section of the project's `pom.xml`. .pom.xml [source,xml] From d959f4a071544c66f24fcaaaff0fed58ddc82f8d Mon Sep 17 00:00:00 2001 From: marnee01 <20825300+marnee01@users.noreply.github.com> Date: Fri, 21 Mar 2025 14:12:04 -0500 Subject: [PATCH 2/2] Config Client: Fix logic to try all URLs (#2798) There was a miss when implementing the MultipleUriStrategy.ALWAYS. It was not trying the next URL if response was null, or if the HTTP Status Code was not OK, but no exception was thrown. (This author does not know under what scenario null would be returned as a response, but the code was already checking for it, so I updated the logic to also try the next URL for null response.) Closes gh-2735 Signed-off-by: marnee01 --- .../client/ConfigServerConfigDataLoader.java | 17 ++++- .../ConfigServicePropertySourceLocator.java | 17 ++++- .../ConfigServerConfigDataLoaderTests.java | 63 +++++++++++++++++- ...nfigServicePropertySourceLocatorTests.java | 64 ++++++++++++++++++- 4 files changed, 157 insertions(+), 4 deletions(-) diff --git a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java index 8b4fb439..223d2bab 100644 --- a/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java +++ b/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLoader.java @@ -359,7 +359,22 @@ public class ConfigServerConfigDataLoader implements ConfigDataLoader logger); + ClientHttpRequestFactory requestFactory = mock(ClientHttpRequestFactory.class); + RestTemplate restTemplate = new RestTemplate(requestFactory); + mockRequestResponse(requestFactory, badURI, HttpStatus.TEMPORARY_REDIRECT); + mockRequestResponse(requestFactory, goodURI, HttpStatus.OK); + when(bootstrapContext.get(RestTemplate.class)).thenReturn(restTemplate); + assertThat(this.loader.load(context, resource)).isNull(); + } + + @Test + public void shouldUseNextUriFor_NoExceptionNotOK_And_CONNECTION_TIMEOUT_ONLY_Strategy_WithFailFastIsTrue() + throws Exception { + // At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to + // be thrown back to + // getRemoteEnvironment, but since status is not OK, the method returns null and + // locate method will + // throw an IllegalStateException with no cause, since fail-fast=true. Second URL + // is never tried, due to the strategy. + assertNextUriIsNotTried(true, ConfigClientProperties.MultipleUriStrategy.CONNECTION_TIMEOUT_ONLY, + HttpStatus.TEMPORARY_REDIRECT, null // IllegalStateException has no cause, + // because getRemoteEnvironment did + // not throw an exception + ); + } + + @Test + public void shouldUseNextUriFor_NoExceptionNotOK_And_ALWAYS_Strategy() throws Exception { + // At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to + // be thrown back to + // getRemoteEnvironment, but since status is not OK, the method will treat it the + // same as an exception and + // thus try the next URL. + assertNextUriIsTried(ConfigClientProperties.MultipleUriStrategy.ALWAYS, HttpStatus.TEMPORARY_REDIRECT); + } + @Test public void shouldUseNextUriFor_TimeOut_And_ALWAYS_Strategy() throws Exception { assertNextUriIsTriedOnTimeout(ConfigClientProperties.MultipleUriStrategy.ALWAYS); @@ -631,12 +686,18 @@ public class ConfigServerConfigDataLoaderTests { private void assertNextUriIsNotTried(ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse, Class expectedCause) throws Exception { + assertNextUriIsNotTried(true, multipleUriStrategy, firstUriResponse, expectedCause); + } + + private void assertNextUriIsNotTried(boolean failFast, + ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse, + Class expectedCause) throws Exception { // Set up with two URIs. String badURI = "http://baduri"; String goodURI = "http://localhost:8888"; String[] uris = new String[] { badURI, goodURI }; properties.setUri(uris); - properties.setFailFast(true); + properties.setFailFast(failFast); // Strategy is CONNECTION_TIMEOUT_ONLY, so it should not try the next URI for // INTERNAL_SERVER_ERROR properties.setMultipleUriStrategy(multipleUriStrategy); diff --git a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java index b53d80a1..eda69567 100644 --- a/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java +++ b/spring-cloud-config-client/src/test/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocatorTests.java @@ -20,6 +20,7 @@ import java.io.ByteArrayInputStream; import java.io.IOException; import java.net.URI; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.LinkedHashMap; @@ -332,6 +333,61 @@ public class ConfigServicePropertySourceLocatorTests { assertNextUriIsTried(ConfigClientProperties.MultipleUriStrategy.ALWAYS, HttpStatus.INTERNAL_SERVER_ERROR); } + @Test + public void shouldUseNextUriFor_NoExceptionNotOK_And_CONNECTION_TIMEOUT_ONLY_Strategy_FailFastIsFalse() + throws Exception { + // At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to + // be thrown back to + // getRemoteEnvironment, but since status is not OK, the method returns null and + // locate method will + // simply return null since fail-fast=false. (Second URL is never tried, due to + // the strategy. + + // Set up with two URIs. + ConfigClientProperties clientProperties = new ConfigClientProperties(this.environment); + String badURI = "http://baduri"; + String goodURI = "http://localhost:8888"; + String[] uris = new String[] { badURI, goodURI }; + clientProperties.setUri(uris); + clientProperties.setFailFast(false); + // Strategy is CONNECTION_TIMEOUT_ONLY, so it should not try the next URI for + // TEMPORARY_REDIRECT + clientProperties.setMultipleUriStrategy(ConfigClientProperties.MultipleUriStrategy.CONNECTION_TIMEOUT_ONLY); + this.locator = new ConfigServicePropertySourceLocator(clientProperties); + ClientHttpRequestFactory requestFactory = Mockito.mock(ClientHttpRequestFactory.class); + RestTemplate restTemplate1 = new RestTemplate(requestFactory); + mockRequestResponse(requestFactory, badURI, HttpStatus.TEMPORARY_REDIRECT); + mockRequestResponse(requestFactory, goodURI, HttpStatus.OK); + this.locator.setRestTemplate(restTemplate1); + assertThat(this.locator.locateCollection(this.environment)).isEqualTo(Collections.emptyList()); + } + + @Test + public void shouldUseNextUriFor_NoExceptionNotOK_And_CONNECTION_TIMEOUT_ONLY_Strategy_WithFailFastIsTrue() + throws Exception { + // At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to + // be thrown back to + // getRemoteEnvironment, but since status is not OK, the method returns null and + // locate method will + // throw an IllegalStateException with no cause, since fail-fast=true. Second URL + // is never tried, due to the strategy. + assertNextUriIsNotTried(true, ConfigClientProperties.MultipleUriStrategy.CONNECTION_TIMEOUT_ONLY, + HttpStatus.TEMPORARY_REDIRECT, null // IllegalStateException has no cause, + // because getRemoteEnvironment did + // not throw an exception + ); + } + + @Test + public void shouldUseNextUriFor_NoExceptionNotOK_And_ALWAYS_Strategy() throws Exception { + // At the time of this writing, TEMPORARY_REDIRECT will not cause an exception to + // be thrown back to + // getRemoteEnvironment, but since status is not OK, the method will treat it the + // same as an exception and + // thus try the next URL. + assertNextUriIsTried(ConfigClientProperties.MultipleUriStrategy.ALWAYS, HttpStatus.TEMPORARY_REDIRECT); + } + @Test public void shouldUseNextUriFor_TimeOut_And_ALWAYS_Strategy() throws Exception { assertNextUriIsTriedOnTimeout(ConfigClientProperties.MultipleUriStrategy.ALWAYS); @@ -459,6 +515,12 @@ public class ConfigServicePropertySourceLocatorTests { private void assertNextUriIsNotTried(ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse, Class expectedCause) { + assertNextUriIsNotTried(true, multipleUriStrategy, firstUriResponse, expectedCause); + } + + private void assertNextUriIsNotTried(boolean failFast, + ConfigClientProperties.MultipleUriStrategy multipleUriStrategy, HttpStatus firstUriResponse, + Class expectedCause) { AbstractThrowableAssert throwableAssert = Assertions.assertThatThrownBy(() -> { // Set up with two URIs. ConfigClientProperties clientProperties = new ConfigClientProperties(this.environment); @@ -466,7 +528,7 @@ public class ConfigServicePropertySourceLocatorTests { String goodURI = "http://localhost:8888"; String[] uris = new String[] { badURI, goodURI }; clientProperties.setUri(uris); - clientProperties.setFailFast(true); + clientProperties.setFailFast(failFast); // Strategy is CONNECTION_TIMEOUT_ONLY, so it should not try the next URI for // INTERNAL_SERVER_ERROR clientProperties.setMultipleUriStrategy(multipleUriStrategy);