fix(errors): keep a fail-envelope sentence out of error.code (#145)

CHANGELOG.md

@@ -71,6 +71,17 @@ All notable changes to the OilPriceAPI Python SDK will be documented in this fil
 
 ### Fixed
 
+- **`error.code` is no longer set to a human sentence (#145).** For fail
+  envelopes, `{"status": "fail", "data": {"error": ...}}`, the SDK copied
+  `data.error` into `error.code` / `error.machine_code` whatever it held. Every
+  fuel-surcharge 400/404 puts the sentence itself there, for example
+  `"Unknown carrier 'nope'. Covered carriers: ..."`, so code-based branching
+  saw a different string on every request. `data.error` is now the code only
+  when it is a snake-case token: upper-snake like `VALIDATION_ERROR` or
+  `INTERVAL_FLOOR`, or lower-snake like `invalid_code` or `no_price_data`. A
+  sentence stays in `error.message` and `error.code` is `None`. A canonical
+  nested `error` object still takes precedence, and API-key redaction is
+  unchanged.
 - **`subscriptions.list()` and `subscriptions.events()` no longer report a
   malformed success as "nothing there" (#142), sync and async.** A 200 without
   a `data.subscriptions` list returned `[]`, and one without `data.events` /

oilpriceapi/exceptions.py

@@ -115,6 +115,21 @@ def _first_value(sources: Iterable[Mapping[str, Any]], *keys: str) -> Any:
     return None
 
 
+def _is_machine_code(value: Any) -> bool:
+    """True for a snake-case token such as ``VALIDATION_ERROR`` or ``invalid_code``.
+
+    The API sends both cases in ``data.error`` (``render_fail``): upper-snake
+    from the subscription and upgrade-trigger routes, lower-snake from
+    ``api_validations.rb`` and ``prices_controller.rb``. A sentence -- anything
+    with spaces or punctuation, or a single word -- is not a code.
+    """
+    if not isinstance(value, str) or "_" not in value or not value[:1].isalpha():
+        return False
+    if not all(part.isascii() and part.isalnum() for part in value.split("_")):
+        return False
+    return value.isupper() or value.islower()
+
+
 def _number(value: Any) -> Any:
     if value is None:
         return None
@@ -510,7 +525,11 @@ def error_from_response(
     message = str(message_value or raw_text.strip() or f"HTTP {status_code} error")
 
     code_value = _first_value(sources, "code", "error_code", "type")
-    if code_value is None and isinstance(nested_data_error, str):
+    # In the fail envelope `data.error` is either a machine code (the sentence is
+    # in `data.message`) or the sentence itself, as every fuel-surcharge 400/404
+    # sends. Only a code-shaped value is a code; a sentence stays the message
+    # and never becomes `error.code` (#145).
+    if code_value is None and _is_machine_code(nested_data_error):
         code_value = nested_data_error
     request_id_value = _first_value(
         sources,

tests/unit/test_fail_envelope_error_code.py

@@ -0,0 +1,271 @@
+"""#145 -- a human sentence in ``data.error`` is a message, never ``error.code``.
+
+The API answers many failures with the JSend fail envelope,
+``{"status": "fail", "data": {"error": ..., "message"?: ...}}`` (``render_fail``
+in ``V1::BaseController``). ``data.error`` holds one of two things:
+
+* a machine code, with the sentence in ``data.message``: ``VALIDATION_ERROR``,
+  ``INTERVAL_FLOOR``, ``WATCH_LIMIT``, and the lower-snake ``invalid_code``,
+  ``no_price_data`` and ``invalid_request`` that ``api_validations.rb`` and
+  ``prices_controller.rb`` send (origin/main, 2026-09-13);
+* the sentence itself, as every ``V1::FuelSurchargeController`` 400/404 does.
+
+``error_from_response`` copied either one into ``error.code``, so a caller
+branching on ``code`` saw a unique free-text string per request.
+
+Bodies marked "live" are verbatim from ``api.oilpriceapi.com`` on 2026-09-13.
+Client tests drive the REAL sync and async clients over a mocked transport.
+"""
+
+import asyncio
+from unittest.mock import AsyncMock, patch
+
+import httpx
+import pytest
+
+from oilpriceapi import AsyncOilPriceAPI, OilPriceAPI
+from oilpriceapi.exceptions import (
+    AuthenticationError,
+    BadRequestError,
+    DataNotFoundError,
+    OilPriceAPIError,
+    PaymentRequiredError,
+    PermissionDeniedError,
+    RateLimitError,
+    ServerError,
+    ValidationError,
+    error_from_response,
+)
+
+# Not a credential: a fixture string. Every request here is mocked.
+FIXTURE_KEY = "-".join(["fixture", "not", "a", "real", "key"])
+
+MODES = ["sync", "async"]
+
+# live: GET /v1/fuel-surcharge/nope/latest
+UNKNOWN_CARRIER_404 = {
+    "status": "fail",
+    "data": {
+        "error": "Unknown carrier 'nope'. Covered carriers: odfl, saia, estes, xpo, abf, tforce, averitt, southeastern-freight.",
+        "covered_carriers": ["odfl", "saia", "estes", "xpo", "abf", "tforce", "averitt", "southeastern-freight"],
+        "hint": "Call GET /v1/fuel-surcharge to list every covered carrier with its latest surcharge.",
+    },
+}
+
+# live: POST /v1/subscriptions {"codes": [], "interval_seconds": 86400}
+CODES_BLANK_422 = {
+    "status": "fail",
+    "data": {
+        "error": "VALIDATION_ERROR",
+        "message": "Codes can't be blank",
+        "details": {"codes": ["can't be blank"]},
+    },
+}
+
+# live: POST /v1/subscriptions {"interval_seconds": -5}; `upgrade.plans` trimmed to one entry.
+INTERVAL_FLOOR_402 = {
+    "status": "fail",
+    "data": {
+        "error": "INTERVAL_FLOOR",
+        "message": "Your plan's minimum snapshot interval is 60s (requested -5s). Upgrade to poll faster.",
+        "limit": 60,
+        "current": -5,
+        "upgrade_trigger": "interval_floor",
+        "upgrade_url": "https://www.oilpriceapi.com/pricing?plan=enterprise_v2&utm_source=api&utm_medium=agent&utm_campaign=agent_min_interval",
+        "upgrade": {
+            "url": "https://www.oilpriceapi.com/pricing?plan=enterprise_v2&utm_source=api&utm_medium=agent&utm_campaign=agent_min_interval",
+            "next_tier": "enterprise_v2",
+            "plans": [{"plan": "developer", "requests": 10000, "watches": 3}],
+        },
+    },
+}
+
+# live: GET /v1/subscriptions/<unknown uuid>
+NOT_FOUND_404 = {
+    "error": {
+        "code": "NOT_FOUND",
+        "message": "Subscription not found",
+        "status": 404,
+        "request_id": "6598da0c-4289-470f-84f3-b4c3d56198b3",
+        "docs": "https://docs.oilpriceapi.com#NOT_FOUND",
+    }
+}
+
+# live: GET /v1/prices/latest?by_code=NOT_A_REAL_CODE_XYZ (lower-snake code + sentence,
+# app/controllers/concerns/api_validations.rb).
+INVALID_CODE_400 = {
+    "status": "fail",
+    "data": {
+        "error": "invalid_code",
+        "message": "Code 'NOT_A_REAL_CODE_XYZ' not found. No close match found. See /v1/commodities for all available codes.",
+        "suggestions": [],
+        "did_you_mean": [],
+        "invalid_codes": ["NOT_A_REAL_CODE_XYZ"],
+        "all_codes_url": "https://api.oilpriceapi.com/v1/commodities",
+    },
+}
+
+
+def _response(status, body):
+    request = httpx.Request(
+        "GET",
+        "https://api.oilpriceapi.com/v1/fuel-surcharge/nope/latest",
+        headers={"Authorization": f"Token {FIXTURE_KEY}"},
+    )
+    return httpx.Response(status, json=body, request=request)
+
+
+def _raised(mode, status, body):
+    """Return the error the real client raises for ``body``."""
+    response = _response(status, body)
+    if mode == "sync":
+        with OilPriceAPI(api_key=FIXTURE_KEY, max_retries=1) as client:
+            with patch.object(client._client, "request", return_value=response):
+                with pytest.raises(OilPriceAPIError) as info:
+                    client.request("GET", "/v1/fuel-surcharge/nope/latest")
+        return info.value
+
+    async def go():
+        async with AsyncOilPriceAPI(api_key=FIXTURE_KEY, max_retries=1) as client:
+            with patch.object(client._client, "request", new=AsyncMock(return_value=response)):
+                with pytest.raises(OilPriceAPIError) as info:
+                    await client.request("GET", "/v1/fuel-surcharge/nope/latest")
+        return info.value
+
+    return asyncio.run(go())
+
+
+# --- the defect ----------------------------------------------------------------
+
+
+@pytest.mark.parametrize("mode", MODES)
+def test_sentence_in_fail_envelope_is_the_message_not_the_code(mode):
+    error = _raised(mode, 404, UNKNOWN_CARRIER_404)
+
+    assert isinstance(error, DataNotFoundError)
+    assert error.code is None
+    assert error.machine_code is None
+    assert error.message == UNKNOWN_CARRIER_404["data"]["error"]
+    assert error.suggestions == UNKNOWN_CARRIER_404["data"]["covered_carriers"]
+    assert error.raw_body == UNKNOWN_CARRIER_404
+
+
+@pytest.mark.parametrize(
+    ("status", "expected_type"),
+    [
+        (400, BadRequestError),
+        (401, AuthenticationError),
+        (402, PaymentRequiredError),
+        (403, PermissionDeniedError),
+        (404, DataNotFoundError),
+        (422, ValidationError),
+        (429, RateLimitError),
+        (503, ServerError),
+    ],
+)
+def test_no_status_class_turns_a_sentence_into_a_code(status, expected_type):
+    body = {"status": "fail", "data": {"error": "period is not supported by the latest endpoint; use /v1/rig-counts/historical"}}
+
+    error = error_from_response(_response(status, body))
+
+    assert isinstance(error, expected_type)
+    assert error.status_code == status
+    assert error.code is None
+    assert error.message == body["data"]["error"]
+
+
+def test_sentence_without_the_status_key_is_not_a_code_either():
+    body = {"data": {"error": "Parcel fuel-surcharge history requires a service_level parameter."}}
+
+    error = error_from_response(_response(400, body))
+
+    assert error.code is None
+    assert error.message == body["data"]["error"]
+
+
+# --- what must not change ------------------------------------------------------
+
+
+@pytest.mark.parametrize("mode", MODES)
+def test_upper_snake_code_is_the_code_and_message_is_the_sentence(mode):
+    error = _raised(mode, 422, CODES_BLANK_422)
+
+    assert isinstance(error, ValidationError)
+    assert error.status_code == 422
+    assert error.code == "VALIDATION_ERROR"
+    assert error.machine_code == "VALIDATION_ERROR"
+    assert error.message == "Codes can't be blank"
+
+
+@pytest.mark.parametrize("mode", MODES)
+def test_upgrade_trigger_keeps_code_message_and_remediation(mode):
+    error = _raised(mode, 402, INTERVAL_FLOOR_402)
+
+    assert isinstance(error, PaymentRequiredError)
+    assert error.code == "INTERVAL_FLOOR"
+    assert error.message.startswith("Your plan's minimum snapshot interval is 60s")
+    assert error.remediation_url == INTERVAL_FLOOR_402["data"]["upgrade_url"]
+
+
+@pytest.mark.parametrize("mode", MODES)
+def test_lower_snake_code_the_api_sends_stays_the_code(mode):
+    error = _raised(mode, 400, INVALID_CODE_400)
+
+    assert isinstance(error, BadRequestError)
+    assert error.code == "invalid_code"
+    assert error.message == INVALID_CODE_400["data"]["message"]
+    assert error.invalid_codes == ["NOT_A_REAL_CODE_XYZ"]
+
+
+def test_code_with_no_sentence_is_both_code_and_message():
+    error = error_from_response(_response(402, {"status": "fail", "data": {"error": "WATCH_LIMIT"}}))
+
+    assert error.code == "WATCH_LIMIT"
+    assert error.message == "WATCH_LIMIT"
+
+
+@pytest.mark.parametrize("mode", MODES)
+def test_canonical_nested_error_object_keeps_precedence(mode):
+    error = _raised(mode, 404, NOT_FOUND_404)
+
+    assert isinstance(error, DataNotFoundError)
+    assert error.code == "NOT_FOUND"
+    assert error.message == "Subscription not found"
+    assert error.request_id == "6598da0c-4289-470f-84f3-b4c3d56198b3"
+
+
+def test_error_object_nested_under_fail_data_keeps_precedence():
+    body = {"status": "fail", "data": {"error": {"code": "DATA_NOT_AVAILABLE", "message": "Not yet published"}}}
+
+    error = error_from_response(_response(404, body))
+
+    assert error.code == "DATA_NOT_AVAILABLE"
+    assert error.message == "Not yet published"
+
+
+# --- redaction is unchanged ----------------------------------------------------
+
+
+@pytest.mark.parametrize("mode", MODES)
+def test_key_in_a_fail_sentence_is_redacted_and_not_used_as_code(mode):
+    body = {"status": "fail", "data": {"error": f"API key {FIXTURE_KEY} cannot use this route"}}
+
+    error = _raised(mode, 403, body)
+
+    assert error.code is None
+    assert error.message == "API key [REDACTED] cannot use this route"
+    for rendered in (error.message, str(error), repr(error.raw_body), error.raw_text):
+        assert FIXTURE_KEY not in rendered
+
+
+@pytest.mark.parametrize("mode", MODES)
+def test_key_in_a_fail_message_is_redacted_beside_a_machine_code(mode):
+    body = {"status": "fail", "data": {"error": "VALIDATION_ERROR", "message": f"Token {FIXTURE_KEY} rejected"}}
+
+    error = _raised(mode, 422, body)
+
+    assert error.code == "VALIDATION_ERROR"
+    # main redacts the whole "Token <key>" header value first; unchanged here.
+    assert error.message == "[REDACTED] rejected"
+    for rendered in (error.message, str(error), repr(error.raw_body), error.raw_text):
+        assert FIXTURE_KEY not in rendered