Log warning if Consul failover interceptor is set multiple times (#308)

* Log warning if Consul failover interceptor is set multiple times

* Log a warning in Consul if both withMultipleHostAndPort and
  withFailoverInterceptor are called when building a Consul
  instance.
* This is important because calling them both results in a
  "last one wins" scenario, since the last method called
  sets the failover interceptor.

Closes #306

* Update javadocs regarding overriding failover interceptors

src/main/java/org/kiwiproject/consul/Consul.java

@@ -25,6 +25,8 @@
 import org.kiwiproject.consul.util.bookend.ConsulBookendInterceptor;
 import org.kiwiproject.consul.util.failover.ConsulFailoverInterceptor;
 import org.kiwiproject.consul.util.failover.strategy.ConsulFailoverStrategy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import retrofit2.Retrofit;
 import retrofit2.converter.jackson.JacksonConverterFactory;
 
@@ -49,6 +51,8 @@
 */
 public class Consul {
 
+    private static final Logger LOG = LoggerFactory.getLogger(Consul.class);
+
     /**
     * Default Consul HTTP API host.
     */
@@ -318,6 +322,7 @@ public static class Builder {
         private Interceptor headerInterceptor;
         private Interceptor consulBookendInterceptor;
         private Interceptor consulFailoverInterceptor;
+        private int numTimesConsulFailoverInterceptorSet;
         private final NetworkTimeoutConfig.Builder networkTimeoutConfigBuilder = new NetworkTimeoutConfig.Builder();
         private ExecutorService executorService;
         private ConnectionPool connectionPool;
@@ -500,6 +505,11 @@ public Builder withHostAndPort(HostAndPort hostAndPort) {
          * Sets the list of hosts to contact if the current request target is
          * unavailable. When the call to a particular URL fails for any reason, the next {@link HostAndPort} specified
          * is used to retry the request. This will continue until all urls are exhausted.
+         * <p>
+         * Internally, this method constructs a {@link ConsulFailoverInterceptor} with a
+         * {@link org.kiwiproject.consul.util.failover.strategy.BlacklistingConsulFailoverStrategy BlacklistingConsulFailoverStrategy}.
+         * If you call this method, you should not use {@link #withFailoverInterceptor(ConsulFailoverStrategy)}
+         * as it will create a new failover interceptor which overrides the one set by this method.
          *
          * @param hostAndPort           A collection of {@link HostAndPort} that define the list of Consul agent addresses to use.
          * @param blacklistTimeInMillis The timeout (in milliseconds) to blacklist a particular {@link HostAndPort} before trying to use it again.
@@ -508,25 +518,47 @@ public Builder withHostAndPort(HostAndPort hostAndPort) {
         public Builder withMultipleHostAndPort(Collection<HostAndPort> hostAndPort, long blacklistTimeInMillis) {
             checkArgument(blacklistTimeInMillis >= 0, "Blacklist time must be positive");
             checkArgument(hostAndPort.size() >= 2, "Minimum of 2 addresses are required");
+            logWarningIfConsulFailoverInterceptorAlreadySet("withMultipleHostAndPort");
 
             consulFailoverInterceptor = new ConsulFailoverInterceptor(hostAndPort, blacklistTimeInMillis);
+            ++numTimesConsulFailoverInterceptorSet;
             withHostAndPort(hostAndPort.stream().findFirst().orElseThrow());
 
             return this;
         }
 
         /**
          * Constructs a failover interceptor with the given {@link ConsulFailoverStrategy}.
+         * <p>
+         * If you call this method, you should not use {@link #withMultipleHostAndPort(Collection, long)}
+         * as it will create a new failover interceptor which overrides the one set by this method.
+         *
          * @param strategy The strategy to use.
          * @return The builder.
          */
         public Builder withFailoverInterceptor(ConsulFailoverStrategy strategy) {
             checkArgument(nonNull(strategy), "Must not provide a null strategy");
+            logWarningIfConsulFailoverInterceptorAlreadySet("withFailoverInterceptor");
 
             consulFailoverInterceptor = new ConsulFailoverInterceptor(strategy);
+            ++numTimesConsulFailoverInterceptorSet;
             return this;
         }
 
+        private void logWarningIfConsulFailoverInterceptorAlreadySet(String methodName) {
+            if (numTimesConsulFailoverInterceptorSet > 0) {
+                LOG.warn("A ConsulFailoverInterceptor was already present; this invocation to '{}' overrides it!" +
+                                " Make sure either 'withMultipleHostAndPort' or 'withFailoverInterceptor' is called," +
+                                " but not both.",
+                        methodName);
+            }
+        }
+
+        @VisibleForTesting
+        int numTimesConsulFailoverInterceptorSet() {
+            return numTimesConsulFailoverInterceptorSet;
+        }
+
         /**
         * Sets the URL from a string.
         *

src/test/java/org/kiwiproject/consul/ConsulTest.java

@@ -1,7 +1,9 @@
 package org.kiwiproject.consul;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
+import static org.mockito.Mockito.mock;
 
 import com.google.common.net.HostAndPort;
 import org.junit.jupiter.api.DisplayName;
@@ -11,6 +13,7 @@
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 import org.junit.jupiter.params.provider.ValueSource;
+import org.kiwiproject.consul.util.failover.strategy.ConsulFailoverStrategy;
 
 import java.util.Collection;
 import java.util.List;
@@ -59,4 +62,34 @@ void shouldRequireFailoverStrategy() {
                     .withMessage("Must not provide a null strategy");
         }
     }
+
+    @Nested
+    class WithMultipleFailoverInterceptors {
+
+        @Test
+        void shouldDetectWhenConsulInterceptorAlreadySetBy_withMultipleHostAndPort() {
+            var hosts = List.of(
+                    HostAndPort.fromString("consul1.acme.com:8500"),
+                    HostAndPort.fromString("consul2.acme.com:8500")
+            );
+            var consulBuilder = Consul.builder()
+                    .withMultipleHostAndPort(hosts, 10_000)
+                    .withFailoverInterceptor(mock(ConsulFailoverStrategy.class));
+
+            assertThat(consulBuilder.numTimesConsulFailoverInterceptorSet()).isEqualTo(2);
+        }
+
+        @Test
+        void shouldDetectWhenConsulInterceptorAlreadySetBy_withFailoverInterceptor() {
+            var hosts = List.of(
+                    HostAndPort.fromString("consul1.acme.com:8500"),
+                    HostAndPort.fromString("consul2.acme.com:8500")
+            );
+            var consulBuilder = Consul.builder()
+                    .withFailoverInterceptor(mock(ConsulFailoverStrategy.class))
+                    .withMultipleHostAndPort(hosts, 7_500);
+
+            assertThat(consulBuilder.numTimesConsulFailoverInterceptorSet()).isEqualTo(2);
+        }
+    }
 }