From 1feb8821884392e788a531289d8d47fb79499759 Mon Sep 17 00:00:00 2001 From: Karl Waldman Date: Sun, 13 Sep 2026 16:06:51 -0400 Subject: [PATCH 1/2] feat(subscriptions): get, update, pause, resume lifecycle (#100) Adds sync and async get/update/pause/resume against GET/PATCH /v1/subscriptions/{id} and POST /v1/subscriptions/{id}/pause|resume, typed as Subscription from the wire shape V1::SubscriptionsController serves. - ids and update payloads are validated before any request is built - a malformed success raises OilPriceAPIError(code="MALFORMED_RESPONSE"); create() now uses the same strict unwrap instead of falling back to data - delete() validates the id too - Subscription.codes is required rather than defaulting to [] - update/pause/resume stay non-replayable writes (KNOWN_WRITES policy) - live lifecycle smoke creates its own watch and deletes it in finally Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- CHANGELOG.md | 30 + docs/reference/resources.md | 8 + oilpriceapi/_subscriptions_common.py | 115 +++- oilpriceapi/async_resources.py | 64 ++- oilpriceapi/models.py | 5 +- oilpriceapi/resources/subscriptions.py | 128 ++++- tests/integration/test_live_subscriptions.py | 56 +- tests/unit/test_subscriptions_lifecycle.py | 553 +++++++++++++++++++ 8 files changed, 949 insertions(+), 10 deletions(-) create mode 100644 tests/unit/test_subscriptions_lifecycle.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a7f341..6f959db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,36 @@ All notable changes to the OilPriceAPI Python SDK will be documented in this fil ## [Unreleased] +### Added + +- **Subscription lifecycle: `get`, `update`, `pause`, `resume` (#100).** Sync + and async, against `GET`/`PATCH /v1/subscriptions/{id}` and + `POST /v1/subscriptions/{id}/pause|resume`. Each returns a typed + `Subscription` with the server's timestamps and nulls as sent. `update()` + sends only the fields you pass (`name`, `codes`, `interval`, + `deliver_webhook`, `status`). Ids and update payloads are validated before + any request is built, and an invalid one raises `ValueError` with nothing + sent. Unknown ids raise `DataNotFoundError`; a refused update (interval + below the plan minimum, webhook delivery the plan lacks) raises + `ValidationError` with the server's `details`. `update`, `pause` and + `resume` are writes and are sent once, like `create`: after an ambiguous + timeout or 5xx the error carries `ambiguous_write=True` and `get()` tells + you whether the change landed. + +### Fixed + +- **`subscriptions.create()` no longer turns a malformed success into a + half-built record.** It fell back to treating the whole `data` object as the + subscription when `data.subscription` was missing, and leaked a raw pydantic + or `TypeError` when the record was null or a list. It now raises + `OilPriceAPIError(code="MALFORMED_RESPONSE")`, the same as the new lifecycle + methods. +- **`subscriptions.delete()` validates the id before sending.** An id such as + `"abc/pause"` or `""` previously produced a request to a different route. +- **`Subscription.codes` is required.** A record with no `codes` used to + default to `[]`, reading as a watch on nothing; the API always sends it, so a + missing value now fails validation instead of being invented. + ## [1.15.0] - 2026-09-13 ### Fixed diff --git a/docs/reference/resources.md b/docs/reference/resources.md index 196819c..990c84e 100644 --- a/docs/reference/resources.md +++ b/docs/reference/resources.md @@ -63,3 +63,11 @@ ## Data Sources ::: oilpriceapi.resources.data_sources.DataSourcesResource + +## Subscriptions + +Agent price watches: `list`, `create`, `get`, `update`, `pause`, `resume`, +`delete`, and the `events` poll. A subscription here is a watch on commodity +codes, not a billing subscription. + +::: oilpriceapi.resources.subscriptions.SubscriptionsResource diff --git a/oilpriceapi/_subscriptions_common.py b/oilpriceapi/_subscriptions_common.py index b611bc0..d738a85 100644 --- a/oilpriceapi/_subscriptions_common.py +++ b/oilpriceapi/_subscriptions_common.py @@ -7,7 +7,11 @@ from __future__ import annotations -from typing import Any, Dict, List, Optional, Union +import re +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union + +if TYPE_CHECKING: + from .models import Subscription # Default attribution source stamped on subscriptions created via this SDK. DEFAULT_SOURCE = "sdk-python" @@ -103,6 +107,115 @@ def build_create_body( return body +# Watch.status is a string enum on the server: { active, paused }. +VALID_STATUSES = ("active", "paused") + +# Watch ids are UUIDs. Anything outside this alphabet would change the URL the +# request goes to (a "/" reaches another route, "?" or "#" rewrites the query), +# so it is refused before a request is built. +_SUBSCRIPTION_ID = re.compile(r"[A-Za-z0-9_-]+") + + +def validate_subscription_id(subscription_id: Any) -> str: + """Return ``subscription_id`` if it can be placed in a URL path segment. + + Raises: + ValueError: If the id is not a non-empty string of letters, digits, + ``-`` or ``_``. Nothing is sent to the API. + """ + if not isinstance(subscription_id, str) or not _SUBSCRIPTION_ID.fullmatch(subscription_id): + raise ValueError( + f"Invalid subscription id {subscription_id!r}: expected the id returned " + f"by subscriptions.list() or subscriptions.create()." + ) + return subscription_id + + +def build_update_body( + name: Optional[str] = None, + codes: Optional[List[str]] = None, + interval: Optional[Union[str, int]] = None, + deliver_webhook: Optional[bool] = None, + status: Optional[str] = None, +) -> Dict[str, Any]: + """Build the PATCH /v1/subscriptions/:id body from the fields given. + + Only arguments that are not ``None`` are sent, so an update never resets a + field the caller did not mention. + + Raises: + ValueError: If no field is given or a field is invalid. Nothing is sent. + """ + body: Dict[str, Any] = {} + if name is not None: + if not isinstance(name, str): + raise ValueError(f"name must be a string, got {type(name).__name__}") + body["name"] = name + if codes is not None: + if isinstance(codes, (str, bytes)) or not isinstance(codes, (list, tuple)): + raise ValueError("codes must be a list of commodity codes, e.g. ['BRENT_CRUDE_USD']") + if not codes: + raise ValueError("codes must contain at least one commodity code") + if not all(isinstance(code, str) and code.strip() for code in codes): + raise ValueError("every code must be a non-empty string") + body["codes"] = list(codes) + if interval is not None: + body["interval_seconds"] = normalize_interval(interval) + if deliver_webhook is not None: + if not isinstance(deliver_webhook, bool): + raise ValueError( + f"deliver_webhook must be True or False, got {deliver_webhook!r}" + ) + body["deliver_webhook"] = deliver_webhook + if status is not None: + if status not in VALID_STATUSES: + raise ValueError( + f"status must be one of {', '.join(VALID_STATUSES)}, got {status!r}" + ) + body["status"] = status + if not body: + raise ValueError( + "update() needs at least one of: name, codes, interval, deliver_webhook, status" + ) + return body + + +def unwrap_subscription(response: Any, *, subject: str) -> "Subscription": + """Return the typed ``data.subscription`` record from a success body. + + Every single-subscription endpoint (show, create, update, pause, resume) + answers ``{"status": "success", "data": {"subscription": {...}}}``. + + Raises: + OilPriceAPIError: ``code="MALFORMED_RESPONSE"`` when the record is + missing, is not an object, or lacks a required field. A malformed + success is reported, never turned into a partial or invented record. + """ + from pydantic import ValidationError as PydanticValidationError + + from .exceptions import OilPriceAPIError + from .models import Subscription + + data = response.get("data") if isinstance(response, dict) else None + record = data.get("subscription") if isinstance(data, dict) else None + if not isinstance(record, dict): + raise OilPriceAPIError( + f"Malformed {subject} response: expected data.subscription to be an object", + code="MALFORMED_RESPONSE", + raw_body=response, + ) + try: + return Subscription(**record) + except PydanticValidationError as error: + raise OilPriceAPIError( + f"Malformed {subject} response: {error.error_count()} invalid or missing " + f"subscription field(s): " + + ", ".join(".".join(str(part) for part in item["loc"]) for item in error.errors()), + code="MALFORMED_RESPONSE", + raw_body=response, + ) from error + + def unwrap_data(response: Any) -> Dict[str, Any]: """Return the ``data`` object from a ``{status, data}`` envelope.""" if isinstance(response, dict) and "data" in response: diff --git a/oilpriceapi/async_resources.py b/oilpriceapi/async_resources.py index dc01c9a..29d6e8c 100644 --- a/oilpriceapi/async_resources.py +++ b/oilpriceapi/async_resources.py @@ -6,7 +6,10 @@ from ._subscriptions_common import ( build_attribution_headers, build_create_body, + build_update_body, unwrap_data, + unwrap_subscription, + validate_subscription_id, ) from .exceptions import ValidationError from .models import DieselPrice, DieselStationsResponse, PriceAlert, Subscription, SubscriptionEvent @@ -1596,12 +1599,67 @@ async def create( json_data=body, headers=headers, ) - data = unwrap_data(response) - sub = data.get("subscription", data) - return Subscription(**sub) + return unwrap_subscription(response, subject="subscriptions.create") + + async def get(self, subscription_id: str) -> Subscription: + """Fetch one subscription. See ``SubscriptionsResource.get``.""" + subscription_id = validate_subscription_id(subscription_id) + response = await self.client.request( + method="GET", + path=f"/v1/subscriptions/{subscription_id}", + ) + return unwrap_subscription(response, subject="subscriptions.get") + + async def update( + self, + subscription_id: str, + *, + name: Optional[str] = None, + codes: Optional[List[str]] = None, + interval: Optional[Union[str, int]] = None, + deliver_webhook: Optional[bool] = None, + status: Optional[str] = None, + ) -> Subscription: + """Change a subscription; only the arguments passed are sent. + + Sent once (PATCH is not replayed). See ``SubscriptionsResource.update``. + """ + subscription_id = validate_subscription_id(subscription_id) + body = build_update_body( + name=name, + codes=codes, + interval=interval, + deliver_webhook=deliver_webhook, + status=status, + ) + response = await self.client.request( + method="PATCH", + path=f"/v1/subscriptions/{subscription_id}", + json_data=body, + ) + return unwrap_subscription(response, subject="subscriptions.update") + + async def pause(self, subscription_id: str) -> Subscription: + """Pause a subscription. See ``SubscriptionsResource.pause``.""" + subscription_id = validate_subscription_id(subscription_id) + response = await self.client.request( + method="POST", + path=f"/v1/subscriptions/{subscription_id}/pause", + ) + return unwrap_subscription(response, subject="subscriptions.pause") + + async def resume(self, subscription_id: str) -> Subscription: + """Resume a paused subscription. See ``SubscriptionsResource.resume``.""" + subscription_id = validate_subscription_id(subscription_id) + response = await self.client.request( + method="POST", + path=f"/v1/subscriptions/{subscription_id}/resume", + ) + return unwrap_subscription(response, subject="subscriptions.resume") async def delete(self, subscription_id: str) -> bool: """Delete a subscription. Returns True on success.""" + subscription_id = validate_subscription_id(subscription_id) await self.client.request( method="DELETE", path=f"/v1/subscriptions/{subscription_id}", diff --git a/oilpriceapi/models.py b/oilpriceapi/models.py index 803e101..a016bf7 100644 --- a/oilpriceapi/models.py +++ b/oilpriceapi/models.py @@ -425,7 +425,10 @@ class Subscription(BaseModel): id: str = Field(description="Unique subscription identifier") name: Optional[str] = Field(default=None, description="User-friendly subscription name") - codes: List[str] = Field(default_factory=list, description="Commodity codes being watched") + # Required: the server always sends it, and a watch cannot exist without + # codes (Watch validates presence). Defaulting a missing value to [] would + # report a malformed record as a watch on nothing (#100). + codes: List[str] = Field(description="Commodity codes being watched") interval_seconds: Optional[int] = Field(default=None, description="Evaluation interval in seconds") status: Optional[str] = Field(default=None, description="Subscription status (active, paused, etc.)") deliver_webhook: Optional[bool] = Field(default=None, description="Whether events are delivered via webhook") diff --git a/oilpriceapi/resources/subscriptions.py b/oilpriceapi/resources/subscriptions.py index 9afb040..1c90895 100644 --- a/oilpriceapi/resources/subscriptions.py +++ b/oilpriceapi/resources/subscriptions.py @@ -10,7 +10,10 @@ from .._subscriptions_common import ( build_attribution_headers, build_create_body, + build_update_body, unwrap_data, + unwrap_subscription, + validate_subscription_id, ) from ..models import Subscription, SubscriptionEvent @@ -98,9 +101,124 @@ def create( json_data=body, headers=headers, ) - data = unwrap_data(response) - sub = data.get("subscription", data) - return Subscription(**sub) + return unwrap_subscription(response, subject="subscriptions.create") + + def get(self, subscription_id: str) -> Subscription: + """Fetch one subscription. + + Args: + subscription_id: The id returned by ``list()`` or ``create()``. + + Returns: + The Subscription, with the server's timestamps and nulls as sent. + + Raises: + ValueError: If the id is malformed. Nothing is sent. + DataNotFoundError: If no subscription with that id belongs to you. + OilPriceAPIError: ``code="MALFORMED_RESPONSE"`` on a malformed success. + + Example: + >>> sub = client.subscriptions.get("f72ceac2-8b9a-406a-a57e-90c625785444") + >>> sub.status + 'active' + """ + subscription_id = validate_subscription_id(subscription_id) + response = self.client.request( + method="GET", + path=f"/v1/subscriptions/{subscription_id}", + ) + return unwrap_subscription(response, subject="subscriptions.get") + + def update( + self, + subscription_id: str, + *, + name: Optional[str] = None, + codes: Optional[List[str]] = None, + interval: Optional[Union[str, int]] = None, + deliver_webhook: Optional[bool] = None, + status: Optional[str] = None, + ) -> Subscription: + """Change a subscription. Only the arguments you pass are sent. + + Sent once: a PATCH is not replayed after a timeout or 5xx. If one of + those is raised with ``ambiguous_write=True``, call ``get()`` to see + whether the change landed. + + Args: + subscription_id: The subscription to change. + name: New name. + codes: Replacement list of commodity codes. + interval: Friendly interval ("5m", "1h", "daily") or seconds. + deliver_webhook: Whether events are delivered by webhook. + status: ``"active"`` or ``"paused"``. + + Returns: + The updated Subscription as the server stored it. + + Raises: + ValueError: If the id or any field is invalid, or no field is given. + Nothing is sent. + DataNotFoundError: If the subscription does not exist. + ValidationError: 422 when the server refuses the change, for example + an interval below your plan minimum or webhook delivery your + plan does not include. + + Example: + >>> client.subscriptions.update(sub.id, name="Brent hourly", interval="1h") + """ + subscription_id = validate_subscription_id(subscription_id) + body = build_update_body( + name=name, + codes=codes, + interval=interval, + deliver_webhook=deliver_webhook, + status=status, + ) + response = self.client.request( + method="PATCH", + path=f"/v1/subscriptions/{subscription_id}", + json_data=body, + ) + return unwrap_subscription(response, subject="subscriptions.update") + + def pause(self, subscription_id: str) -> Subscription: + """Pause a subscription so it stops being evaluated. + + Sent once, like every write in this SDK: after an ambiguous timeout or + 5xx, call ``get()`` to check the status rather than retrying blind. + + Returns: + The Subscription, with ``status == "paused"``. + + Example: + >>> client.subscriptions.pause(sub.id).status + 'paused' + """ + subscription_id = validate_subscription_id(subscription_id) + response = self.client.request( + method="POST", + path=f"/v1/subscriptions/{subscription_id}/pause", + ) + return unwrap_subscription(response, subject="subscriptions.pause") + + def resume(self, subscription_id: str) -> Subscription: + """Resume a paused subscription. The server schedules it to run now. + + Returns: + The Subscription, with ``status == "active"`` and the new + ``next_run_at``. + + Example: + >>> client.subscriptions.resume(sub.id).status + 'active' + """ + subscription_id = validate_subscription_id(subscription_id) + response = self.client.request( + method="POST", + path=f"/v1/subscriptions/{subscription_id}/resume", + ) + return unwrap_subscription(response, subject="subscriptions.resume") def delete(self, subscription_id: str) -> bool: """Delete a subscription. @@ -111,9 +229,13 @@ def delete(self, subscription_id: str) -> bool: Returns: True on success. + Raises: + ValueError: If the id is malformed. Nothing is sent. + Example: >>> client.subscriptions.delete(sub.id) """ + subscription_id = validate_subscription_id(subscription_id) self.client.request( method="DELETE", path=f"/v1/subscriptions/{subscription_id}", diff --git a/tests/integration/test_live_subscriptions.py b/tests/integration/test_live_subscriptions.py index 83c1a9e..fafba46 100644 --- a/tests/integration/test_live_subscriptions.py +++ b/tests/integration/test_live_subscriptions.py @@ -6,18 +6,24 @@ unit gate (``--ignore=tests/integration``). They are skipped automatically when the key is absent (e.g. on forks / CI without the secret). -Read-only by default: we fetch a market brief and list subscriptions. No -subscriptions are created or deleted, so nothing is written to production. +The brief and list tests are read-only. ``test_subscription_lifecycle_live`` +writes: it creates ONE watch of its own on the key's account, walks it through +get / update / pause / resume, and deletes it in a ``finally`` that runs even +when an assertion fails. It never reads, changes or deletes any other watch. +A subscription here is an agent "watch", not a billing subscription: nothing +in this lifecycle changes what the account is charged. The API rate limit is 1 request/second, so calls are spaced with small sleeps. """ import os import time +import uuid import pytest from oilpriceapi import MarketBrief, OilPriceAPI +from oilpriceapi.exceptions import DataNotFoundError TEST_KEY = os.environ.get("OILPRICEAPI_TEST_KEY") @@ -55,3 +61,49 @@ def test_subscriptions_list_live(client): """subscriptions.list() returns a list (possibly empty) without error.""" subs = client.subscriptions.list() assert isinstance(subs, list) + + +def test_subscription_lifecycle_live(client): + """create -> get -> update -> pause -> resume -> delete, cleaned up always.""" + name = f"sdk-python-lifecycle-{uuid.uuid4().hex[:12]}" + created = client.subscriptions.create(["BRENT_CRUDE_USD"], interval="daily", name=name) + watch_id = created.id + deleted = False + try: + assert created.name == name + assert created.codes == ["BRENT_CRUDE_USD"] + assert created.interval_seconds == 86400 + time.sleep(RATE_LIMIT_SLEEP) + + fetched = client.subscriptions.get(watch_id) + assert fetched.id == watch_id + assert fetched.created_at == created.created_at + time.sleep(RATE_LIMIT_SLEEP) + + renamed = client.subscriptions.update(watch_id, name=f"{name}-renamed") + assert renamed.id == watch_id + assert renamed.name == f"{name}-renamed" + time.sleep(RATE_LIMIT_SLEEP) + + paused = client.subscriptions.pause(watch_id) + assert paused.status == "paused" + time.sleep(RATE_LIMIT_SLEEP) + + resumed = client.subscriptions.resume(watch_id) + assert resumed.status == "active" + assert resumed.next_run_at is not None + time.sleep(RATE_LIMIT_SLEEP) + + assert client.subscriptions.delete(watch_id) is True + deleted = True + time.sleep(RATE_LIMIT_SLEEP) + + with pytest.raises(DataNotFoundError): + client.subscriptions.get(watch_id) + finally: + if not deleted: + time.sleep(RATE_LIMIT_SLEEP) + try: + client.subscriptions.delete(watch_id) + except DataNotFoundError: + pass diff --git a/tests/unit/test_subscriptions_lifecycle.py b/tests/unit/test_subscriptions_lifecycle.py new file mode 100644 index 0000000..f80387f --- /dev/null +++ b/tests/unit/test_subscriptions_lifecycle.py @@ -0,0 +1,553 @@ +"""Subscription lifecycle: get, update, pause, resume (#100). + +Every test drives the REAL client -- request building, the retry loop, error +normalization, response decoding -- against a mocked transport, patching +`httpx.Client.request` / `httpx.AsyncClient.request` as the rest of this suite +does. Sync and async run the same cases so the two clients cannot drift. + +Wire shapes are the ones production and `V1::SubscriptionsController` return +(verified 2026-09-13): + +* success: ``{"status": "success", "data": {"subscription": {...}}}`` +* unknown id: 404 ``{"error": {"code": "NOT_FOUND", "message": ..., "request_id": ...}}`` +* update validation: 422 ``{"status": "fail", "data": {"error": "VALIDATION_ERROR", "message", "details"}}`` +* watch limit on create: 402 ``{"status": "fail", "data": {"error": "WATCH_LIMIT", ..., "upgrade_url", "upgrade"}}`` +""" + +import asyncio +import json +from unittest.mock import AsyncMock, Mock, patch + +import httpx +import pytest + +from oilpriceapi import AsyncOilPriceAPI, OilPriceAPI, Subscription +from oilpriceapi.exceptions import ( + AuthenticationError, + DataNotFoundError, + OilPriceAPIError, + PaymentRequiredError, + PermissionDeniedError, + RateLimitError, + TimeoutError, + ValidationError, +) + +# Not a credential: a fixture string, every request here is mocked. +FIXTURE_KEY = "-".join(["fixture", "not", "a", "real", "key"]) + +WATCH_ID = "f72ceac2-8b9a-406a-a57e-90c625785444" + +WIRE_SUBSCRIPTION = { + "id": WATCH_ID, + "name": "Brent daily", + "codes": ["BRENT_CRUDE_USD"], + "interval_seconds": 86400, + "status": "active", + "deliver_webhook": False, + "source": "api", + "tool_name": None, + "last_evaluated_at": None, + "next_run_at": "2026-09-13T19:57:22Z", + "created_at": "2026-09-13T19:57:20Z", +} + +MODES = ["sync", "async"] + + +def _response(status, payload=None, *, body=None, headers=None): + response = Mock() + response.status_code = status + response.headers = headers or {} + if body is not None: + response.content = body + response.text = body.decode() + response.json.side_effect = json.JSONDecodeError("Expecting value", "", 0) + else: + text = json.dumps(payload) + response.content = text.encode() + response.text = text + response.json.return_value = payload + return response + + +def _success(subscription): + return _response(200, {"status": "success", "data": {"subscription": subscription}}) + + +def _run(mode, call, *, responses=None, side_effect=None, max_retries=1): + """Run ``call(client)`` on a sync or async client over a mocked transport. + + Returns ``(result, transport_mock)``. Exceptions propagate. + """ + effect = side_effect if side_effect is not None else responses + if mode == "sync": + client = OilPriceAPI(api_key=FIXTURE_KEY, max_retries=max_retries) + with patch("httpx.Client.request") as transport, patch("time.sleep"): + if isinstance(effect, list): + transport.side_effect = effect + elif isinstance(effect, BaseException): + transport.side_effect = effect + else: + transport.return_value = effect + return call(client), transport + + client = AsyncOilPriceAPI(api_key=FIXTURE_KEY, max_retries=max_retries) + + async def go(): + with patch("httpx.AsyncClient.request") as transport, patch( + "asyncio.sleep", new=AsyncMock() + ): + if isinstance(effect, list): + transport.side_effect = effect + elif isinstance(effect, BaseException): + transport.side_effect = effect + else: + transport.return_value = effect + return await call(client), transport + + return asyncio.run(go()) + + +def _run_expect(mode, call, exc_type, **kwargs): + """Like _run, but assert ``exc_type`` is raised; return (exc, transport).""" + captured = {} + if mode == "sync": + client = OilPriceAPI(api_key=FIXTURE_KEY, max_retries=kwargs.get("max_retries", 1)) + effect = kwargs.get("side_effect", kwargs.get("responses")) + with patch("httpx.Client.request") as transport, patch("time.sleep"): + if isinstance(effect, (list, BaseException)): + transport.side_effect = effect + else: + transport.return_value = effect + with pytest.raises(exc_type) as info: + call(client) + captured["transport"] = transport + return info.value, captured["transport"] + + client = AsyncOilPriceAPI(api_key=FIXTURE_KEY, max_retries=kwargs.get("max_retries", 1)) + effect = kwargs.get("side_effect", kwargs.get("responses")) + + async def go(): + with patch("httpx.AsyncClient.request") as transport, patch( + "asyncio.sleep", new=AsyncMock() + ): + if isinstance(effect, (list, BaseException)): + transport.side_effect = effect + else: + transport.return_value = effect + with pytest.raises(exc_type) as info: + await call(client) + return info.value, transport + + return asyncio.run(go()) + + +def _sent(transport): + """The (method, url, json) of the single request the transport received.""" + assert transport.call_count == 1 + kwargs = transport.call_args.kwargs + return kwargs["method"], str(kwargs["url"]), kwargs.get("json") + + +# --------------------------------------------------------------------------- +# Success paths + + +@pytest.mark.parametrize("mode", MODES) +def test_get_returns_typed_subscription_with_wire_values(mode): + sub, transport = _run( + mode, lambda c: c.subscriptions.get(WATCH_ID), responses=_success(WIRE_SUBSCRIPTION) + ) + assert isinstance(sub, Subscription) + assert sub.id == WATCH_ID + assert sub.codes == ["BRENT_CRUDE_USD"] + assert sub.interval_seconds == 86400 + assert sub.status == "active" + # Nulls stay null; timestamps are the server's, not "now". + assert sub.last_evaluated_at is None + assert sub.tool_name is None + assert sub.next_run_at.isoformat() == "2026-09-13T19:57:22+00:00" + assert sub.created_at.isoformat() == "2026-09-13T19:57:20+00:00" + method, url, body = _sent(transport) + assert method == "GET" + assert url.endswith(f"/v1/subscriptions/{WATCH_ID}") + assert body is None + + +@pytest.mark.parametrize("mode", MODES) +def test_update_sends_only_the_given_fields_and_returns_the_server_record(mode): + updated = dict(WIRE_SUBSCRIPTION, name="renamed", interval_seconds=3600) + sub, transport = _run( + mode, + lambda c: c.subscriptions.update(WATCH_ID, name="renamed", interval="1h"), + responses=_success(updated), + ) + assert sub.name == "renamed" + assert sub.interval_seconds == 3600 + method, url, body = _sent(transport) + assert method == "PATCH" + assert url.endswith(f"/v1/subscriptions/{WATCH_ID}") + assert body == {"name": "renamed", "interval_seconds": 3600} + + +@pytest.mark.parametrize("mode", MODES) +def test_update_maps_every_supported_field(mode): + _, transport = _run( + mode, + lambda c: c.subscriptions.update( + WATCH_ID, + codes=["WTI_USD", "BRENT_CRUDE_USD"], + deliver_webhook=False, + status="paused", + ), + responses=_success(dict(WIRE_SUBSCRIPTION, status="paused")), + ) + _, _, body = _sent(transport) + assert body == { + "codes": ["WTI_USD", "BRENT_CRUDE_USD"], + "deliver_webhook": False, + "status": "paused", + } + + +@pytest.mark.parametrize("mode", MODES) +def test_pause_posts_member_action_and_returns_paused(mode): + sub, transport = _run( + mode, + lambda c: c.subscriptions.pause(WATCH_ID), + responses=_success(dict(WIRE_SUBSCRIPTION, status="paused")), + ) + assert sub.status == "paused" + method, url, body = _sent(transport) + assert method == "POST" + assert url.endswith(f"/v1/subscriptions/{WATCH_ID}/pause") + assert body is None + + +@pytest.mark.parametrize("mode", MODES) +def test_resume_posts_member_action_and_returns_active(mode): + sub, transport = _run( + mode, + lambda c: c.subscriptions.resume(WATCH_ID), + responses=_success(WIRE_SUBSCRIPTION), + ) + assert sub.status == "active" + method, url, _ = _sent(transport) + assert method == "POST" + assert url.endswith(f"/v1/subscriptions/{WATCH_ID}/resume") + + +# --------------------------------------------------------------------------- +# Pre-network validation: nothing is sent + + +BAD_IDS = ["", " ", None, 42, "abc/pause", "../webhooks", "a?b=1", "a#b", " abc"] + + +@pytest.mark.parametrize("mode", MODES) +@pytest.mark.parametrize("bad_id", BAD_IDS) +@pytest.mark.parametrize("action", ["get", "pause", "resume", "delete"]) +def test_invalid_id_is_rejected_before_the_network(mode, bad_id, action): + exc, transport = _run_expect( + mode, + lambda c: getattr(c.subscriptions, action)(bad_id), + ValueError, + responses=_success(WIRE_SUBSCRIPTION), + ) + assert transport.call_count == 0 + + +@pytest.mark.parametrize("mode", MODES) +@pytest.mark.parametrize("bad_id", BAD_IDS) +def test_update_invalid_id_is_rejected_before_the_network(mode, bad_id): + _, transport = _run_expect( + mode, + lambda c: c.subscriptions.update(bad_id, name="x"), + ValueError, + responses=_success(WIRE_SUBSCRIPTION), + ) + assert transport.call_count == 0 + + +BAD_UPDATES = [ + pytest.param({}, id="empty-payload"), + pytest.param({"interval": "5x"}, id="bad-interval"), + pytest.param({"interval": 0}, id="zero-interval"), + pytest.param({"deliver_webhook": "yes"}, id="non-bool-deliver_webhook"), + pytest.param({"deliver_webhook": 1}, id="int-deliver_webhook"), + pytest.param({"status": "cancelled"}, id="unknown-status"), + pytest.param({"status": "ACTIVE "}, id="unnormalized-status"), + pytest.param({"codes": []}, id="empty-codes"), + pytest.param({"codes": "BRENT_CRUDE_USD"}, id="codes-as-string"), + pytest.param({"codes": ["BRENT_CRUDE_USD", ""]}, id="blank-code"), + pytest.param({"codes": ["BRENT_CRUDE_USD", 5]}, id="non-string-code"), + pytest.param({"name": 5}, id="non-string-name"), +] + + +@pytest.mark.parametrize("mode", MODES) +@pytest.mark.parametrize("fields", BAD_UPDATES) +def test_invalid_update_payload_is_rejected_before_the_network(mode, fields): + _, transport = _run_expect( + mode, + lambda c: c.subscriptions.update(WATCH_ID, **fields), + ValueError, + responses=_success(WIRE_SUBSCRIPTION), + ) + assert transport.call_count == 0 + + +# --------------------------------------------------------------------------- +# Server errors keep their type and recovery metadata + + +NOT_FOUND = { + "error": { + "code": "NOT_FOUND", + "message": "Subscription not found", + "status": 404, + "request_id": "f34f0d7d-3a1c-464f-b951-27163cf0aae1", + "docs": "https://docs.oilpriceapi.com#NOT_FOUND", + } +} + + +@pytest.mark.parametrize("mode", MODES) +@pytest.mark.parametrize( + "call", + [ + pytest.param(lambda c: c.subscriptions.get(WATCH_ID), id="get"), + pytest.param(lambda c: c.subscriptions.update(WATCH_ID, name="x"), id="update"), + pytest.param(lambda c: c.subscriptions.pause(WATCH_ID), id="pause"), + pytest.param(lambda c: c.subscriptions.resume(WATCH_ID), id="resume"), + ], +) +def test_unknown_id_raises_data_not_found_with_request_id(mode, call): + exc, transport = _run_expect(mode, call, DataNotFoundError, responses=_response(404, NOT_FOUND)) + assert exc.status_code == 404 + assert exc.code == "NOT_FOUND" + assert exc.request_id == "f34f0d7d-3a1c-464f-b951-27163cf0aae1" + assert transport.call_count == 1 + + +@pytest.mark.parametrize("mode", MODES) +def test_update_422_raises_validation_error_with_details(mode): + payload = { + "status": "fail", + "data": { + "error": "VALIDATION_ERROR", + "message": "Interval seconds is below your plan minimum of 3600 seconds", + "details": {"interval_seconds": ["is below your plan minimum of 3600 seconds"]}, + }, + } + exc, _ = _run_expect( + mode, + lambda c: c.subscriptions.update(WATCH_ID, interval=60), + ValidationError, + responses=_response(422, payload), + ) + assert exc.status_code == 422 + assert exc.code == "VALIDATION_ERROR" + assert "plan minimum" in str(exc) + assert exc.raw_body["data"]["details"]["interval_seconds"] + + +@pytest.mark.parametrize("mode", MODES) +def test_update_webhook_entitlement_refusal_is_a_422_not_a_success(mode): + """The Watch model refuses deliver_webhook without the entitlement.""" + payload = { + "status": "fail", + "data": { + "error": "VALIDATION_ERROR", + "message": "Deliver webhook requires a plan with webhook delivery", + "details": {"deliver_webhook": ["requires a plan with webhook delivery"]}, + }, + } + exc, _ = _run_expect( + mode, + lambda c: c.subscriptions.update(WATCH_ID, deliver_webhook=True), + ValidationError, + responses=_response(422, payload), + ) + assert "webhook delivery" in str(exc) + + +UPGRADE_URL = ( + "https://www.oilpriceapi.com/pricing?plan=starter" + "&utm_source=api&utm_medium=agent&utm_campaign=agent_watches" +) +WATCH_LIMIT = { + "status": "fail", + "data": { + "error": "WATCH_LIMIT", + "message": "Your plan allows up to 1 active watches. Upgrade for more.", + "limit": 1, + "current": 1, + "upgrade_trigger": "watch_limit", + "upgrade_url": UPGRADE_URL, + "upgrade": {"url": UPGRADE_URL, "next_tier": "starter", "plans": []}, + }, +} + + +@pytest.mark.parametrize("mode", MODES) +def test_create_watch_limit_402_keeps_upgrade_recovery_metadata(mode): + exc, transport = _run_expect( + mode, + lambda c: c.subscriptions.create(["BRENT_CRUDE_USD"], interval="daily"), + PaymentRequiredError, + responses=_response(402, WATCH_LIMIT), + ) + assert exc.status_code == 402 + assert exc.code == "WATCH_LIMIT" + assert exc.remediation_url == UPGRADE_URL + assert exc.raw_body["data"]["upgrade"]["next_tier"] == "starter" + assert transport.call_count == 1 + + +@pytest.mark.parametrize("mode", MODES) +@pytest.mark.parametrize( + "status,exc_type", + [(401, AuthenticationError), (403, PermissionDeniedError), (429, RateLimitError)], +) +def test_auth_permission_and_rate_limit_are_typed(mode, status, exc_type): + payload = {"error": {"code": "X", "message": "refused", "status": status}} + headers = {"Retry-After": "0"} if status == 429 else {} + exc, _ = _run_expect( + mode, + lambda c: c.subscriptions.pause(WATCH_ID), + exc_type, + responses=_response(status, payload, headers=headers), + ) + assert exc.status_code == status + + +@pytest.mark.parametrize("mode", MODES) +def test_get_429_then_success_recovers(mode): + limited = _response(429, {"error": {"code": "RATE_LIMITED", "message": "slow"}}, headers={"Retry-After": "0"}) + sub, transport = _run( + mode, + lambda c: c.subscriptions.get(WATCH_ID), + responses=[limited, _success(WIRE_SUBSCRIPTION)], + max_retries=2, + ) + assert sub.id == WATCH_ID + assert transport.call_count == 2 + + +# --------------------------------------------------------------------------- +# Malformed successes are reported, never laundered + + +MALFORMED_BODIES = [ + pytest.param({"status": "success", "data": {}}, id="data-without-subscription"), + pytest.param({"status": "success", "data": {"subscriptions": [WIRE_SUBSCRIPTION]}}, id="list-shape"), + pytest.param({"status": "success", "data": {"subscription": None}}, id="null-subscription"), + pytest.param({"status": "success", "data": {"subscription": [WIRE_SUBSCRIPTION]}}, id="subscription-is-list"), + pytest.param( + {"status": "success", "data": {"subscription": {k: v for k, v in WIRE_SUBSCRIPTION.items() if k != "id"}}}, + id="missing-id", + ), + pytest.param( + {"status": "success", "data": {"subscription": {k: v for k, v in WIRE_SUBSCRIPTION.items() if k != "codes"}}}, + id="missing-codes", + ), + pytest.param({"status": "success", "data": WIRE_SUBSCRIPTION}, id="unwrapped-record"), + pytest.param({}, id="empty-object"), + pytest.param([WIRE_SUBSCRIPTION], id="bare-list"), +] + + +@pytest.mark.parametrize("mode", MODES) +@pytest.mark.parametrize("payload", MALFORMED_BODIES) +@pytest.mark.parametrize( + "call", + [ + pytest.param(lambda c: c.subscriptions.get(WATCH_ID), id="get"), + pytest.param(lambda c: c.subscriptions.update(WATCH_ID, name="x"), id="update"), + pytest.param(lambda c: c.subscriptions.pause(WATCH_ID), id="pause"), + pytest.param(lambda c: c.subscriptions.resume(WATCH_ID), id="resume"), + pytest.param( + lambda c: c.subscriptions.create(["BRENT_CRUDE_USD"], interval="daily"), id="create" + ), + ], +) +def test_malformed_success_raises_malformed_response(mode, payload, call): + exc, _ = _run_expect(mode, call, OilPriceAPIError, responses=_response(200, payload)) + assert exc.code == "MALFORMED_RESPONSE" + assert exc.raw_body == payload + + +@pytest.mark.parametrize("mode", MODES) +def test_non_json_200_is_a_parse_failure_not_an_empty_success(mode): + _run_expect( + mode, + lambda c: c.subscriptions.get(WATCH_ID), + ValueError, + responses=_response(200, body=b"gateway"), + ) + + +# --------------------------------------------------------------------------- +# Timeouts: reads recover, writes are sent once and flagged ambiguous + + +@pytest.mark.parametrize("mode", MODES) +def test_get_timeout_raises_timeout_error(mode): + exc, transport = _run_expect( + mode, + lambda c: c.subscriptions.get(WATCH_ID), + TimeoutError, + side_effect=httpx.TimeoutException("timed out"), + max_retries=2, + ) + assert transport.call_count == 2 + + +@pytest.mark.parametrize("mode", MODES) +def test_get_timeout_then_success_recovers(mode): + sub, transport = _run( + mode, + lambda c: c.subscriptions.get(WATCH_ID), + side_effect=[httpx.TimeoutException("timed out"), _success(WIRE_SUBSCRIPTION)], + max_retries=2, + ) + assert sub.id == WATCH_ID + assert transport.call_count == 2 + + +@pytest.mark.parametrize("mode", MODES) +@pytest.mark.parametrize( + "call", + [ + pytest.param(lambda c: c.subscriptions.update(WATCH_ID, name="x"), id="update"), + pytest.param(lambda c: c.subscriptions.pause(WATCH_ID), id="pause"), + pytest.param(lambda c: c.subscriptions.resume(WATCH_ID), id="resume"), + ], +) +def test_write_timeout_is_sent_once_and_marked_ambiguous(mode, call): + exc, transport = _run_expect( + mode, + call, + TimeoutError, + side_effect=httpx.TimeoutException("timed out"), + max_retries=3, + ) + assert transport.call_count == 1 + assert getattr(exc, "ambiguous_write", False) is True + + +# --------------------------------------------------------------------------- +# Model contract + + +def test_subscription_model_requires_codes_rather_than_fabricating_empty(): + """A record missing ``codes`` is malformed, not a watch on nothing.""" + record = {k: v for k, v in WIRE_SUBSCRIPTION.items() if k != "codes"} + with pytest.raises(Exception): + Subscription(**record) + + +def test_subscription_model_keeps_explicit_empty_codes(): + assert Subscription(**dict(WIRE_SUBSCRIPTION, codes=[])).codes == [] From e7bc922a90c42d4eb8b9004741609d402a0c6144 Mon Sep 17 00:00:00 2001 From: Karl Waldman Date: Sun, 13 Sep 2026 16:26:35 -0400 Subject: [PATCH 2/2] fix(subscriptions): local refusals raise ValidationError, not ValueError (#100) Review follow-up on #143. Every local input refusal in the subscription lifecycle now uses the repo's ValidationError convention, matching _url._reject: field=, value=, status_code=None because no request was sent. - get/update/pause/resume and the new delete() id check raise a plain ValidationError (new behaviour, no ValueError contract to keep) - the interval path that raised ValueError on origin/main -- subscriptions.create(interval=...), normalize_interval, build_create_body -- raises the new SubscriptionIntervalError(ValidationError, ValueError), modelled on FuturesContractError, so `except ValueError` keeps working - SubscriptionIntervalError exported from oilpriceapi - tests assert the exact type, status_code is None and field Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- CHANGELOG.md | 14 +++- oilpriceapi/__init__.py | 2 + oilpriceapi/_subscriptions_common.py | 83 +++++++++++++++------- oilpriceapi/exceptions.py | 19 +++++ oilpriceapi/resources/subscriptions.py | 11 +-- tests/unit/test_subscriptions_lifecycle.py | 74 +++++++++++++------ tests/unit/test_subscriptions_resource.py | 20 +++++- 7 files changed, 171 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f959db..61b0385 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,8 @@ All notable changes to the OilPriceAPI Python SDK will be documented in this fil `Subscription` with the server's timestamps and nulls as sent. `update()` sends only the fields you pass (`name`, `codes`, `interval`, `deliver_webhook`, `status`). Ids and update payloads are validated before - any request is built, and an invalid one raises `ValueError` with nothing - sent. Unknown ids raise `DataNotFoundError`; a refused update (interval + any request is built; an invalid one raises `ValidationError` with + `status_code=None`, `field` naming the argument, and nothing sent. Unknown ids raise `DataNotFoundError`; a refused update (interval below the plan minimum, webhook delivery the plan lacks) raises `ValidationError` with the server's `details`. `update`, `pause` and `resume` are writes and are sent once, like `create`: after an ambiguous @@ -29,7 +29,15 @@ All notable changes to the OilPriceAPI Python SDK will be documented in this fil `OilPriceAPIError(code="MALFORMED_RESPONSE")`, the same as the new lifecycle methods. - **`subscriptions.delete()` validates the id before sending.** An id such as - `"abc/pause"` or `""` previously produced a request to a different route. + `"abc/pause"` or `""` previously produced a request to a different route; it + now raises `ValidationError(field="subscription_id", status_code=None)`. +- **A bad subscription `interval` is now an SDK refusal as well as a + `ValueError`.** `subscriptions.create(interval=...)`, `normalize_interval` and + `build_create_body` raise the new `SubscriptionIntervalError`, a subclass of + both `ValidationError` and `ValueError` (like `FuturesContractError`), so + `except OilPriceAPIError` catches it and existing `except ValueError` code + keeps working. It carries `field="interval"`, the rejected `value`, and + `status_code=None`. - **`Subscription.codes` is required.** A record with no `codes` used to default to `[]`, reading as a watch on nothing; the API always sends it, so a missing value now fails validation instead of being invented. diff --git a/oilpriceapi/__init__.py b/oilpriceapi/__init__.py index 8640979..0433434 100644 --- a/oilpriceapi/__init__.py +++ b/oilpriceapi/__init__.py @@ -25,6 +25,7 @@ PermissionDeniedError, RateLimitError, ServerError, + SubscriptionIntervalError, TimeoutError, ValidationError, ) @@ -63,6 +64,7 @@ "DataNotFoundError", "ServerError", "FuturesContractError", + "SubscriptionIntervalError", "ValidationError", "NetworkError", "TimeoutError", diff --git a/oilpriceapi/_subscriptions_common.py b/oilpriceapi/_subscriptions_common.py index d738a85..2c96b1a 100644 --- a/oilpriceapi/_subscriptions_common.py +++ b/oilpriceapi/_subscriptions_common.py @@ -10,6 +10,8 @@ import re from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union +from .exceptions import SubscriptionIntervalError, ValidationError + if TYPE_CHECKING: from .models import Subscription @@ -35,6 +37,19 @@ _UNIT_SECONDS = {"s": 1, "m": 60, "h": 3600, "d": 86400} +def _refuse(message: str, field: Optional[str], value: Any) -> ValidationError: + """A local input refusal. No request was sent, so there is no HTTP status.""" + return ValidationError(message=message, field=field, value=value, status_code=None) + + +def _interval_refusal(message: str, interval: Any) -> SubscriptionIntervalError: + # Dual-base (ValidationError + ValueError): this path raised ValueError + # before #100, and callers may catch that. + return SubscriptionIntervalError( + message=message, field="interval", value=interval, status_code=None + ) + + def normalize_interval(interval: Union[str, int]) -> int: """Convert a friendly interval into ``interval_seconds``. @@ -42,17 +57,19 @@ def normalize_interval(interval: Union[str, int]) -> int: ```` string where unit is one of s/m/h/d (e.g. "30s", "2h"). Raises: - ValueError: If the interval cannot be parsed or is non-positive. + SubscriptionIntervalError: If the interval cannot be parsed or is + non-positive. It is both a ``ValidationError`` (``field="interval"``, + ``status_code=None``) and, for compatibility, a ``ValueError``. """ if isinstance(interval, bool): # bool is an int subclass; reject explicitly - raise ValueError(f"Invalid interval: {interval!r}") + raise _interval_refusal(f"Invalid interval: {interval!r}", interval) if isinstance(interval, int): if interval <= 0: - raise ValueError(f"interval_seconds must be positive, got {interval}") + raise _interval_refusal(f"interval_seconds must be positive, got {interval}", interval) return interval if not isinstance(interval, str): - raise ValueError(f"Invalid interval type: {type(interval).__name__}") + raise _interval_refusal(f"Invalid interval type: {type(interval).__name__}", interval) key = interval.strip().lower() if key in _INTERVAL_ALIASES: @@ -62,19 +79,20 @@ def normalize_interval(interval: Union[str, int]) -> int: if key.isdigit(): seconds = int(key) if seconds <= 0: - raise ValueError(f"interval_seconds must be positive, got {seconds}") + raise _interval_refusal(f"interval_seconds must be positive, got {seconds}", interval) return seconds # form e.g. "45s", "2h". if len(key) >= 2 and key[-1] in _UNIT_SECONDS and key[:-1].isdigit(): seconds = int(key[:-1]) * _UNIT_SECONDS[key[-1]] if seconds <= 0: - raise ValueError(f"interval_seconds must be positive, got {seconds}") + raise _interval_refusal(f"interval_seconds must be positive, got {seconds}", interval) return seconds - raise ValueError( + raise _interval_refusal( f"Unrecognized interval {interval!r}. Use seconds (int), a named alias " - f"('5m', '1h', 'daily'), or '' where unit is s/m/h/d." + f"('5m', '1h', 'daily'), or '' where unit is s/m/h/d.", + interval, ) @@ -120,13 +138,16 @@ def validate_subscription_id(subscription_id: Any) -> str: """Return ``subscription_id`` if it can be placed in a URL path segment. Raises: - ValueError: If the id is not a non-empty string of letters, digits, - ``-`` or ``_``. Nothing is sent to the API. + ValidationError: ``field="subscription_id"``, ``status_code=None``, if + the id is not a non-empty string of letters, digits, ``-`` or + ``_``. Nothing is sent to the API. """ if not isinstance(subscription_id, str) or not _SUBSCRIPTION_ID.fullmatch(subscription_id): - raise ValueError( + raise _refuse( f"Invalid subscription id {subscription_id!r}: expected the id returned " - f"by subscriptions.list() or subscriptions.create()." + f"by subscriptions.list() or subscriptions.create().", + "subscription_id", + subscription_id, ) return subscription_id @@ -144,38 +165,52 @@ def build_update_body( field the caller did not mention. Raises: - ValueError: If no field is given or a field is invalid. Nothing is sent. + ValidationError: ``status_code=None``, with ``field`` naming the invalid + argument (``None`` when no field is given at all). Nothing is sent. """ body: Dict[str, Any] = {} if name is not None: if not isinstance(name, str): - raise ValueError(f"name must be a string, got {type(name).__name__}") + raise _refuse(f"name must be a string, got {type(name).__name__}", "name", name) body["name"] = name if codes is not None: if isinstance(codes, (str, bytes)) or not isinstance(codes, (list, tuple)): - raise ValueError("codes must be a list of commodity codes, e.g. ['BRENT_CRUDE_USD']") + raise _refuse( + "codes must be a list of commodity codes, e.g. ['BRENT_CRUDE_USD']", "codes", codes + ) if not codes: - raise ValueError("codes must contain at least one commodity code") + raise _refuse("codes must contain at least one commodity code", "codes", codes) if not all(isinstance(code, str) and code.strip() for code in codes): - raise ValueError("every code must be a non-empty string") + raise _refuse("every code must be a non-empty string", "codes", codes) body["codes"] = list(codes) if interval is not None: - body["interval_seconds"] = normalize_interval(interval) + try: + body["interval_seconds"] = normalize_interval(interval) + except SubscriptionIntervalError as error: + # update() is new in #100: no caller relies on ValueError here, so + # it gets the plain ValidationError every new refusal uses. + raise _refuse(error.message, "interval", interval) from None if deliver_webhook is not None: if not isinstance(deliver_webhook, bool): - raise ValueError( - f"deliver_webhook must be True or False, got {deliver_webhook!r}" + raise _refuse( + f"deliver_webhook must be True or False, got {deliver_webhook!r}", + "deliver_webhook", + deliver_webhook, ) body["deliver_webhook"] = deliver_webhook if status is not None: if status not in VALID_STATUSES: - raise ValueError( - f"status must be one of {', '.join(VALID_STATUSES)}, got {status!r}" + raise _refuse( + f"status must be one of {', '.join(VALID_STATUSES)}, got {status!r}", + "status", + status, ) body["status"] = status if not body: - raise ValueError( - "update() needs at least one of: name, codes, interval, deliver_webhook, status" + raise _refuse( + "update() needs at least one of: name, codes, interval, deliver_webhook, status", + None, + None, ) return body diff --git a/oilpriceapi/exceptions.py b/oilpriceapi/exceptions.py index 7329ede..9fae953 100644 --- a/oilpriceapi/exceptions.py +++ b/oilpriceapi/exceptions.py @@ -396,6 +396,25 @@ def __str__(self) -> str: return self.message +class SubscriptionIntervalError(ValidationError, ValueError): + """Raised locally when a subscription interval cannot be parsed (#100). + + Two base classes, deliberately: + + * ``ValidationError`` -- so ``except OilPriceAPIError`` catches it, like + every other refusal in this SDK. It is raised before any request is + built, so ``status_code`` is ``None``, ``field`` is ``"interval"`` and + ``value`` is the rejected input. + * ``ValueError`` -- so code written against the pre-#100 ``raise + ValueError`` from ``subscriptions.create(interval=...)`` and + ``normalize_interval`` keeps working. This is not a breaking change. + + Only the interval path that already raised ``ValueError`` gets the dual + base. Refusals introduced with ``get``/``update``/``pause``/``resume`` and + the ``delete`` id check raise a plain ``ValidationError``. + """ + + class ServerError(OilPriceAPIError): """Raised when the server returns HTTP 5xx.""" diff --git a/oilpriceapi/resources/subscriptions.py b/oilpriceapi/resources/subscriptions.py index 1c90895..eca4316 100644 --- a/oilpriceapi/resources/subscriptions.py +++ b/oilpriceapi/resources/subscriptions.py @@ -113,7 +113,8 @@ def get(self, subscription_id: str) -> Subscription: The Subscription, with the server's timestamps and nulls as sent. Raises: - ValueError: If the id is malformed. Nothing is sent. + ValidationError: If the id is malformed (``field="subscription_id"``, + ``status_code=None``). Nothing is sent. DataNotFoundError: If no subscription with that id belongs to you. OilPriceAPIError: ``code="MALFORMED_RESPONSE"`` on a malformed success. @@ -157,8 +158,9 @@ def update( The updated Subscription as the server stored it. Raises: - ValueError: If the id or any field is invalid, or no field is given. - Nothing is sent. + ValidationError: ``status_code=None``, ``field`` naming the argument, + if the id or any field is invalid, or no field is given. Nothing + is sent. (Distinct from the server's 422, which has a status.) DataNotFoundError: If the subscription does not exist. ValidationError: 422 when the server refuses the change, for example an interval below your plan minimum or webhook delivery your @@ -230,7 +232,8 @@ def delete(self, subscription_id: str) -> bool: True on success. Raises: - ValueError: If the id is malformed. Nothing is sent. + ValidationError: If the id is malformed (``field="subscription_id"``, + ``status_code=None``). Nothing is sent. Example: >>> client.subscriptions.delete(sub.id) diff --git a/tests/unit/test_subscriptions_lifecycle.py b/tests/unit/test_subscriptions_lifecycle.py index f80387f..9f95b77 100644 --- a/tests/unit/test_subscriptions_lifecycle.py +++ b/tests/unit/test_subscriptions_lifecycle.py @@ -29,6 +29,7 @@ PaymentRequiredError, PermissionDeniedError, RateLimitError, + SubscriptionIntervalError, TimeoutError, ValidationError, ) @@ -252,49 +253,82 @@ def test_invalid_id_is_rejected_before_the_network(mode, bad_id, action): exc, transport = _run_expect( mode, lambda c: getattr(c.subscriptions, action)(bad_id), - ValueError, + ValidationError, responses=_success(WIRE_SUBSCRIPTION), ) + # A plain ValidationError: these refusals are new in #100, so there is no + # ValueError contract to keep. No request was sent, so no HTTP status. + assert type(exc) is ValidationError + assert exc.status_code is None + assert exc.field == "subscription_id" + assert exc.value == bad_id assert transport.call_count == 0 @pytest.mark.parametrize("mode", MODES) @pytest.mark.parametrize("bad_id", BAD_IDS) def test_update_invalid_id_is_rejected_before_the_network(mode, bad_id): - _, transport = _run_expect( + exc, transport = _run_expect( mode, lambda c: c.subscriptions.update(bad_id, name="x"), - ValueError, + ValidationError, responses=_success(WIRE_SUBSCRIPTION), ) + assert type(exc) is ValidationError + assert exc.status_code is None + assert exc.field == "subscription_id" assert transport.call_count == 0 BAD_UPDATES = [ - pytest.param({}, id="empty-payload"), - pytest.param({"interval": "5x"}, id="bad-interval"), - pytest.param({"interval": 0}, id="zero-interval"), - pytest.param({"deliver_webhook": "yes"}, id="non-bool-deliver_webhook"), - pytest.param({"deliver_webhook": 1}, id="int-deliver_webhook"), - pytest.param({"status": "cancelled"}, id="unknown-status"), - pytest.param({"status": "ACTIVE "}, id="unnormalized-status"), - pytest.param({"codes": []}, id="empty-codes"), - pytest.param({"codes": "BRENT_CRUDE_USD"}, id="codes-as-string"), - pytest.param({"codes": ["BRENT_CRUDE_USD", ""]}, id="blank-code"), - pytest.param({"codes": ["BRENT_CRUDE_USD", 5]}, id="non-string-code"), - pytest.param({"name": 5}, id="non-string-name"), + pytest.param({}, None, id="empty-payload"), + pytest.param({"interval": "5x"}, "interval", id="bad-interval"), + pytest.param({"interval": 0}, "interval", id="zero-interval"), + pytest.param({"deliver_webhook": "yes"}, "deliver_webhook", id="non-bool-deliver_webhook"), + pytest.param({"deliver_webhook": 1}, "deliver_webhook", id="int-deliver_webhook"), + pytest.param({"status": "cancelled"}, "status", id="unknown-status"), + pytest.param({"status": "ACTIVE "}, "status", id="unnormalized-status"), + pytest.param({"codes": []}, "codes", id="empty-codes"), + pytest.param({"codes": "BRENT_CRUDE_USD"}, "codes", id="codes-as-string"), + pytest.param({"codes": ["BRENT_CRUDE_USD", ""]}, "codes", id="blank-code"), + pytest.param({"codes": ["BRENT_CRUDE_USD", 5]}, "codes", id="non-string-code"), + pytest.param({"name": 5}, "name", id="non-string-name"), ] @pytest.mark.parametrize("mode", MODES) -@pytest.mark.parametrize("fields", BAD_UPDATES) -def test_invalid_update_payload_is_rejected_before_the_network(mode, fields): - _, transport = _run_expect( +@pytest.mark.parametrize("fields,field", BAD_UPDATES) +def test_invalid_update_payload_is_rejected_before_the_network(mode, fields, field): + exc, transport = _run_expect( mode, lambda c: c.subscriptions.update(WATCH_ID, **fields), - ValueError, + ValidationError, + responses=_success(WIRE_SUBSCRIPTION), + ) + assert type(exc) is ValidationError + assert exc.status_code is None + assert exc.field == field + if field is not None: + assert exc.value == fields[field] + assert transport.call_count == 0 + + +@pytest.mark.parametrize("mode", MODES) +@pytest.mark.parametrize("bad_interval", ["5x", 0, -60, True, "", 1.5]) +def test_create_bad_interval_keeps_value_error_and_is_a_validation_error(mode, bad_interval): + """create() raised ValueError for a bad interval before #100; keep that contract.""" + exc, transport = _run_expect( + mode, + lambda c: c.subscriptions.create(["BRENT_CRUDE_USD"], interval=bad_interval), + SubscriptionIntervalError, responses=_success(WIRE_SUBSCRIPTION), ) + assert isinstance(exc, ValidationError) + assert isinstance(exc, ValueError) + assert isinstance(exc, OilPriceAPIError) + assert exc.status_code is None + assert exc.field == "interval" + assert exc.value is bad_interval assert transport.call_count == 0 @@ -484,7 +518,7 @@ def test_non_json_200_is_a_parse_failure_not_an_empty_success(mode): _run_expect( mode, lambda c: c.subscriptions.get(WATCH_ID), - ValueError, + json.JSONDecodeError, responses=_response(200, body=b"gateway"), ) diff --git a/tests/unit/test_subscriptions_resource.py b/tests/unit/test_subscriptions_resource.py index 5f0edf5..861ca61 100644 --- a/tests/unit/test_subscriptions_resource.py +++ b/tests/unit/test_subscriptions_resource.py @@ -47,8 +47,26 @@ def test_normalize_interval(self, value, expected): @pytest.mark.parametrize("bad", ["", "abc", "0", 0, -5, "-1h", "5x", True]) def test_normalize_interval_invalid(self, bad): - with pytest.raises(ValueError): + with pytest.raises(ValueError) as info: normalize_interval(bad) + # Still a ValueError for existing callers, and also an SDK refusal: + # catchable as OilPriceAPIError, local (no HTTP status), naming the field. + from oilpriceapi import OilPriceAPIError, SubscriptionIntervalError, ValidationError + + assert isinstance(info.value, SubscriptionIntervalError) + assert isinstance(info.value, ValidationError) + assert isinstance(info.value, OilPriceAPIError) + assert info.value.status_code is None + assert info.value.field == "interval" + + @pytest.mark.parametrize("bad", ["abc", 0]) + def test_build_create_body_bad_interval_keeps_value_error(self, bad): + with pytest.raises(ValueError) as info: + build_create_body(["BRENT_CRUDE_USD"], bad) + from oilpriceapi import SubscriptionIntervalError + + assert isinstance(info.value, SubscriptionIntervalError) + assert info.value.field == "interval" def test_build_create_body(self): body = build_create_body(["BRENT_CRUDE_USD"], "5m", name="Brent")