Next URL is not attempted if not successful, but exception is not thrown

[marnee01]

Describe the bug
See this code in ConfigServicePropertySourceLocator.getRemoteEnvironment method:

			if (response == null || response.getStatusCode() != HttpStatus.OK) {
				return null;
			}

We'd like to have this updated so that when this condition is met, it tries the next URL (if any).
We'd also like to have this log the status code at ERROR level when response is not null. And if response is null, log that fact at ERROR level.

Sample
It's difficult to provide a working example. We have a strange situation with our environment that is causing this condition to evaluate to true, but it is related to an Nginx issue, and I would not know how to replicate this in a standalone project.

[ryanjbaxter]

Just so I am clear you woud like something like this?

if (response == null || response.getStatusCode() != HttpStatus.OK) {
				if(response == null) {
					logger.warn("The response from " + uri + " was null, trying the next uri if available");
				}
				if (response.getStatusCode() != HttpStatus.OK) {
					logger.warn("The response from " + uri + " had a status code of " + response.getStatusCode() ". Trying the next uri if available");
				}
				continue;
			}

I don't think we need to log it as an error, I think this should be a warning.

[marnee01]

Basically, yes. Maybe it would be good to also have the same log messages that reports that it's trying the next URL, like it does in the catch blocks, for consistency.

As far as error vs warning, I'm a little unclear why there would be a null response or a non-OK response that doesn't generate one of the caught exceptions. I observed this happening, but due to the lack of logging, I am not sure what the response looked like. I couldn't reproduce it locally, so I couldn't step through the running code. If you think this is not an error condition, then a warning seems fine.

[ryanjbaxter]

So when looking at making these changes, you actually wrote a test that breaks this assumption and verifies that when that status code is not 200 that it does not try the next uri
shouldNotUseNextUriFor_404_And_CONNECTION_TIMEOUT_ONLY_Strategy

As far as an error or warning, I can see it go both ways. In the case where there is another uri to try it feels like a warning, but if there is no other uri to try it seems like it could be an error.

[marnee01]

That is for the case when the strategy is ConfigClientProperties.MultipleUriStrategy.CONNECTION_TIMEOUT_ONLY, not for strategy=ALWAYS.

I think this was a miss on my part. I don't know what sort of HTTP response code would result in this condition or why response would ever be null.

If null is returned from this method, IllegalArgumentException (line 178 in ConfigServicePropertySourceLocator.locate(...) method) is eventually thrown, so it doesn't seem like there is some case where this would be a happy path scenario. As such, it seems trying the next URL (when strategy is ALWAYS) would be desirable.

Here is kind of what I'm thinking now (starting with the for loop):

		for (int i = 0; i < noOfUrls; i++) {
			Credentials credentials = properties.getCredentials(i);
			String uri = credentials.getUri();
			String username = credentials.getUsername();
			String password = credentials.getPassword();

			logger.info("Fetching config from server at : " + uri);

			try {
				HttpHeaders headers = new HttpHeaders();
				headers.setAccept(acceptHeader);
				headers.setAcceptCharset(acceptCharsetHeader);
				requestTemplateFactory.addAuthorizationToken(headers, username, password);
				if (StringUtils.hasText(token)) {
					headers.add(TOKEN_HEADER, token);
				}
				if (StringUtils.hasText(state) && properties.isSendState()) {
					headers.add(STATE_HEADER, state);
				}

				final HttpEntity<Void> entity = new HttpEntity<>((Void) null, headers);
				response = restTemplate.exchange(uri + path, HttpMethod.GET, entity, Environment.class, args);
				
				if ((response == null || response.getStatusCode() != HttpStatus.OK) 
					&& i < noOfUrls - 1 
					&& defaultProperties.getMultipleUriStrategy() == MultipleUriStrategy.ALWAYS)
				{
					// TODO: Error or warn?
					logger.warn("Failed to fetch configs from server at  : " + uri
							+ ". Will try the next url if available. Response : " + response == null ? "null" : response.getstatusCode());
					continue;
				}
			}
			catch (HttpClientErrorException | HttpServerErrorException e) {
				if (i < noOfUrls - 1 && defaultProperties.getMultipleUriStrategy() == MultipleUriStrategy.ALWAYS) {
					logger.info("Failed to fetch configs from server at  : " + uri
							+ ". Will try the next url if available. Error : " + e.getMessage());
					continue;
				}

				if (e.getStatusCode() != HttpStatus.NOT_FOUND) {
					throw e;
				}
			}
			catch (ResourceAccessException e) {
				logger.info("Exception on Url - " + uri + ":" + e + ". Will be trying the next url if available");
				if (i == noOfUrls - 1) {
					throw e;
				}
				else {
					continue;
				}
			}

			if (response == null || response.getStatusCode() != HttpStatus.OK) {
				// TODO: Error or warn?
				logger.warn("Failed to fetch configs from server at  : " + uri 
					+ ".  Response : response == null ? "null" : response.getstatusCode());
				return null;
			}

			Environment result = response.getBody();
			return result;
		}

		return null;
	}

[ryanjbaxter]

Can you create a PR with your proposed changes so its easier to review?

[marnee01]

I can, but I wasn't planning to do the full merge-ready change. Is this just for preliminary review of the diff of this piece of code without copying/pasting, and then you or one of the main developers would test, polish, finalize, and determine if it has to go in that other related class, also? If you're wanting me to do the whole thing, it will be a couple of days or couple of weeks before I could get to it.