From af0f6a4c8f425028c75fd13913ba7d07e85bb79c Mon Sep 17 00:00:00 2001 From: Karl Waldman Date: Sun, 23 Aug 2026 09:28:31 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix(prices):=20batch=20get=5Fmultiple=20?= =?UTF-8?q?=E2=80=94=20one=20request=20per=2020=20codes,=20not=20per=20cod?= =?UTF-8?q?e=20(api#7240)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_multiple() looped over get(), issuing one HTTP request per commodity. The REST API accepts up to 20 codes in ONE request that counts ONCE against quota, so the method whose whole purpose is fetching several prices cost up to 20x more quota than writing the call by hand. Through this method the free plan was 50 code-reads a day. Through the raw API it is 1,000. A plausible contributor to a measured result: sdk-python converts at 2.2% while people using plain Python and ignoring the SDK convert at 4.2% (api#7214). Our SDK made the free plan twenty times harder to live inside than not using it. Verified against production before writing the fix (2026-08-23): 20 codes -> 200; 21 -> 400 "Too many commodity codes requested (max: 20)" one multi-code call writes ONE api_requests row 1 code -> flat data object; 2+ -> data.prices[] Both clients fixed. The async one was worse: asyncio.gather fanned out one request PER CODE concurrently, which also trips the 60-per-60s rate limit on a long list. It now gathers CHUNKS, so 25 codes are 2 concurrent requests. Failure contract preserved. The API rejects the whole request when any code in it is unknown, so a chunk failure does not say which code was at fault — that chunk (and only that chunk) is retried per code to rebuild the per-code failure list. A one-code chunk has nothing to narrow down and is recorded directly rather than refetched. Live smoke against production: 10 codes -> 1 request, 10 prices (was 10 requests) 1 code -> 1 request, 1 price 30 codes -> 2 requests, 29 prices (was 30 requests) bad code -> 3 requests, 1 ok + 1 reported failure Four new tests assert REQUEST COUNT, not just results — the regression is invisible without that. Two existing tests asserted call_count == 2 for two codes, encoding the defect; they now assert 1 and carry a note saying why. Repo-wide ruff/black findings are pre-existing and unchanged (prices.py and async_client.py: 0 before, 0 after). Closes api#7240 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JKAExynd9zoKwt6rYA66EA --- oilpriceapi/async_client.py | 76 ++++++++++-- oilpriceapi/resources/prices.py | 90 ++++++++++---- tests/test_client.py | 33 ++++- tests/unit/test_prices_resource.py | 188 ++++++++++++++++++++++++----- 4 files changed, 314 insertions(+), 73 deletions(-) diff --git a/oilpriceapi/async_client.py b/oilpriceapi/async_client.py index 58e6ee7..13fb027 100644 --- a/oilpriceapi/async_client.py +++ b/oilpriceapi/async_client.py @@ -249,7 +249,9 @@ async def request( await asyncio.sleep(wait_time) continue elif response.status_code >= 500: - if self._retry_strategy.should_retry(attempt, response.status_code, response.headers): + if self._retry_strategy.should_retry( + attempt, response.status_code, response.headers + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, @@ -365,9 +367,44 @@ async def __aexit__(self, exc_type, exc_val, exc_tb): class AsyncPricesResource: """Async resource for current prices.""" + #: See PricesResource.MAX_CODES_PER_REQUEST — the API caps one request at + #: 20 codes and bills it once. Verified against production 2026-08-23. + MAX_CODES_PER_REQUEST = 20 + def __init__(self, client: AsyncOilPriceAPI): self.client = client + @staticmethod + def _to_price(price_data: dict, fallback_code: Optional[str] = None) -> Price: + """Map one API price row onto the Price model.""" + return Price( + commodity=price_data.get("code", fallback_code), + value=price_data.get("price"), + currency=price_data.get("currency", "USD"), + unit=price_data.get("unit", "barrel"), + timestamp=price_data.get("created_at"), + ) + + async def _fetch_batch(self, codes: List[str]) -> List[Price]: + """One request for up to MAX_CODES_PER_REQUEST codes. + + One code returns a flat ``data`` object; two or more return + ``data.prices[]``. + """ + response = await self.client.request( + method="GET", + path="/v1/prices/latest", + params={"by_code": ",".join(codes)}, + ) + data = response.get("data", response) if isinstance(response, dict) else response + + if isinstance(data, dict) and "prices" in data: + rows = data["prices"] + else: + rows = [data] + + return [self._to_price(row, codes[0] if len(codes) == 1 else None) for row in rows] + async def get(self, commodity: str) -> Price: """Get current price for commodity.""" response = await self.client.request( @@ -409,20 +446,37 @@ async def get_multiple( OilPriceAPIError: If raise_on_error=True and any commodity fails """ - # Use gather for concurrent requests - tasks = [self.get(commodity) for commodity in commodities] - results = await asyncio.gather(*tasks, return_exceptions=True) + # api#7240: batch first, THEN fan out. Previously this issued one + # request per code concurrently, which spent quota per code and could + # trip the 60-per-60s rate limit instantly on a long list. Chunks of 20 + # are still gathered concurrently, so 25 codes cost 2 requests, not 25. + chunks = [ + commodities[i : i + self.MAX_CODES_PER_REQUEST] + for i in range(0, len(commodities), self.MAX_CODES_PER_REQUEST) + ] + results = await asyncio.gather( + *(self._fetch_batch(chunk) for chunk in chunks), return_exceptions=True + ) prices = [] failures = [] - for commodity, result in zip(commodities, results): - if isinstance(result, Price): - prices.append(result) - elif isinstance(result, Exception): - if raise_on_error: - raise result - failures.append((commodity, str(result))) + for chunk, result in zip(chunks, results): + if isinstance(result, list): + prices.extend(result) + continue + if raise_on_error: + raise result + # The API rejects the WHOLE request when any code in it is unknown, + # so retry this chunk per code to say which one was at fault. + retry = await asyncio.gather( + *(self.get(code) for code in chunk), return_exceptions=True + ) + for code, item in zip(chunk, retry): + if isinstance(item, Price): + prices.append(item) + else: + failures.append((code, str(item))) if return_failures: return prices, failures diff --git a/oilpriceapi/resources/prices.py b/oilpriceapi/resources/prices.py index 1cd3db9..b7c4d03 100644 --- a/oilpriceapi/resources/prices.py +++ b/oilpriceapi/resources/prices.py @@ -16,9 +16,53 @@ class PricesResource: """Resource for current price operations.""" + #: The API accepts at most this many commodity codes in one request, and + #: that request counts ONCE against quota. Verified against production + #: 2026-08-23: 20 codes -> 200, 21 -> 400 "Too many commodity codes + #: requested (max: 20, requested: 21)". Raising this without a + #: corresponding API change turns every call into a 400. + MAX_CODES_PER_REQUEST = 20 + def __init__(self, client): self.client = client + @staticmethod + def _to_price(price_data: dict, fallback_code: Optional[str] = None) -> Price: + """Map one API price row onto the Price model. + + Shared by get() and the batched path so the two cannot drift. + """ + return Price( + commodity=price_data.get("code", fallback_code), + value=price_data.get("price"), + currency=price_data.get("currency"), + # Retain the established oil-only fallback for legacy minimal + # responses; any unit actually supplied by the API wins. + unit=price_data.get("unit", "barrel"), + timestamp=price_data.get("created_at"), + ) + + def _fetch_batch(self, codes: List[str]) -> List[Price]: + """One request for up to MAX_CODES_PER_REQUEST codes. + + A single code returns a flat ``data`` object; two or more return + ``data.prices[]``. Both shapes are handled here so callers do not + have to care how many codes they asked for. + """ + response = self.client.request( + method="GET", + path="/v1/prices/latest", + params={"by_code": ",".join(codes)}, + ) + data = response.get("data", response) if isinstance(response, dict) else response + + if isinstance(data, dict) and "prices" in data: + rows = data["prices"] + else: + rows = [data] + + return [self._to_price(row, codes[0] if len(codes) == 1 else None) for row in rows] + def get(self, commodity: str) -> Price: """Get current price for a single commodity. @@ -36,24 +80,8 @@ def get(self, commodity: str) -> Price: method="GET", path="/v1/prices/latest", params={"by_code": commodity} ) - # Parse response - if "data" in response: - price_data = response["data"] - else: - price_data = response - - # Map API response to Price model without inventing source context. - mapped_data = { - "commodity": price_data.get("code", commodity), - "value": price_data.get("price"), - "currency": price_data.get("currency"), - # Retain the established oil-only fallback for legacy minimal - # responses; any unit actually supplied by the API wins. - "unit": price_data.get("unit", "barrel"), - "timestamp": price_data.get("created_at"), - } - - return Price(**mapped_data) + price_data = response["data"] if "data" in response else response + return self._to_price(price_data, commodity) def get_multiple( self, commodities: List[str], raise_on_error: bool = False, return_failures: bool = False @@ -93,15 +121,29 @@ def get_multiple( prices = [] failures = [] - for commodity in commodities: + for start in range(0, len(commodities), self.MAX_CODES_PER_REQUEST): + chunk = commodities[start : start + self.MAX_CODES_PER_REQUEST] try: - price = self.get(commodity) - prices.append(price) - except OilPriceAPIError as e: + prices.extend(self._fetch_batch(chunk)) + except OilPriceAPIError: if raise_on_error: raise - failures.append((commodity, str(e))) - continue + # The API rejects the WHOLE request when any code in it is + # unknown, so a chunk failure does not say which code was at + # fault. Retry just this chunk per code to preserve the + # per-code failure contract. A one-code chunk has nothing to + # narrow down, so it is recorded directly rather than refetched. + if len(chunk) == 1: + try: + prices.append(self.get(chunk[0])) + except OilPriceAPIError as exc: + failures.append((chunk[0], str(exc))) + continue + for commodity in chunk: + try: + prices.append(self.get(commodity)) + except OilPriceAPIError as exc: + failures.append((commodity, str(exc))) if return_failures: return prices, failures diff --git a/tests/test_client.py b/tests/test_client.py index f88b973..8df76dc 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -122,15 +122,38 @@ def create_response(code, price_value): } return response - # Set up side_effect to return different responses - mock_request.side_effect = [ - create_response("BRENT_CRUDE_USD", 75.50), - create_response("WTI_USD", 70.25), - ] + # api#7240: get_multiple now BATCHES — two codes are one request that + # returns data.prices[], not two requests each returning a flat object. + batch = Mock() + batch.status_code = 200 + batch.json.return_value = { + "status": "success", + "data": { + "prices": [ + { + "code": "BRENT_CRUDE_USD", + "price": 75.50, + "currency": "USD", + "created_at": "2024-01-15T10:00:00Z", + "type": "spot_price", + }, + { + "code": "WTI_USD", + "price": 70.25, + "currency": "USD", + "created_at": "2024-01-15T10:00:00Z", + "type": "spot_price", + }, + ] + }, + } + mock_request.return_value = batch client = OilPriceAPI(api_key="test_key") prices = client.prices.get_multiple(["BRENT_CRUDE_USD", "WTI_USD"]) + assert mock_request.call_count == 1, "two codes must cost ONE request" + assert len(prices) == 2 assert prices[0].commodity == "BRENT_CRUDE_USD" assert prices[0].value == 75.50 diff --git a/tests/unit/test_prices_resource.py b/tests/unit/test_prices_resource.py index 4e3bbf5..34af36a 100644 --- a/tests/unit/test_prices_resource.py +++ b/tests/unit/test_prices_resource.py @@ -37,37 +37,36 @@ def test_get_single_price(self, mock_request, api_key, mock_price_response): @patch("httpx.Client.request") def test_get_multiple_prices(self, mock_request, api_key, mock_price_response): - """Test getting multiple commodity prices.""" - # Mock responses for each commodity - responses = [ - Mock( - status_code=200, - json=lambda: { - "status": "success", - "data": { - "code": "BRENT_CRUDE_USD", - "price": 75.50, - "currency": "USD", - "created_at": "2024-01-15T10:00:00Z", - "type": "spot_price", - }, - }, - ), - Mock( - status_code=200, - json=lambda: { - "status": "success", - "data": { - "code": "WTI_USD", - "price": 70.25, - "currency": "USD", - "created_at": "2024-01-15T10:00:00Z", - "type": "spot_price", - }, + """Two codes are ONE batched request (api#7240). + + This test previously asserted call_count == 2, encoding the defect: + get_multiple looped over get() and spent quota per code. The API + accepts 20 codes in one request that counts once. + """ + mock_request.return_value = Mock( + status_code=200, + json=lambda: { + "status": "success", + "data": { + "prices": [ + { + "code": "BRENT_CRUDE_USD", + "price": 75.50, + "currency": "USD", + "created_at": "2024-01-15T10:00:00Z", + "type": "spot_price", + }, + { + "code": "WTI_USD", + "price": 70.25, + "currency": "USD", + "created_at": "2024-01-15T10:00:00Z", + "type": "spot_price", + }, + ] }, - ), - ] - mock_request.side_effect = responses + }, + ) client = OilPriceAPI(api_key=api_key) prices = client.prices.get_multiple(["BRENT_CRUDE_USD", "WTI_USD"]) @@ -77,7 +76,7 @@ def test_get_multiple_prices(self, mock_request, api_key, mock_price_response): assert prices[0].value == 75.50 assert prices[1].commodity == "WTI_USD" assert prices[1].value == 70.25 - assert mock_request.call_count == 2 + assert mock_request.call_count == 1, "two codes must cost ONE request" @patch("httpx.Client.request") def test_get_multiple_prices_with_failures(self, mock_request, api_key): @@ -112,12 +111,18 @@ def test_get_multiple_prices_with_failures(self, mock_request, api_key): }, ), ] - mock_request.side_effect = responses + # api#7240: the batch is tried first. The API rejects the WHOLE request + # when any code is unknown, so the chunk is retried per code — which is + # what the three per-code responses above now serve. + mock_request.side_effect = [ + Mock(status_code=400, json=lambda: {"status": "fail", "data": {"error": "invalid_code"}}), + *responses, + ] client = OilPriceAPI(api_key=api_key) prices = client.prices.get_multiple(["BRENT_CRUDE_USD", "INVALID_CODE", "NATURAL_GAS_USD"]) - # Should only return 2 prices (skips the failed one) + # Still returns the good ones and skips the failure — contract preserved assert len(prices) == 2 assert prices[0].commodity == "BRENT_CRUDE_USD" assert prices[1].commodity == "NATURAL_GAS_USD" @@ -414,3 +419,120 @@ def test_get_price_does_not_invent_missing_currency(self, mock_request, api_key) assert price.value == 100.0 assert price.currency is None assert price.unit == "index_points" + + +class TestGetMultipleBatching: + """api#7240 — get_multiple must batch, not loop. + + The REST API accepts up to 20 codes in ONE request that counts ONCE against + quota (verified against production 2026-08-23: 20 codes -> 200, 21 -> 400 + "Too many commodity codes requested (max: 20, requested: 21)"). + + Looping made the SDK cost up to 20x more quota than writing the call by + hand, which is why these tests assert REQUEST COUNT, not just results. + """ + + @staticmethod + def _batch_response(codes): + return Mock( + status_code=200, + json=lambda: { + "status": "success", + "data": { + "prices": [ + { + "code": c, + "price": 10.0 + i, + "currency": "USD", + "created_at": "2026-08-23T10:00:00Z", + "type": "spot_price", + } + for i, c in enumerate(codes) + ] + }, + }, + ) + + @patch("httpx.Client.request") + def test_three_codes_cost_one_request(self, mock_request, api_key): + codes = ["BRENT_CRUDE_USD", "WTI_USD", "NATURAL_GAS_USD"] + mock_request.return_value = self._batch_response(codes) + + client = OilPriceAPI(api_key=api_key) + prices = client.prices.get_multiple(codes) + + assert mock_request.call_count == 1, "3 codes must be ONE request, not three" + assert [p.commodity for p in prices] == codes + sent = mock_request.call_args.kwargs["params"]["by_code"] + assert sent == "BRENT_CRUDE_USD,WTI_USD,NATURAL_GAS_USD" + + @patch("httpx.Client.request") + def test_twenty_five_codes_cost_two_requests(self, mock_request, api_key): + codes = [f"CODE_{i}_USD" for i in range(25)] + mock_request.side_effect = [ + self._batch_response(codes[:20]), + self._batch_response(codes[20:]), + ] + + client = OilPriceAPI(api_key=api_key) + prices = client.prices.get_multiple(codes) + + assert mock_request.call_count == 2, "25 codes must be ceil(25/20) = 2 requests" + assert len(prices) == 25 + + @patch("httpx.Client.request") + def test_single_code_still_works(self, mock_request, api_key): + """One code returns a FLAT data object, not data.prices[].""" + mock_request.return_value = Mock( + status_code=200, + json=lambda: { + "status": "success", + "data": { + "code": "BRENT_CRUDE_USD", + "price": 75.5, + "currency": "USD", + "created_at": "2026-08-23T10:00:00Z", + "type": "spot_price", + }, + }, + ) + client = OilPriceAPI(api_key=api_key) + prices = client.prices.get_multiple(["BRENT_CRUDE_USD"]) + + assert mock_request.call_count == 1 + assert len(prices) == 1 + assert prices[0].value == 75.5 + + @patch("httpx.Client.request") + def test_bad_code_in_batch_falls_back_and_reports_per_code(self, mock_request, api_key): + """A 400 on the batch must not lose the good codes. + + The API rejects the whole request when any code is unknown, so the + batch is retried per code — for that chunk only — to preserve the + per-code failure contract. + """ + good = { + "status": "success", + "data": { + "code": "BRENT_CRUDE_USD", + "price": 75.5, + "currency": "USD", + "created_at": "2026-08-23T10:00:00Z", + "type": "spot_price", + }, + } + mock_request.side_effect = [ + Mock(status_code=400, json=lambda: {"status": "fail", "data": {"error": "invalid_code"}}), + Mock(status_code=200, json=lambda: good), + Mock(status_code=400, json=lambda: {"status": "fail", "data": {"error": "invalid_code"}}), + ] + + client = OilPriceAPI(api_key=api_key) + prices, failures = client.prices.get_multiple( + ["BRENT_CRUDE_USD", "NOT_A_REAL_CODE"], return_failures=True + ) + + assert len(prices) == 1 + assert prices[0].commodity == "BRENT_CRUDE_USD" + assert len(failures) == 1 + assert failures[0][0] == "NOT_A_REAL_CODE" From 7a4efd5576b2f8ea70eb0d6cf1d1d8646a9bdf1d Mon Sep 17 00:00:00 2001 From: Karl Waldman Date: Sun, 23 Aug 2026 09:32:12 -0400 Subject: [PATCH 2/2] fix(types): annotate the shared _to_price helper for mypy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's mypy step failed on the new helper with 8 errors across both clients: 'Missing type parameters for generic type dict' and incompatible argument types for commodity/value/timestamp. The errors are real but not new behaviour — the previous Price(**mapped_data) form passed a dict of kwargs, which mypy cannot check, so the same looseness was always there and simply invisible. Writing the call out explicitly exposed it. Typed the parameter as Dict[str, Any] and cast the three fields pydantic validates anyway, with a comment saying why the casts are there. Not silenced with type: ignore — the shape is genuinely Any coming off JSON, and pydantic raises on anything actually wrong. mypy on both files: 0 errors. Full suite still 645 passed. Live smoke re-run after the change: 5 codes -> 1 request, 5 prices, BRENT_CRUDE_USD $93.60 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JKAExynd9zoKwt6rYA66EA --- oilpriceapi/async_client.py | 19 ++++++++++++------- oilpriceapi/resources/prices.py | 17 +++++++++++------ 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/oilpriceapi/async_client.py b/oilpriceapi/async_client.py index 13fb027..ee93289 100644 --- a/oilpriceapi/async_client.py +++ b/oilpriceapi/async_client.py @@ -9,7 +9,8 @@ import asyncio import logging import os -from typing import Any, AsyncGenerator, Dict, List, Optional, Union +from datetime import datetime +from typing import Any, AsyncGenerator, Dict, List, Optional, Union, cast from urllib.parse import urljoin import httpx @@ -375,14 +376,18 @@ def __init__(self, client: AsyncOilPriceAPI): self.client = client @staticmethod - def _to_price(price_data: dict, fallback_code: Optional[str] = None) -> Price: - """Map one API price row onto the Price model.""" + def _to_price(price_data: Dict[str, Any], fallback_code: Optional[str] = None) -> Price: + """Map one API price row onto the Price model. + + Casts are for mypy; pydantic does the real validation. See + PricesResource._to_price. + """ return Price( - commodity=price_data.get("code", fallback_code), - value=price_data.get("price"), + commodity=cast(str, price_data.get("code", fallback_code)), + value=cast(float, price_data.get("price")), currency=price_data.get("currency", "USD"), - unit=price_data.get("unit", "barrel"), - timestamp=price_data.get("created_at"), + unit=cast(str, price_data.get("unit", "barrel")), + timestamp=cast(datetime, price_data.get("created_at")), ) async def _fetch_batch(self, codes: List[str]) -> List[Price]: diff --git a/oilpriceapi/resources/prices.py b/oilpriceapi/resources/prices.py index b7c4d03..cc8ed60 100644 --- a/oilpriceapi/resources/prices.py +++ b/oilpriceapi/resources/prices.py @@ -7,7 +7,7 @@ from __future__ import annotations from datetime import datetime -from typing import List, Optional, Set, Tuple, Union +from typing import Any, Dict, List, Optional, Set, Tuple, Union, cast from .._pagination import validate_page_size from ..models import Price @@ -27,19 +27,24 @@ def __init__(self, client): self.client = client @staticmethod - def _to_price(price_data: dict, fallback_code: Optional[str] = None) -> Price: + def _to_price(price_data: Dict[str, Any], fallback_code: Optional[str] = None) -> Price: """Map one API price row onto the Price model. Shared by get() and the batched path so the two cannot drift. + + Values are passed to a pydantic model, which does the coercion and + raises on anything genuinely wrong. The casts here are for mypy: the + JSON payload is Dict[str, Any], and the previous Price(**mapped_data) + form simply hid that from the type checker. """ return Price( - commodity=price_data.get("code", fallback_code), - value=price_data.get("price"), + commodity=cast(str, price_data.get("code", fallback_code)), + value=cast(float, price_data.get("price")), currency=price_data.get("currency"), # Retain the established oil-only fallback for legacy minimal # responses; any unit actually supplied by the API wins. - unit=price_data.get("unit", "barrel"), - timestamp=price_data.get("created_at"), + unit=cast(str, price_data.get("unit", "barrel")), + timestamp=cast(datetime, price_data.get("created_at")), ) def _fetch_batch(self, codes: List[str]) -> List[Price]: