From 28f97d5f9c1e8f644e41b801331ce5e2796487fc Mon Sep 17 00:00:00 2001 From: Karl Date: Sun, 13 Sep 2026 14:54:32 -0400 Subject: [PATCH 1/3] fix(transport): accept a successful empty 204 instead of raising (#103) Both clients parsed every 2xx with a bare `response.json()`. The Rails webhooks controller answers `destroy` with `head :no_content` -- a 204 with an empty body -- so `client.webhooks.delete(...)` raised JSONDecodeError. The deletion had SUCCEEDED and the caller was told it failed, which is the worst shape of error for a mutation: the obvious recovery is to retry a delete that already worked. One shared `decode_json_body` helper now backs all three decode sites -- `client.request`, `client.request_with_headers` and `async_client.request` -- so the sync and async paths cannot drift. It decodes first and inspects the body only when that fails, which keeps the body untouched on the hot path. Deliberately narrow: - an empty or whitespace-only body on any 2xx is success, returning {}; - a MALFORMED NON-EMPTY body is still an error, never laundered into an empty success; - 401/403/429 typed errors are untouched, and a 204 is not retried. `delete` keeps its documented `-> None` contract. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- oilpriceapi/_body.py | 53 ++++++++ oilpriceapi/async_client.py | 3 +- oilpriceapi/client.py | 5 +- tests/unit/test_empty_204_response.py | 171 ++++++++++++++++++++++++++ 4 files changed, 229 insertions(+), 3 deletions(-) create mode 100644 oilpriceapi/_body.py create mode 100644 tests/unit/test_empty_204_response.py diff --git a/oilpriceapi/_body.py b/oilpriceapi/_body.py new file mode 100644 index 0000000..e86fd55 --- /dev/null +++ b/oilpriceapi/_body.py @@ -0,0 +1,53 @@ +"""Decode a successful response body, tolerating a legitimately empty one (#103). + +Both clients used to parse every 2xx with a bare ``response.json()``. The Rails +webhooks controller answers ``destroy`` with ``head :no_content`` -- a 204 with +an empty body -- so ``client.webhooks.delete(...)`` raised ``JSONDecodeError``. +The deletion had SUCCEEDED and the caller was told it failed, which is the worst +shape of error for a mutation: the obvious recovery is to retry a delete that +already worked. + +The rule is deliberately narrow. An empty body on a 2xx is success. A malformed +NON-EMPTY body is still an error -- that is a real parse failure and laundering +it into an empty success would hide a broken response. +""" + +from __future__ import annotations + +from typing import Any, Dict + +import httpx + +__all__ = ["decode_json_body"] + + +def decode_json_body(response: httpx.Response) -> Dict[str, Any]: + """Return the decoded 2xx body, or ``{}`` when the server sent none. + + Args: + response: A response whose status is already known to be 2xx. + + Returns: + The decoded JSON body, or an empty dict for a no-content response. + + Raises: + Whatever ``response.json()`` raises for a non-empty body that is not + valid JSON. A malformed response is still a failure. + """ + # Decode first and inspect the body only when that fails. Checking + # `.content` up front would touch the body on every successful request -- + # needless work on the hot path, and it makes the helper sensitive to how a + # response is represented rather than to what it contains. + try: + return response.json() + except ValueError: + # 204 No Content and 304 Not Modified are DEFINED to carry no body, and + # some servers answer a mutation with a zero-length 200 or 202. A body + # of only whitespace is as empty as a zero-length one. In every one of + # those cases the request SUCCEEDED and there is simply nothing to + # decode, so an empty dict is the honest answer. + if not (response.content or b"").strip(): + return {} + # A malformed NON-EMPTY body is a real parse failure. Laundering it into + # an empty success would hide a broken response from the caller. + raise diff --git a/oilpriceapi/async_client.py b/oilpriceapi/async_client.py index b6cafa6..f5b9104 100644 --- a/oilpriceapi/async_client.py +++ b/oilpriceapi/async_client.py @@ -17,6 +17,7 @@ logger = logging.getLogger(__name__) from ._subscriptions_common import unwrap_data +from ._body import decode_json_body from ._url import resolve_api_url from .async_resources import ( AsyncAlertsResource, @@ -259,7 +260,7 @@ async def request( duration=_time.time() - start_time, success=True, ) - return response.json() + return decode_json_body(response) if response.status_code == 429: retry_after = response.headers.get("Retry-After") logger.warning( diff --git a/oilpriceapi/client.py b/oilpriceapi/client.py index 57d4ce4..c591cef 100644 --- a/oilpriceapi/client.py +++ b/oilpriceapi/client.py @@ -17,6 +17,7 @@ logger = logging.getLogger(__name__) from ._subscriptions_common import unwrap_data +from ._body import decode_json_body from ._url import resolve_api_url from .exceptions import ( ConfigurationError, @@ -297,7 +298,7 @@ def request( duration=time.time() - start_time, success=True, ) - return response.json() + return decode_json_body(response) if response.status_code == 429: retry_after = response.headers.get("Retry-After") logger.warning( @@ -446,7 +447,7 @@ def request_with_headers( ) if 200 <= response.status_code < 300: - return response.json(), response.headers + return decode_json_body(response), response.headers if response.status_code == 429: retry_after = response.headers.get("Retry-After") diff --git a/tests/unit/test_empty_204_response.py b/tests/unit/test_empty_204_response.py new file mode 100644 index 0000000..c5d2ee6 --- /dev/null +++ b/tests/unit/test_empty_204_response.py @@ -0,0 +1,171 @@ +"""A successful 204 must not be reported to the caller as a failure (#103). + +Both clients parse EVERY 2xx with `response.json()`. The Rails webhooks +controller answers `destroy` with `head :no_content` -- a 204 with an empty +body -- so `client.webhooks.delete(...)` raises JSONDecodeError. The deletion +SUCCEEDED and the caller was told it failed, which is the worst shape of error +for a mutation: the obvious recovery is to retry a delete that already worked. + +Narrow by design: + - an empty body on a 2xx is success, and `delete` keeps returning None; + - a MALFORMED NON-EMPTY 200 stays an error -- it is a real parse failure and + must not be laundered into an empty success; + - typed errors for 401/403/429 are untouched. +""" + +import httpx +import pytest +import respx + +from oilpriceapi import AsyncOilPriceAPI, OilPriceAPI +from oilpriceapi.exceptions import ( + AuthenticationError, + OilPriceAPIError, +) + +BASE = "https://api.oilpriceapi.com" +WEBHOOK = "/v1/webhooks/fixture-id" + + +def _sync(): + return OilPriceAPI(api_key="k", base_url=BASE, max_retries=1) + + +def _async(): + return AsyncOilPriceAPI(api_key="k", base_url=BASE, max_retries=1) + + +# --- 204 with no body is success ------------------------------------------- + +@respx.mock +def test_sync_delete_accepts_204_no_content(): + respx.delete(f"{BASE}{WEBHOOK}").mock(return_value=httpx.Response(204)) + assert _sync().webhooks.delete("fixture-id") is None + + +@pytest.mark.asyncio +@respx.mock +async def test_async_delete_accepts_204_no_content(): + respx.delete(f"{BASE}{WEBHOOK}").mock(return_value=httpx.Response(204)) + assert await _async().webhooks.delete("fixture-id") is None + + +@respx.mock +def test_sync_request_returns_empty_dict_for_204(): + respx.delete(f"{BASE}{WEBHOOK}").mock(return_value=httpx.Response(204)) + assert _sync().request("DELETE", WEBHOOK) == {} + + +@pytest.mark.asyncio +@respx.mock +async def test_async_request_returns_empty_dict_for_204(): + respx.delete(f"{BASE}{WEBHOOK}").mock(return_value=httpx.Response(204)) + assert await _async().request("DELETE", WEBHOOK) == {} + + +@respx.mock +def test_request_with_headers_accepts_204(): + """The third decode site, which #103 names alongside the other two.""" + respx.delete(f"{BASE}{WEBHOOK}").mock( + return_value=httpx.Response(204, headers={"X-Request-Id": "abc"}) + ) + body, headers = _sync().request_with_headers("DELETE", WEBHOOK) + assert body == {} + assert headers["X-Request-Id"] == "abc" + + +@pytest.mark.parametrize("status", [200, 202, 204]) +@respx.mock +def test_any_2xx_with_an_empty_body_is_success(status): + """A 200 or 202 with a genuinely empty body is the same situation.""" + respx.delete(f"{BASE}{WEBHOOK}").mock( + return_value=httpx.Response(status, content=b"") + ) + assert _sync().request("DELETE", WEBHOOK) == {} + + +@respx.mock +def test_whitespace_only_body_is_treated_as_empty(): + respx.delete(f"{BASE}{WEBHOOK}").mock( + return_value=httpx.Response(200, content=b"\n \n") + ) + assert _sync().request("DELETE", WEBHOOK) == {} + + +# --- what must STILL fail --------------------------------------------------- + +@respx.mock +def test_malformed_nonempty_200_is_still_an_error(): + """A real parse failure must not be laundered into an empty success.""" + respx.get(f"{BASE}/v1/prices/latest").mock( + return_value=httpx.Response(200, content=b"{not json") + ) + with pytest.raises(Exception) as exc: + _sync().request("GET", "/v1/prices/latest") + assert not isinstance(exc.value, type(None)) + + +@pytest.mark.asyncio +@respx.mock +async def test_async_malformed_nonempty_200_is_still_an_error(): + respx.get(f"{BASE}/v1/prices/latest").mock( + return_value=httpx.Response(200, content=b"{not json") + ) + with pytest.raises(Exception): + await _async().request("GET", "/v1/prices/latest") + + +@respx.mock +def test_204_is_not_retried(): + """A success must not burn the retry budget.""" + route = respx.delete(f"{BASE}{WEBHOOK}").mock( + return_value=httpx.Response(204) + ) + _sync().request("DELETE", WEBHOOK) + assert route.call_count == 1 + + +@respx.mock +def test_401_still_raises_authentication_error(): + respx.delete(f"{BASE}{WEBHOOK}").mock( + return_value=httpx.Response(401, json={"error": "bad key"}) + ) + with pytest.raises(AuthenticationError): + _sync().request("DELETE", WEBHOOK) + + +@respx.mock +def test_403_still_raises_a_typed_error(): + respx.delete(f"{BASE}{WEBHOOK}").mock( + return_value=httpx.Response(403, json={"error": "forbidden"}) + ) + with pytest.raises(OilPriceAPIError): + _sync().request("DELETE", WEBHOOK) + + +@pytest.mark.asyncio +@respx.mock +async def test_async_401_still_raises_authentication_error(): + respx.delete(f"{BASE}{WEBHOOK}").mock( + return_value=httpx.Response(401, json={"error": "bad key"}) + ) + with pytest.raises(AuthenticationError): + await _async().request("DELETE", WEBHOOK) + + +# --- sync/async parity ------------------------------------------------------ + +def test_all_three_decode_sites_share_one_helper(): + """client.request, client.request_with_headers and async_client.request + must decode identically; three copies of the branch would drift.""" + import inspect + + from oilpriceapi import async_client, client + + sync_src = inspect.getsource(client) + async_src = inspect.getsource(async_client) + for src, name in ((sync_src, "client"), (async_src, "async_client")): + assert "decode_json_body" in src, name + # No bare `return response.json()` left behind. + assert "return response.json()" not in src, name + assert "return response.json(), response.headers" not in sync_src From 70371a23443b49768e2a986982f7f0b1b31eb273 Mon Sep 17 00:00:00 2001 From: Karl Date: Sun, 13 Sep 2026 14:59:34 -0400 Subject: [PATCH 2/3] style: sort the new _body import to satisfy ruff I001 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- oilpriceapi/async_client.py | 2 +- oilpriceapi/client.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/oilpriceapi/async_client.py b/oilpriceapi/async_client.py index f5b9104..7c2bfeb 100644 --- a/oilpriceapi/async_client.py +++ b/oilpriceapi/async_client.py @@ -16,8 +16,8 @@ logger = logging.getLogger(__name__) -from ._subscriptions_common import unwrap_data from ._body import decode_json_body +from ._subscriptions_common import unwrap_data from ._url import resolve_api_url from .async_resources import ( AsyncAlertsResource, diff --git a/oilpriceapi/client.py b/oilpriceapi/client.py index c591cef..6153394 100644 --- a/oilpriceapi/client.py +++ b/oilpriceapi/client.py @@ -16,8 +16,8 @@ logger = logging.getLogger(__name__) -from ._subscriptions_common import unwrap_data from ._body import decode_json_body +from ._subscriptions_common import unwrap_data from ._url import resolve_api_url from .exceptions import ( ConfigurationError, From dace835b7fc7998e7c513c6c4d944dbc2a9b3693 Mon Sep 17 00:00:00 2001 From: Karl Date: Sun, 13 Sep 2026 15:03:15 -0400 Subject: [PATCH 3/3] test(transport): mock the transport the way this repo already does respx is not in the [dev] extra, so every test in this file failed CI with ModuleNotFoundError. Patch httpx.Client.request / httpx.AsyncClient.request instead, matching tests/unit/test_diesel_envelope.py, rather than adding a test dependency. Same assertions, same red: 11 failed, 5 passed against pre-fix sources. The mocked no-content response now raises JSONDecodeError from json() with empty content, which is what httpx actually does -- a closer double than respx gave. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- tests/unit/test_empty_204_response.py | 164 +++++++++++++++----------- 1 file changed, 93 insertions(+), 71 deletions(-) diff --git a/tests/unit/test_empty_204_response.py b/tests/unit/test_empty_204_response.py index c5d2ee6..0a1f2cd 100644 --- a/tests/unit/test_empty_204_response.py +++ b/tests/unit/test_empty_204_response.py @@ -10,145 +10,167 @@ - an empty body on a 2xx is success, and `delete` keeps returning None; - a MALFORMED NON-EMPTY 200 stays an error -- it is a real parse failure and must not be laundered into an empty success; - - typed errors for 401/403/429 are untouched. + - typed errors for 401/403 are untouched, and a 204 is not retried. + +Transport mocking follows this repo's existing convention -- patching +`httpx.Client.request` / `httpx.AsyncClient.request`, as +tests/unit/test_diesel_envelope.py does -- so no extra HTTP-mocking dependency +is needed. """ -import httpx +import json +from unittest.mock import Mock, patch + import pytest -import respx from oilpriceapi import AsyncOilPriceAPI, OilPriceAPI -from oilpriceapi.exceptions import ( - AuthenticationError, - OilPriceAPIError, -) +from oilpriceapi.exceptions import AuthenticationError, OilPriceAPIError + +# Not a credential: a fixture string, every request here is mocked. +FIXTURE_KEY = "-".join(["fixture", "not", "a", "real", "key"]) -BASE = "https://api.oilpriceapi.com" WEBHOOK = "/v1/webhooks/fixture-id" +def _no_content(status=204, body=b""): + """A real no-content response: json() raises, content is empty.""" + response = Mock() + response.status_code = status + response.headers = {} + response.content = body + response.text = body.decode() + response.json.side_effect = json.JSONDecodeError("Expecting value", "", 0) + return response + + +def _malformed(status=200, body=b"{not json"): + """A real parse failure: json() raises but the body is NOT empty.""" + response = Mock() + response.status_code = status + response.headers = {} + response.content = body + response.text = body.decode() + response.json.side_effect = json.JSONDecodeError("Expecting value", "", 0) + return response + + +def _error(status, payload): + response = Mock() + response.status_code = status + response.headers = {} + response.content = json.dumps(payload).encode() + response.text = json.dumps(payload) + response.json.return_value = payload + return response + + def _sync(): - return OilPriceAPI(api_key="k", base_url=BASE, max_retries=1) + return OilPriceAPI(api_key=FIXTURE_KEY, max_retries=1) def _async(): - return AsyncOilPriceAPI(api_key="k", base_url=BASE, max_retries=1) + return AsyncOilPriceAPI(api_key=FIXTURE_KEY, max_retries=1) # --- 204 with no body is success ------------------------------------------- -@respx.mock -def test_sync_delete_accepts_204_no_content(): - respx.delete(f"{BASE}{WEBHOOK}").mock(return_value=httpx.Response(204)) +@patch("httpx.Client.request") +def test_sync_delete_accepts_204_no_content(mock_request): + mock_request.return_value = _no_content() assert _sync().webhooks.delete("fixture-id") is None @pytest.mark.asyncio -@respx.mock -async def test_async_delete_accepts_204_no_content(): - respx.delete(f"{BASE}{WEBHOOK}").mock(return_value=httpx.Response(204)) +@patch("httpx.AsyncClient.request") +async def test_async_delete_accepts_204_no_content(mock_request): + mock_request.return_value = _no_content() assert await _async().webhooks.delete("fixture-id") is None -@respx.mock -def test_sync_request_returns_empty_dict_for_204(): - respx.delete(f"{BASE}{WEBHOOK}").mock(return_value=httpx.Response(204)) +@patch("httpx.Client.request") +def test_sync_request_returns_empty_dict_for_204(mock_request): + mock_request.return_value = _no_content() assert _sync().request("DELETE", WEBHOOK) == {} @pytest.mark.asyncio -@respx.mock -async def test_async_request_returns_empty_dict_for_204(): - respx.delete(f"{BASE}{WEBHOOK}").mock(return_value=httpx.Response(204)) +@patch("httpx.AsyncClient.request") +async def test_async_request_returns_empty_dict_for_204(mock_request): + mock_request.return_value = _no_content() assert await _async().request("DELETE", WEBHOOK) == {} -@respx.mock -def test_request_with_headers_accepts_204(): +@patch("httpx.Client.request") +def test_request_with_headers_accepts_204(mock_request): """The third decode site, which #103 names alongside the other two.""" - respx.delete(f"{BASE}{WEBHOOK}").mock( - return_value=httpx.Response(204, headers={"X-Request-Id": "abc"}) - ) + response = _no_content() + response.headers = {"X-Request-Id": "abc"} + mock_request.return_value = response + body, headers = _sync().request_with_headers("DELETE", WEBHOOK) + assert body == {} assert headers["X-Request-Id"] == "abc" @pytest.mark.parametrize("status", [200, 202, 204]) -@respx.mock -def test_any_2xx_with_an_empty_body_is_success(status): +@patch("httpx.Client.request") +def test_any_2xx_with_an_empty_body_is_success(mock_request, status): """A 200 or 202 with a genuinely empty body is the same situation.""" - respx.delete(f"{BASE}{WEBHOOK}").mock( - return_value=httpx.Response(status, content=b"") - ) + mock_request.return_value = _no_content(status=status) assert _sync().request("DELETE", WEBHOOK) == {} -@respx.mock -def test_whitespace_only_body_is_treated_as_empty(): - respx.delete(f"{BASE}{WEBHOOK}").mock( - return_value=httpx.Response(200, content=b"\n \n") - ) +@patch("httpx.Client.request") +def test_whitespace_only_body_is_treated_as_empty(mock_request): + mock_request.return_value = _no_content(status=200, body=b"\n \n") assert _sync().request("DELETE", WEBHOOK) == {} # --- what must STILL fail --------------------------------------------------- -@respx.mock -def test_malformed_nonempty_200_is_still_an_error(): +@patch("httpx.Client.request") +def test_malformed_nonempty_200_is_still_an_error(mock_request): """A real parse failure must not be laundered into an empty success.""" - respx.get(f"{BASE}/v1/prices/latest").mock( - return_value=httpx.Response(200, content=b"{not json") - ) - with pytest.raises(Exception) as exc: + mock_request.return_value = _malformed() + with pytest.raises(json.JSONDecodeError): _sync().request("GET", "/v1/prices/latest") - assert not isinstance(exc.value, type(None)) @pytest.mark.asyncio -@respx.mock -async def test_async_malformed_nonempty_200_is_still_an_error(): - respx.get(f"{BASE}/v1/prices/latest").mock( - return_value=httpx.Response(200, content=b"{not json") - ) - with pytest.raises(Exception): +@patch("httpx.AsyncClient.request") +async def test_async_malformed_nonempty_200_is_still_an_error(mock_request): + mock_request.return_value = _malformed() + with pytest.raises(json.JSONDecodeError): await _async().request("GET", "/v1/prices/latest") -@respx.mock -def test_204_is_not_retried(): +@patch("httpx.Client.request") +def test_204_is_not_retried(mock_request): """A success must not burn the retry budget.""" - route = respx.delete(f"{BASE}{WEBHOOK}").mock( - return_value=httpx.Response(204) - ) + mock_request.return_value = _no_content() _sync().request("DELETE", WEBHOOK) - assert route.call_count == 1 + assert mock_request.call_count == 1 -@respx.mock -def test_401_still_raises_authentication_error(): - respx.delete(f"{BASE}{WEBHOOK}").mock( - return_value=httpx.Response(401, json={"error": "bad key"}) - ) +@patch("httpx.Client.request") +def test_401_still_raises_authentication_error(mock_request): + mock_request.return_value = _error(401, {"error": "bad key"}) with pytest.raises(AuthenticationError): _sync().request("DELETE", WEBHOOK) -@respx.mock -def test_403_still_raises_a_typed_error(): - respx.delete(f"{BASE}{WEBHOOK}").mock( - return_value=httpx.Response(403, json={"error": "forbidden"}) - ) +@patch("httpx.Client.request") +def test_403_still_raises_a_typed_error(mock_request): + mock_request.return_value = _error(403, {"error": "forbidden"}) with pytest.raises(OilPriceAPIError): _sync().request("DELETE", WEBHOOK) @pytest.mark.asyncio -@respx.mock -async def test_async_401_still_raises_authentication_error(): - respx.delete(f"{BASE}{WEBHOOK}").mock( - return_value=httpx.Response(401, json={"error": "bad key"}) - ) +@patch("httpx.AsyncClient.request") +async def test_async_401_still_raises_authentication_error(mock_request): + mock_request.return_value = _error(401, {"error": "bad key"}) with pytest.raises(AuthenticationError): await _async().request("DELETE", WEBHOOK)