From e8ed77cb89836e86835ea87b9d03366ed0d9835a Mon Sep 17 00:00:00 2001
From: David Pilato <david@pilato.fr>
Date: Mon, 24 Sep 2018 18:16:38 +0200
Subject: [PATCH] Fix HttpWaitStrategy.forStatusCodeMatching used with
 HttpWaitStrategy.forStatusCode

In #630 we introduced predicates but with a default one which is always present whatever what is passed in the `forStatusCodeMatching()` method.

This commit adds a test that demonstrates the issue:

* We have a service returning `200 OK`
* The predicate expects anything which is a code >= to `300`
* The test should throw a Timeout as this condition is never reached but without the current fix, the test never throws the Timeout as 200 matches the default builtin predicate.

This commit fixes the problem by checking at startup time what is/are the predicates that needs to be applied.

Note that in most cases, an HTTP service is expected to throw a `200 OK` status so that fix might not fix actually any real problem and might be a theory only. But I'd prefer to have code that actually implements what is supposed to work.

Closes #880.
---
 .../wait/strategy/HttpWaitStrategy.java       | 25 +++++++++++++------
 .../wait/strategy/HttpWaitStrategyTest.java   | 15 +++++++++++
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/core/src/main/java/org/testcontainers/containers/wait/strategy/HttpWaitStrategy.java b/core/src/main/java/org/testcontainers/containers/wait/strategy/HttpWaitStrategy.java
index 29f5a64b6df..6096e745e36 100644
--- a/core/src/main/java/org/testcontainers/containers/wait/strategy/HttpWaitStrategy.java
+++ b/core/src/main/java/org/testcontainers/containers/wait/strategy/HttpWaitStrategy.java
@@ -39,11 +39,7 @@ public class HttpWaitStrategy extends AbstractWaitStrategy {
     private String username;
     private String password;
     private Predicate<String> responsePredicate;
-    private Predicate<Integer> statusCodePredicate = responseCode -> {
-        // If we did not provide any status code, we assume by default HttpURLConnection.HTTP_OK
-        if (statusCodes.isEmpty() && HttpURLConnection.HTTP_OK == responseCode) return true;
-        return statusCodes.contains(responseCode);
-    };
+    private Predicate<Integer> statusCodePredicate = null;
     private Optional<Integer> livenessPort = Optional.empty();
 
     /**
@@ -63,7 +59,7 @@ public HttpWaitStrategy forStatusCode(int statusCode) {
      * @return this
      */
     public HttpWaitStrategy forStatusCodeMatching(Predicate<Integer> statusCodePredicate) {
-        this.statusCodePredicate = this.statusCodePredicate.or(statusCodePredicate);
+        this.statusCodePredicate = statusCodePredicate;
         return this;
     }
 
@@ -159,7 +155,22 @@ protected void waitUntilReady() {
 
                         log.trace("Get response code {}", connection.getResponseCode());
 
-                        if (!statusCodePredicate.test(connection.getResponseCode())) {
+                        // Choose the statusCodePredicate strategy depending on what we defined.
+                        Predicate<Integer> predicate;
+                        if (statusCodes.isEmpty() && statusCodePredicate == null) {
+                            // We have no status code and no predicate so we expect a 200 OK response code
+                            predicate = responseCode -> HttpURLConnection.HTTP_OK == responseCode;
+                        } else if (!statusCodes.isEmpty() && statusCodePredicate == null) {
+                            // We use the default status predicate checker when we only have status codes
+                            predicate = responseCode -> statusCodes.contains(responseCode);
+                        } else if (statusCodes.isEmpty()) {
+                            // We only have a predicate
+                            predicate = statusCodePredicate;
+                        } else {
+                            // We have both predicate and status code
+                            predicate = statusCodePredicate.or(responseCode -> statusCodes.contains(responseCode));
+                        }
+                        if (!predicate.test(connection.getResponseCode())) {
                             throw new RuntimeException(String.format("HTTP response code was: %s",
                                 connection.getResponseCode()));
                         }
diff --git a/core/src/test/java/org/testcontainers/junit/wait/strategy/HttpWaitStrategyTest.java b/core/src/test/java/org/testcontainers/junit/wait/strategy/HttpWaitStrategyTest.java
index 65a693f514f..5820333245a 100644
--- a/core/src/test/java/org/testcontainers/junit/wait/strategy/HttpWaitStrategyTest.java
+++ b/core/src/test/java/org/testcontainers/junit/wait/strategy/HttpWaitStrategyTest.java
@@ -84,6 +84,21 @@ public void testWaitUntilReadyWithTimeoutAndWithManyStatusCodesAndLambda() {
         ));
     }
 
+    /**
+     * Expects that the WaitStrategy throws a {@link RetryCountExceededException} after not receiving any of the
+     * error code defined with {@link HttpWaitStrategy#forStatusCode(int)}
+     * and {@link HttpWaitStrategy#forStatusCodeMatching(Predicate)}. Note that a 200 status code should not
+     * be considered as a successful return as not explicitly set.
+     * Test case for: https://github.com/testcontainers/testcontainers-java/issues/880
+     */
+    @Test
+    public void testWaitUntilReadyWithTimeoutAndWithLambdaShouldNotMatchOk() {
+        waitUntilReadyAndTimeout(startContainerWithCommand(createShellCommand("200 OK", GOOD_RESPONSE_BODY),
+            createHttpWaitStrategy(ready)
+                .forStatusCodeMatching(it -> it >= 300)
+        ));
+    }
+
     /**
      * Expects that the WaitStrategy throws a {@link RetryCountExceededException} after not receiving an HTTP 200
      * response from the container within the timeout period.