fix: harden push version negotiation and add missing test coverage

Alfonso Sastre · cursoragent · Alfonso Sastre · commit 7530fb20dca2 · 2026-02-25T14:15:42.000+01:00
- Guard missing "url" key in _pick_version_details_url so malformed
  version entries are skipped instead of raising KeyError
- Clarify docstring: fallback may select a version newer than requested
- Add response.raise_for_status() after both HTTP fetches in push_object
  so 4xx/5xx responses raise httpx.HTTPStatusError instead of silently
  failing with a KeyError on response.json()["data"]
- Add raise_for_status() to MockResponse in async_client.py (required
  after wiring raise_for_status into push_object); also add httpx/
  MagicMock imports and replace bare next() with next(..., None) +
  assertion for a clearer failure message
- Add 11 new tests covering: _pick_version_details_url (exact match,
  fallback, no mutual version, empty list, missing url/version keys),
  push_object version negotiation two-GET flow, no-mutual-version
  ValueError, and HTTP error propagation on both GET requests

Co-authored-by: Cursor &lt;cursoragent@cursor.com&gt;
diff --git a/ocpi/core/push.py b/ocpi/core/push.py
@@ -25,9 +25,17 @@ def _pick_version_details_url(
     """Pick the best version details URL from an OCPI /versions response.
 
     Tries the requested version first, then falls back to the highest
-    mutually supported version.
+    mutually supported version from _VERSION_PREFERENCE. The fallback may
+    select a version newer than ``requested`` when the receiver does not
+    support the requested version but does support a higher one.
+
+    Entries missing either the ``version`` or ``url`` key are silently skipped.
     """
-    by_version = {v["version"]: v["url"] for v in versions_list if "version" in v}
+    by_version = {
+        v["version"]: v["url"]
+        for v in versions_list
+        if "version" in v and "url" in v
+    }
 
     if requested.value in by_version:
         return by_version[requested.value]
@@ -130,6 +138,7 @@ async def push_object(
                 headers={"authorization": client_auth_token},
             )
             logger.info(f"Response status_code - `{response.status_code}`")
+            response.raise_for_status()
             response_data = response.json()["data"]
 
             # If response is a versions list, negotiate version and
@@ -154,6 +163,7 @@ async def push_object(
                 logger.info(
                     f"Version details response: {response.status_code}"
                 )
+                response.raise_for_status()
                 response_data = response.json()["data"]
 
             endpoints = response_data["endpoints"]
diff --git a/tests/test_core/test_push.py b/tests/test_core/test_push.py
@@ -2,12 +2,14 @@
 
 from unittest.mock import AsyncMock, MagicMock, patch
 
+import httpx
 import pytest
 
 from ocpi.core import enums, schemas
 from ocpi.core.adapter import BaseAdapter
 from ocpi.core.crud import Crud
 from ocpi.core.push import (
+    _pick_version_details_url,
     client_method,
     client_url,
     push_object,
@@ -533,3 +535,270 @@ async def test_push_object_multiple_receivers():
     # Should have responses for both receivers
     assert len(result.receiver_responses) == 2
     assert mock_client.return_value.__aenter__.return_value.get.await_count == 2
+
+
+# ---------------------------------------------------------------------------
+# _pick_version_details_url unit tests
+# ---------------------------------------------------------------------------
+
+
+def test_pick_version_details_url_exact_match():
+    """Returns the URL for the exact requested version."""
+    versions_list = [
+        {"version": "2.1.1", "url": "https://example.com/ocpi/2.1.1/details"},
+        {"version": "2.2.1", "url": "https://example.com/ocpi/2.2.1/details"},
+        {"version": "2.3.0", "url": "https://example.com/ocpi/2.3.0/details"},
+    ]
+    result = _pick_version_details_url(versions_list, VersionNumber.v_2_2_1)
+    assert result == "https://example.com/ocpi/2.2.1/details"
+
+
+def test_pick_version_details_url_fallback_to_highest():
+    """Falls back to the highest available version when requested is absent."""
+    versions_list = [
+        {"version": "2.1.1", "url": "https://example.com/ocpi/2.1.1/details"},
+        {"version": "2.3.0", "url": "https://example.com/ocpi/2.3.0/details"},
+    ]
+    result = _pick_version_details_url(versions_list, VersionNumber.v_2_2_1)
+    assert result == "https://example.com/ocpi/2.3.0/details"
+
+
+def test_pick_version_details_url_no_mutual_version():
+    """Returns None when no known version is present in the list."""
+    versions_list = [{"version": "1.0", "url": "https://example.com/ocpi/1.0/details"}]
+    result = _pick_version_details_url(versions_list, VersionNumber.v_2_2_1)
+    assert result is None
+
+
+def test_pick_version_details_url_empty_list():
+    """Returns None for an empty versions list."""
+    result = _pick_version_details_url([], VersionNumber.v_2_2_1)
+    assert result is None
+
+
+def test_pick_version_details_url_missing_url_key():
+    """Entries without a 'url' key are skipped without raising KeyError."""
+    versions_list = [
+        {"version": "2.2.1"},  # no "url" — must not raise
+        {"version": "2.3.0", "url": "https://example.com/ocpi/2.3.0/details"},
+    ]
+    result = _pick_version_details_url(versions_list, VersionNumber.v_2_2_1)
+    # 2.2.1 entry is skipped; falls back to 2.3.0
+    assert result == "https://example.com/ocpi/2.3.0/details"
+
+
+def test_pick_version_details_url_missing_version_key():
+    """Entries without a 'version' key are skipped without raising KeyError."""
+    versions_list = [
+        {"url": "https://example.com/ocpi/unknown/details"},  # no "version"
+        {"version": "2.2.1", "url": "https://example.com/ocpi/2.2.1/details"},
+    ]
+    result = _pick_version_details_url(versions_list, VersionNumber.v_2_2_1)
+    assert result == "https://example.com/ocpi/2.2.1/details"
+
+
+# ---------------------------------------------------------------------------
+# push_object — version negotiation path
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_push_object_version_negotiation_via_versions_list():
+    """push_object performs a second GET to fetch version details when the
+    first response returns an OCPI versions list instead of version details."""
+    mock_crud = AsyncMock(spec=MockCrud)
+    mock_crud.get.return_value = {"id": "loc-123"}
+
+    mock_adapter = MagicMock(spec=BaseAdapter)
+    mock_adapter.location_adapter.return_value.model_dump.return_value = {"id": "loc-123"}
+
+    push = schemas.Push(
+        module_id=enums.ModuleID.locations,
+        object_id="loc-123",
+        receivers=[
+            schemas.Receiver(
+                endpoints_url="https://example.com/versions", auth_token="token"
+            ),
+        ],
+    )
+
+    mock_versions_response = MagicMock()
+    mock_versions_response.status_code = 200
+    mock_versions_response.raise_for_status = MagicMock()
+    mock_versions_response.json.return_value = {
+        "data": [
+            {"version": "2.2.1", "url": "https://example.com/ocpi/2.2.1/details"},
+        ]
+    }
+
+    mock_details_response = MagicMock()
+    mock_details_response.status_code = 200
+    mock_details_response.raise_for_status = MagicMock()
+    mock_details_response.json.return_value = {
+        "data": {
+            "endpoints": [
+                {
+                    "identifier": enums.ModuleID.locations,
+                    "role": InterfaceRole.receiver,
+                    "url": "https://example.com/locations",
+                }
+            ]
+        }
+    }
+
+    mock_push_response = MagicMock()
+    mock_push_response.status_code = 200
+    mock_push_response.json.return_value = {"status_code": 1000}
+
+    with patch("ocpi.core.push.httpx.AsyncClient") as mock_client:
+        mock_client.return_value.__aenter__.return_value.get = AsyncMock(
+            side_effect=[mock_versions_response, mock_details_response]
+        )
+        mock_client.return_value.__aenter__.return_value.send = AsyncMock(
+            return_value=mock_push_response
+        )
+        mock_client.return_value.__aenter__.return_value.build_request = MagicMock()
+
+        result = await push_object(
+            version=VersionNumber.v_2_2_1,
+            push=push,
+            crud=mock_crud,
+            adapter=mock_adapter,
+            auth_token="auth-token",
+        )
+
+    assert len(result.receiver_responses) == 1
+    assert result.receiver_responses[0].status_code == 200
+
+    get_calls = mock_client.return_value.__aenter__.return_value.get.call_args_list
+    assert len(get_calls) == 2
+    assert get_calls[1][0][0] == "https://example.com/ocpi/2.2.1/details"
+
+
+@pytest.mark.asyncio
+async def test_push_object_version_negotiation_no_mutual_version():
+    """push_object raises ValueError when no mutual OCPI version can be negotiated."""
+    mock_crud = AsyncMock(spec=MockCrud)
+    mock_adapter = MagicMock(spec=BaseAdapter)
+
+    push = schemas.Push(
+        module_id=enums.ModuleID.locations,
+        object_id="loc-123",
+        receivers=[
+            schemas.Receiver(
+                endpoints_url="https://example.com/versions", auth_token="token"
+            ),
+        ],
+    )
+
+    mock_versions_response = MagicMock()
+    mock_versions_response.status_code = 200
+    mock_versions_response.raise_for_status = MagicMock()
+    mock_versions_response.json.return_value = {
+        "data": [
+            {"version": "1.0", "url": "https://example.com/ocpi/1.0/details"},
+        ]
+    }
+
+    with patch("ocpi.core.push.httpx.AsyncClient") as mock_client:
+        mock_client.return_value.__aenter__.return_value.get = AsyncMock(
+            return_value=mock_versions_response
+        )
+
+        with pytest.raises(ValueError, match="No mutual OCPI version found"):
+            await push_object(
+                version=VersionNumber.v_2_2_1,
+                push=push,
+                crud=mock_crud,
+                adapter=mock_adapter,
+            )
+
+
+# ---------------------------------------------------------------------------
+# push_object — HTTP error handling
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_push_object_raises_on_non_200_endpoints_response():
+    """push_object propagates httpx.HTTPStatusError when the first GET fails."""
+    mock_crud = AsyncMock(spec=MockCrud)
+    mock_adapter = MagicMock(spec=BaseAdapter)
+
+    push = schemas.Push(
+        module_id=enums.ModuleID.locations,
+        object_id="loc-123",
+        receivers=[
+            schemas.Receiver(
+                endpoints_url="https://example.com/versions", auth_token="token"
+            ),
+        ],
+    )
+
+    mock_error_response = MagicMock()
+    mock_error_response.status_code = 503
+    mock_error_response.raise_for_status.side_effect = httpx.HTTPStatusError(
+        "Service Unavailable",
+        request=MagicMock(),
+        response=mock_error_response,
+    )
+
+    with patch("ocpi.core.push.httpx.AsyncClient") as mock_client:
+        mock_client.return_value.__aenter__.return_value.get = AsyncMock(
+            return_value=mock_error_response
+        )
+
+        with pytest.raises(httpx.HTTPStatusError):
+            await push_object(
+                version=VersionNumber.v_2_2_1,
+                push=push,
+                crud=mock_crud,
+                adapter=mock_adapter,
+            )
+
+
+@pytest.mark.asyncio
+async def test_push_object_raises_on_non_200_version_details_response():
+    """push_object propagates httpx.HTTPStatusError when the version details GET fails."""
+    mock_crud = AsyncMock(spec=MockCrud)
+    mock_adapter = MagicMock(spec=BaseAdapter)
+
+    push = schemas.Push(
+        module_id=enums.ModuleID.locations,
+        object_id="loc-123",
+        receivers=[
+            schemas.Receiver(
+                endpoints_url="https://example.com/versions", auth_token="token"
+            ),
+        ],
+    )
+
+    mock_versions_response = MagicMock()
+    mock_versions_response.status_code = 200
+    mock_versions_response.raise_for_status = MagicMock()
+    mock_versions_response.json.return_value = {
+        "data": [
+            {"version": "2.2.1", "url": "https://example.com/ocpi/2.2.1/details"},
+        ]
+    }
+
+    mock_error_response = MagicMock()
+    mock_error_response.status_code = 404
+    mock_error_response.raise_for_status.side_effect = httpx.HTTPStatusError(
+        "Not Found",
+        request=MagicMock(),
+        response=mock_error_response,
+    )
+
+    with patch("ocpi.core.push.httpx.AsyncClient") as mock_client:
+        mock_client.return_value.__aenter__.return_value.get = AsyncMock(
+            side_effect=[mock_versions_response, mock_error_response]
+        )
+
+        with pytest.raises(httpx.HTTPStatusError):
+            await push_object(
+                version=VersionNumber.v_2_2_1,
+                push=push,
+                crud=mock_crud,
+                adapter=mock_adapter,
+            )
diff --git a/tests/test_modules/mocks/async_client.py b/tests/test_modules/mocks/async_client.py
@@ -1,12 +1,18 @@
+from unittest.mock import MagicMock
+
+import httpx
+
 from ocpi.core.dependencies import get_versions
 from ocpi.core.endpoints import ENDPOINTS
 from ocpi.core.enums import ModuleID, RoleEnum
 from ocpi.modules.versions.enums import VersionNumber
 from ocpi.modules.versions.v_2_2_1.schemas import VersionDetail
 
 _locations_endpoint = next(
-    e for e in ENDPOINTS[VersionNumber.v_2_2_1][RoleEnum.cpo] if e.identifier == ModuleID.locations
+    (e for e in ENDPOINTS[VersionNumber.v_2_2_1][RoleEnum.cpo] if e.identifier == ModuleID.locations),
+    None,
 )
+assert _locations_endpoint is not None, "locations endpoint missing from CPO 2.2.1 ENDPOINTS_LIST"
 
 fake_endpoints_data = {
     "data": VersionDetail(
@@ -26,6 +32,14 @@ def __init__(self, json_data, status_code):
     def json(self):
         return self.json_data
 
+    def raise_for_status(self):
+        if self.status_code >= 400:
+            raise httpx.HTTPStatusError(
+                f"HTTP {self.status_code}",
+                request=MagicMock(),
+                response=self,  # type: ignore[arg-type]
+            )
+
 
 # Connector mocks
 
