fix: Wait for the database in the test container through the v2 API, and through the v1 API only where the server has no v2 API.

eventsourcingdb/container.py

@@ -16,6 +16,7 @@
 from docker.models.containers import Container as DockerContainer
 
 from .client import Client
+from .transport.statuses_without_v2_api import STATUSES_WITHOUT_V2_API
 
 logger = logging.getLogger(__name__)
 
@@ -36,6 +37,9 @@
 PING_TIMEOUT_SECONDS = 1
 POLL_INTERVAL_SECONDS = 0.1
 
+V1_PING_PATH = "/api/v1/ping"
+V2_PING_PATH = "/api/v2/system/ping"
+
 
 def _format_seconds(seconds: float) -> str:
     # Whole seconds without a fraction, others as short as possible, such as 1.5.
@@ -328,23 +332,44 @@ def _wait_until_reachable(self, container: DockerContainer, deadline: float) ->
                 return None
 
             mapped_port = self._extract_port_from_container_info(container.attrs)
-            if mapped_port is not None and self._check_endpoint_available(
-                f"http://{self._host}:{mapped_port}/api/v1/ping",
-                min(remaining, PING_TIMEOUT_SECONDS),
+            if mapped_port is not None and self._is_database_ready(
+                f"http://{self._host}:{mapped_port}", deadline
             ):
                 return mapped_port
 
             time.sleep(max(min(POLL_INTERVAL_SECONDS, deadline - time.monotonic()), 0))
 
-    def _check_endpoint_available(self, url: str, timeout: float) -> bool:
+    def _is_database_ready(self, base_url: str, deadline: float) -> bool:
+        # The v1 API is asked only where the server lacks the v2 API, since a
+        # later release of EventSourcingDB removes it. Any other failure means
+        # that the server is not ready yet, so the next attempt asks the v2 API
+        # again, rather than remembering an answer of a server that may still
+        # be starting. The statuses that tell a server without the v2 API are
+        # shared with the client, but not the way the client chooses its API,
+        # so that the readiness of the container that tests the client does
+        # not depend on what those tests are meant to check.
+        status = self._ping(f"{base_url}{V2_PING_PATH}", deadline)
+        if status == HTTPStatus.OK:
+            return True
+        if status not in STATUSES_WITHOUT_V2_API:
+            return False
+
+        return self._ping(f"{base_url}{V1_PING_PATH}", deadline) == HTTPStatus.OK
+
+    def _ping(self, url: str, deadline: float) -> int | None:
+        remaining = deadline - time.monotonic()
+        if remaining <= 0:
+            return None
+
+        # Bandit sees that a request has a bound only if it is passed by name.
+        timeout = min(remaining, PING_TIMEOUT_SECONDS)
+
+        # The ping needs no API token, so it gets none.
         try:
             response = requests.get(url, timeout=timeout)
         except requests.RequestException:
-            return False
-        return self._check_response_ok(response)
-
-    def _check_response_ok(self, response: requests.Response) -> bool:
-        return response is not None and response.status_code == HTTPStatus.OK
+            return None
+        return response.status_code
 
     def with_api_token(self, token: str) -> "Container":
         self._api_token = token

tests/test_container.py

@@ -12,7 +12,11 @@
 from docker.models.images import ImageCollection
 
 from eventsourcingdb import Container
-from eventsourcingdb.container import DEFAULT_STARTUP_TIMEOUT_SECONDS, START_LABEL
+from eventsourcingdb.container import (
+    DEFAULT_STARTUP_TIMEOUT_SECONDS,
+    PING_TIMEOUT_SECONDS,
+    START_LABEL,
+)
 
 from .shared.image_tag import get_image_tag
 
@@ -21,6 +25,9 @@
 FAKE_PORT = 49153
 POLL_GUARD_SECONDS = 2
 
+V1_PING_URL = f'http://localhost:{FAKE_PORT}/api/v1/ping'
+V2_PING_URL = f'http://localhost:{FAKE_PORT}/api/v2/system/ping'
+
 
 class CreatedContainers:
     """
@@ -116,17 +123,31 @@ def reload(self) -> None:
 
 class FakePing:
     """
-    Answers the pings with the given statuses in turn, and with the last one
-    from then on.
+    Answers the pings of each URL with the given answers in turn, and with the
+    last one from then on. An answer is a status, or an error that the ping
+    raises instead, such as when the connection is refused or the ping hits its
+    bound. A URL without answers is answered with 404, as a server answers a
+    path that it does not know.
     """
 
-    def __init__(self, statuses: list[int]) -> None:
-        self.statuses = statuses
+    def __init__(self, answers: dict[str, list[int | requests.RequestException]]) -> None:
+        self.answers = answers
         self.urls: list[str] = []
+        self.timeouts: list[float | None] = []
 
-    def __call__(self, url: str, timeout: float) -> SimpleNamespace:
+    def __call__(self, url: str, timeout: float | None = None) -> SimpleNamespace:
         self.urls.append(url)
-        return SimpleNamespace(status_code=self.statuses[min(len(self.urls), len(self.statuses)) - 1])
+        self.timeouts.append(timeout)
+
+        # As requests does, which refuses a bound that leaves no time at all.
+        if timeout is not None and timeout <= 0:
+            raise ValueError(f'The ping must have time left, but its bound is {timeout}.')
+
+        answers = self.answers.get(url, [404])
+        answer = answers[min(self.urls.count(url), len(answers)) - 1]
+        if isinstance(answer, requests.RequestException):
+            raise answer
+        return SimpleNamespace(status_code=answer)
 
 
 def listen_on_another_port(command: list[str]) -> list[str]:
@@ -205,6 +226,136 @@ async def test_leaves_containers_alone_that_it_did_not_start() -> None:
             other_container.remove(force=True)
 
 
+class TestWaitingForTheDatabase:
+    """
+    Every attempt asks the ping of the v2 API, and the ping of the v1 API only
+    if the server answers that with 404 or 405, as a server without the v2 API
+    does, so that the container becomes ready against servers with either API.
+    """
+
+    @staticmethod
+    def test_is_ready_once_the_v2_api_answers_its_ping_without_asking_the_v1_api(
+        monkeypatch: pytest.MonkeyPatch,
+    ) -> None:
+        ping = FakePing({V2_PING_URL: [200], V1_PING_URL: [200]})
+        monkeypatch.setattr(requests, 'get', ping)
+
+        mapped_port = Container()._wait_until_reachable(
+            FakeContainer(published_from_poll=1), time.monotonic() + POLL_GUARD_SECONDS
+        )
+
+        assert mapped_port == FAKE_PORT
+        assert ping.urls == [V2_PING_URL]
+
+    @staticmethod
+    def test_is_ready_once_the_v2_api_answers_its_ping_if_the_server_lacks_the_v1_api(
+        monkeypatch: pytest.MonkeyPatch,
+    ) -> None:
+        ping = FakePing({V2_PING_URL: [200], V1_PING_URL: [404]})
+        monkeypatch.setattr(requests, 'get', ping)
+
+        mapped_port = Container()._wait_until_reachable(
+            FakeContainer(published_from_poll=1), time.monotonic() + POLL_GUARD_SECONDS
+        )
+
+        assert mapped_port == FAKE_PORT
+        assert ping.urls == [V2_PING_URL]
+
+    @staticmethod
+    @pytest.mark.parametrize('status', [404, 405])
+    def test_asks_the_v1_api_within_the_same_attempt_if_the_server_lacks_the_v2_api(
+        status: int, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        ping = FakePing({V2_PING_URL: [status], V1_PING_URL: [200]})
+        monkeypatch.setattr(requests, 'get', ping)
+        fake_container = FakeContainer(published_from_poll=1)
+
+        mapped_port = Container()._wait_until_reachable(
+            fake_container, time.monotonic() + POLL_GUARD_SECONDS
+        )
+
+        assert mapped_port == FAKE_PORT
+        assert ping.urls == [V2_PING_URL, V1_PING_URL]
+        assert fake_container.polls == 1
+        assert all(
+            timeout is not None and 0 < timeout <= PING_TIMEOUT_SECONDS
+            for timeout in ping.timeouts
+        ), f'Every ping must be bounded, but the bounds were {ping.timeouts}.'
+
+    @staticmethod
+    @pytest.mark.parametrize(
+        'failure',
+        [
+            pytest.param(503, id='another status'),
+            pytest.param(requests.exceptions.ReadTimeout('Read timed out.'), id='bound per request'),
+            pytest.param(requests.exceptions.ConnectionError('Connection refused.'), id='refused'),
+        ],
+    )
+    def test_tries_again_without_asking_the_v1_api_if_the_ping_of_the_v2_api_fails_otherwise(
+        failure: int | requests.RequestException, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        ping = FakePing({V2_PING_URL: [failure, 200], V1_PING_URL: [200]})
+        monkeypatch.setattr(requests, 'get', ping)
+        fake_container = FakeContainer(published_from_poll=1)
+
+        mapped_port = Container()._wait_until_reachable(
+            fake_container, time.monotonic() + POLL_GUARD_SECONDS
+        )
+
+        assert mapped_port == FAKE_PORT
+        assert ping.urls == [V2_PING_URL, V2_PING_URL]
+        assert fake_container.polls == 2
+
+    @staticmethod
+    @pytest.mark.parametrize(
+        'failure',
+        [
+            pytest.param(204, id='another success status'),
+            pytest.param(503, id='another status'),
+            pytest.param(requests.exceptions.ReadTimeout('Read timed out.'), id='bound per request'),
+        ],
+    )
+    def test_starts_the_next_attempt_with_the_v2_api_if_the_ping_of_the_v1_api_fails(
+        failure: int | requests.RequestException, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        ping = FakePing({V2_PING_URL: [404, 200], V1_PING_URL: [failure]})
+        monkeypatch.setattr(requests, 'get', ping)
+        fake_container = FakeContainer(published_from_poll=1)
+
+        mapped_port = Container()._wait_until_reachable(
+            fake_container, time.monotonic() + POLL_GUARD_SECONDS
+        )
+
+        assert mapped_port == FAKE_PORT
+        assert ping.urls == [V2_PING_URL, V1_PING_URL, V2_PING_URL]
+        assert fake_container.polls == 2
+
+    @staticmethod
+    def test_does_not_ask_the_v1_api_once_the_ping_of_the_v2_api_used_up_the_time(
+        monkeypatch: pytest.MonkeyPatch,
+    ) -> None:
+        ping = FakePing({V2_PING_URL: [404], V1_PING_URL: [200]})
+
+        # Less time than a single ping may take, so that the time left bounds
+        # the ping rather than the bound of every ping.
+        time_left = PING_TIMEOUT_SECONDS / 2
+        deadline = time.monotonic() + time_left
+
+        def answer_once_the_time_is_up(url: str, timeout: float | None = None) -> SimpleNamespace:
+            time.sleep(max(deadline - time.monotonic(), 0))
+            return ping(url, timeout)
+
+        monkeypatch.setattr(requests, 'get', answer_once_the_time_is_up)
+
+        mapped_port = Container()._wait_until_reachable(FakeContainer(published_from_poll=1), deadline)
+
+        assert mapped_port is None
+        assert ping.urls == [V2_PING_URL]
+        assert ping.timeouts[0] is not None and 0 < ping.timeouts[0] <= time_left, (
+            f'The ping must be bounded by the time left, {time_left}s, but was bounded by {ping.timeouts[0]}.'
+        )
+
+
 class TestStartupTimeout:
     @staticmethod
     def test_is_30_seconds_by_default() -> None:
@@ -230,7 +381,7 @@ def test_rejects_a_startup_timeout_that_is_not_a_finite_number_greater_than_zero
 
     @staticmethod
     def test_reads_the_port_again_on_every_poll(monkeypatch: pytest.MonkeyPatch) -> None:
-        ping = FakePing([200])
+        ping = FakePing({V2_PING_URL: [200]})
         monkeypatch.setattr(requests, 'get', ping)
         fake_container = FakeContainer(published_from_poll=3)
 
@@ -240,11 +391,11 @@ def test_reads_the_port_again_on_every_poll(monkeypatch: pytest.MonkeyPatch) ->
 
         assert mapped_port == FAKE_PORT
         assert fake_container.polls == 3
-        assert ping.urls == [f'http://localhost:{FAKE_PORT}/api/v1/ping']
+        assert ping.urls == [V2_PING_URL]
 
     @staticmethod
     def test_accepts_only_status_200_from_the_ping(monkeypatch: pytest.MonkeyPatch) -> None:
-        ping = FakePing([204, 500, 200])
+        ping = FakePing({V2_PING_URL: [204, 500, 200]})
         monkeypatch.setattr(requests, 'get', ping)
         fake_container = FakeContainer(published_from_poll=1)
 
@@ -253,7 +404,7 @@ def test_accepts_only_status_200_from_the_ping(monkeypatch: pytest.MonkeyPatch)
         )
 
         assert mapped_port == FAKE_PORT
-        assert len(ping.urls) == 3
+        assert ping.urls == [V2_PING_URL] * 3
 
     @staticmethod
     def test_replaces_a_running_container_that_does_not_become_reachable_in_time(