diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ad54f0..2e2e8dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,25 @@ +## 0.1.1 + +* **A provider rate limit no longer reaches the wire as terminal.** `plugin_error_of` declared + `retryable = false` for the whole legacy `UserError` family, `RateLimitError` included, so a 429 + a consumer reads off the declared field fails the record on the first throttle instead of backing + off, and charges it to the customer. `RateLimitError` now serializes + `error_type = "dependency"`, `audience = "user"`, `retryable = true` and an `error_reason` that + defaults to `rate_limited`, mirroring the `utic_plugin_base` class of the same name. An explicit + `failure_category` still overrides that default reason, as it does for the rest of the family. + Every other member of + the family stays terminal: terminal is the safe default, so a transient condition declares itself. + The `plugins_controller` reads retryability off the HTTP status band today, so its behaviour is + unchanged either way. +* **`plugin_error.error_reason` is lower_snake_case again.** It was assigned straight from + `failure_category`, which is a separate SCREAMING_SNAKE vocabulary (`AUTH_PERMISSION_DENIED`), so + a plugin that set one put the wrong spelling on a field specified lower_snake_case. The category + is now normalized for this field only and still rides the top-level `failure_category` verbatim. + Compatibility: the verbatim spelling reached the wire only in 0.1.0, so a consumer written + against that release reads `error_reason` change spelling across this patch bump. Nothing is + lost, since the top-level `failure_category` is unchanged, but read that field rather than + parsing `error_reason` if you need the category verbatim. + ## 0.1.0 * **This package now owns the `/invoke` transport for the reserved fields.** diff --git a/test/api/test_api.py b/test/api/test_api.py index 0961c05..3ec4a8a 100644 --- a/test/api/test_api.py +++ b/test/api/test_api.py @@ -12,6 +12,9 @@ FileData, SourceIdentifiers, ) +from unstructured_ingest.error import RateLimitError as IngestRateLimitError +from unstructured_ingest.error import UserAuthError as IngestUserAuthError +from unstructured_ingest.error import UserError as IngestUserError from unstructured_platform_plugins.etl_uvicorn.api_generator import ( EtlApiException, @@ -916,3 +919,132 @@ def _string_annotated_precheck(usage: "list") -> "None": ) assert client.get("/precheck").json()["status_code"] == 200 + + +class _CategorizedUserError(IngestUserError): + """A user error that also carries a preflight failure_category, as partitioner's do.""" + + failure_category = "AUTH_PERMISSION_DENIED" + + +def test_rate_limit_error_reaches_the_wire_as_retryable(): + """A provider 429 is transient: terminalizing it fails a record that backing off recovers. + + Mirrors the utic_plugin_base RateLimitError declaration (platform-libs #889), which + carries error_type="dependency", error_reason="rate_limited" and retryable=True. + """ + + def _rate_limited() -> None: + raise IngestRateLimitError("provider throttled the request") + + client = TestClient(wrap_in_fastapi(func=_rate_limited, plugin_id="mock_plugin")) + + body = client.post("/invoke").json() + + assert body["status_code"] == 429 + plugin_error = body["plugin_error"] + assert plugin_error["audience"] == "user" + assert plugin_error["retryable"] is True + assert plugin_error["error_type"] == "dependency" + assert plugin_error["error_reason"] == "rate_limited" + + +def test_non_rate_limited_user_errors_stay_terminal(): + """Terminal is the safe default: only the transient condition declares itself.""" + + def _bad_credentials() -> None: + raise IngestUserAuthError("credential rejected") + + client = TestClient(wrap_in_fastapi(func=_bad_credentials, plugin_id="mock_plugin")) + + plugin_error = client.post("/invoke").json()["plugin_error"] + + assert plugin_error["retryable"] is False + assert plugin_error["error_type"] == "configuration" + assert plugin_error["error_reason"] == "invalid_input" + + +def test_rate_limit_failure_category_overrides_the_default_reason(): + """`rate_limited` is the class DEFAULT reason, not a fixed one. + + A plugin that declares a `failure_category` on a rate limit has said something more + specific than the class did, so the category wins the reason -- the same precedence every + other member of the family gets. The retryability and typing still come from the class. + """ + + class _CategorizedRateLimitError(IngestRateLimitError): + failure_category = "PROVIDER_RATE_LIMITED" + + def _rate_limited() -> None: + raise _CategorizedRateLimitError("provider throttled the request") + + client = TestClient(wrap_in_fastapi(func=_rate_limited, plugin_id="mock_plugin")) + + plugin_error = client.post("/invoke").json()["plugin_error"] + + assert plugin_error["error_reason"] == "provider_rate_limited" + assert plugin_error["error_type"] == "dependency" + assert plugin_error["retryable"] is True + + +def test_precheck_rate_limit_error_is_retryable(): + """The precheck envelope carries the same retryability as invoke.""" + + def _rate_limited_precheck() -> None: + raise IngestRateLimitError("provider throttled the precheck") + + client = TestClient( + wrap_in_fastapi( + func=_no_params, plugin_id="mock_plugin", precheck_func=_rate_limited_precheck + ) + ) + + plugin_error = client.get("/precheck").json()["plugin_error"] + + assert plugin_error["retryable"] is True + assert plugin_error["error_reason"] == "rate_limited" + + +def test_error_reason_is_lower_snake_case_on_the_wire(): + """`error.reason` is lower_snake_case. `failure_category` is a separate + SCREAMING_SNAKE vocabulary and keeps its own top-level field.""" + + def _categorized() -> None: + raise _CategorizedUserError("credential rejected") + + client = TestClient(wrap_in_fastapi(func=_categorized, plugin_id="mock_plugin")) + + body = client.post("/invoke").json() + + assert body["failure_category"] == "AUTH_PERMISSION_DENIED" + assert body["plugin_error"]["error_reason"] == "auth_permission_denied" + + +def test_error_reason_normalizes_a_non_snake_failure_category(): + """failure_category is arbitrary plugin-supplied text, so lowercasing alone does not + produce a lower_snake_case token.""" + + class _SpacedCategoryError(IngestUserError): + failure_category = "Auth Permission-Denied " + + def _spaced() -> None: + raise _SpacedCategoryError("credential rejected") + + client = TestClient(wrap_in_fastapi(func=_spaced, plugin_id="mock_plugin")) + + body = client.post("/invoke").json() + + assert body["failure_category"] == "Auth Permission-Denied " + assert body["plugin_error"]["error_reason"] == "auth_permission_denied" + + +def test_unusable_failure_category_falls_back_to_the_default_reason(): + class _PunctuationCategoryError(IngestUserError): + failure_category = "///" + + def _punctuation() -> None: + raise _PunctuationCategoryError("credential rejected") + + client = TestClient(wrap_in_fastapi(func=_punctuation, plugin_id="mock_plugin")) + + assert client.post("/invoke").json()["plugin_error"]["error_reason"] == "invalid_input" diff --git a/unstructured_platform_plugins/__version__.py b/unstructured_platform_plugins/__version__.py index 2a08aec..95d99bb 100644 --- a/unstructured_platform_plugins/__version__.py +++ b/unstructured_platform_plugins/__version__.py @@ -1 +1 @@ -__version__ = "0.1.0" # pragma: no cover +__version__ = "0.1.1" # pragma: no cover diff --git a/unstructured_platform_plugins/etl_uvicorn/api_generator.py b/unstructured_platform_plugins/etl_uvicorn/api_generator.py index 767ae3f..bb16525 100644 --- a/unstructured_platform_plugins/etl_uvicorn/api_generator.py +++ b/unstructured_platform_plugins/etl_uvicorn/api_generator.py @@ -3,6 +3,7 @@ import inspect import json import logging +import re from typing import Any, Callable, Optional, Union, get_origin from fastapi import FastAPI, HTTPException, status @@ -12,7 +13,7 @@ from starlette.responses import RedirectResponse from typing_extensions import deprecated from unstructured_ingest.data_types.file_data import BatchFileData, FileData, file_data_from_dict -from unstructured_ingest.error import UnstructuredIngestError, UserError +from unstructured_ingest.error import RateLimitError, UnstructuredIngestError, UserError from uvicorn.config import LOG_LEVELS from uvicorn.importer import import_from_string @@ -111,19 +112,52 @@ def failure_category_of(error: BaseException) -> Optional[str]: return category if isinstance(category, str) else None +def _as_error_reason(category: Optional[str]) -> Optional[str]: + """Render a failure_category as an ``error.reason`` token, or None when it cannot be. + + ``failure_category`` and ``error.reason`` are two vocabularies, not one: the preflight + categories are SCREAMING_SNAKE (``AUTH_PERMISSION_DENIED``) while ``error.reason`` is + specified lower_snake_case. ``failure_category`` still rides its own top-level response + field verbatim, so nothing is lost by normalizing the copy that lands here. + + The category is plugin-supplied free text rather than a closed enum, so lowercasing + alone would not produce a snake_case token; punctuation and whitespace collapse to + single underscores and a category with no usable characters yields None. + """ + if category is None: + return None + reason = re.sub(r"[^a-z0-9]+", "_", category.lower()).strip("_") + return reason or None + + def plugin_error_of(error: BaseException) -> Optional[PluginErrorMetadata]: """Map the legacy UserError family onto the canonical plugin-error envelope. The ratified ErrorAudience vocabulary owns the wire spelling. A non-user failure remains unclassified: orchestrators must not infer actionability from its HTTP status. + + Terminal is the safe default, so a transient condition has to declare itself: a provider + rate limit is the one member of this family that clears on its own, and it mirrors the + utic_plugin_base ``RateLimitError`` declaration (platform-libs #889) field for field. + Retryability is orthogonal to audience -- a throttled request is still the caller's + quota, so the audience stays ``user``. + + The class picks the DEFAULT reason; an explicit ``failure_category`` still wins, on this + branch as on every other. A plugin that declares a category has said something more + specific than the class did (a rate limit raised as ``PROVIDER_RATE_LIMITED``), and + dropping it here would discard the more precise of the two. """ if not isinstance(error, UserError): return None + if isinstance(error, RateLimitError): + error_type, default_reason, retryable = "dependency", "rate_limited", True + else: + error_type, default_reason, retryable = "configuration", "invalid_input", False return PluginErrorMetadata( - error_type="configuration", - error_reason=failure_category_of(error) or "invalid_input", + error_type=error_type, + error_reason=_as_error_reason(failure_category_of(error)) or default_reason, audience=ErrorAudience.USER, - retryable=False, + retryable=retryable, )