From 133841bedb6f94edfd00f91cf6c91850b387fd39 Mon Sep 17 00:00:00 2001 From: Emmanuel Sandorfi Date: Thu, 24 Sep 2026 16:21:04 +0200 Subject: [PATCH 1/2] feat(facet): batch classify and attributes set to one file --- AGENTS.md | 60 +++++++++++- README.md | 71 ++++++++++++++ lighton/__init__.py | 15 ++- lighton/content_type.py | 162 +++++++++++++++++++++++++++++++- lighton/enums.py | 15 +++ lighton/exceptions.py | 24 ++++- lighton/file.py | 84 ++++++++++++++--- tests/e2e/cli.py | 58 +++++++++++- tests/test_client.py | 20 ++++ tests/test_file.py | 202 +++++++++++++++++++++++++++++++++++++++- 10 files changed, 688 insertions(+), 23 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f60e4b9..c3a1777 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,7 +22,7 @@ lighton/ workspace.py # Workspace, active-record, lives at root apikey.py # ApiKey / ApiKeyScope, active-record, lives at root tag.py # Tag, active-record (list/create/delete only; no single GET) - content_type.py # ContentType/Facet/Attribute, content-type taxonomy + file facets + content_type.py # ContentType/Facet/Attribute/FacetAction/FacetResult, taxonomy + file facets file.py # File, active-record + wait_all(); upload = ingestion batch.py # ingest_many() batch upload behavior: BatchIngestJob (threads/poll) job.py # ParseJob/ExtractJob, client-bound async handles you poll() @@ -152,6 +152,16 @@ parsed), `ServerError` (5xx), and `MaintenanceError`. payload stays on `.body`. A 503 **without** the marker stays a plain `ServerError` (pinned by a test). Still not retried: 5xx never is, and a maintenance window outlasts any cooldown worth sleeping through. +- **`LightOnAPIError.index`** is the 0-based position of the failing action inside a + batch request, `None` otherwise. Parsed in the **base `__init__`** by `_index()` + (sibling of `_retry_after`/`_timestamp`), so every subclass inherits it through its + `super().__init__` and `from_response` stays the single construction point; the + position is also appended to the message (`... (action 3)`) so a bare traceback + names the offender. Not a `BatchActionError` subclass, because the API sends + `index` on **400/403/404/422 alike**: a body-keyed class would either break + `except NotFoundError` for batch callers or fork four ways for one integer, and + `MaintenanceError` stays the *one* body-keyed mapping. `_index()` tests + `isinstance(value, int)` rather than truthiness, since index 0 is a real answer. ## Resource management: active-record @@ -349,8 +359,52 @@ use `_ActiveRecord.list`. `File` classification (all via `POST /files//facets` with an `action`): `classify`/ `unclassify` (assign/remove a content type, T2), `set_attribute`/`clear_attribute` (an attribute value under an assigned type, T3), each accepting a `ContentType` or a path -string (one `_facet(action, ct, **extra)` helper builds the body). `facets()` GETs the -file's assigned types as `list[Facet]`. Like tags, File models no facet fields locally. +string. `facets()` GETs the file's assigned types as `list[Facet]`. Like tags, File +models no facet fields locally. + +- **`batch_facets(actions)`** is the batch sibling (POST `/files//facets/batch`, + max **50**), and unlike `ContentType.batch` it is **typed both ways**. That + divergence is the point, not an oversight: a taxonomy action spans five different + shapes (`adopt` takes a path list, `define_content_type` code/label/parent, + `define_attribute` seven fields), so modelling it means five models or one wide + model whose valid fields depend on the action; a file-facet action has exactly + **one** shape (four fields, four verbs), which `FacetAction` models cleanly. Raw + dicts are still accepted alongside `FacetAction`, so the escape hatch is what the + two surfaces share. `ContentType.batch` was deliberately left untouched: it is + shipped public API and retyping its return would break `r["status"]` for everyone. +- **Naming split.** The `FacetAction` constructors carry the **SDK's** method names + (`classify`/`unclassify`/`set_attribute`/`clear_attribute`) so a batch is a + mechanical transcription of the one-by-one calls it replaces; `FacetActionType` + (enums.py, its full domain is documented, mirrors the generated + `FileFacetActionRequestActionEnum`) and the wire carry the **API's** + (`set_value`/`clear_value`). The enum exists partly to make that mapping + discoverable. Method name is `batch_facets`, not `facets_batch`: every write on + `File` is verb-first, and bare `batch` is unusable because `Workspace`/`batch.py` + already own a batch *ingest* concept. +- **One wire encoder.** `FacetAction._body()` is the single place that knows the + body field names, and `_facet` now takes a `FacetAction`, so the four single-action + methods and the batch cannot drift; a test pins single-action bodies equal to batch + bodies. `FacetAction`/`FacetResult`/`MAX_FACET_ACTIONS` live in `content_type.py` + next to `Facet`/`Attribute` (the `Template` precedent for taxonomy-adjacent data + models); `types/` would invert the dependency by pulling `ContentType` down into it. +- **The 50 cap raises a `ValueError` client-side**, the same trade as + `define_attribute`'s missing-`choices` check, and empty is a local no-op (the + `tag`/`untag`/`delete_many` convention). It is deliberately **not chunked**: the + endpoint fails fast at the offending action and commits everything before it, so + chunking would both scatter that boundary (the reported `index` would be relative + to a batch the caller never wrote) and forfeit the single BM25 reindex that is the + whole reason the endpoint exists. +- **`FacetResult` is curated, not the generated `BatchResultItem`**: the generated + name is anonymous and can't document which status belongs to which verb. Its `data` + stays a raw dict for the reason `ContentType.batch` already gives (a classification + for `classify`, an attribute for `set_value`, null for the 204 verbs, so no one + model fits), but the *envelope* is uniform, which is what `FacetResult` buys. No + back-reference to the action: results only come back on success, complete and in + order, so `results[i]` is `actions[i]` by construction. +- **Partial commit, unlike `delete_many`.** `/files/bulk-delete` is all-or-nothing + server-side, so it raises and there is no per-item report to give. This endpoint + commits the prefix before the failure and does **not** return those results, so the + *position* is the payload, and it rides on the exception (`LightOnAPIError.index`). If adding new resources, subclass `_ActiveRecord`: set `_base`/`_resource`, declare the field schema (narrow `id`), and add `create()`/`save()`. Everything else is inherited. diff --git a/README.md b/README.md index f318158..8d34b53 100644 --- a/README.md +++ b/README.md @@ -849,6 +849,70 @@ doc.clear_attribute("legal:contract:nda", "jurisdiction") doc.unclassify("legal:contract:nda") ``` +Doing several of these at once? See +[Many writes in one request](#many-writes-in-one-request). + +### Many writes in one request + +Classifying a document is rarely one call: it's the `classify`, then one write per +attribute. `batch_facets()` sends up to 50 of them in a **single request**, which +also reindexes the document once instead of once per action, and that is the part +that actually costs time. Build the list with `FacetAction`, whose constructors +take the same arguments as the methods above, so a batch is a transcription of the +calls it replaces: + +```python +from lighton import FacetAction, File + +doc = File.get_by_name(client, "nda-2026.pdf", workspace=42)[0] + +results = doc.batch_facets([ + FacetAction.classify("legal:contract:nda"), + FacetAction.set_attribute("legal:contract:nda", "jurisdiction", "FR"), + FacetAction.set_attribute("legal:contract:nda", "signed_on", "2026-07-01"), + FacetAction.set_attribute("legal:contract:nda", "signed", True), +]) +print([r.status for r in results]) # [201, 201, 201, 201] +``` + +The list is inert until it reaches a file, so the same one applies to a whole +corpus: + +```python +actions = [ + FacetAction.classify("legal:contract:nda"), + FacetAction.set_attribute("legal:contract:nda", "jurisdiction", "FR"), +] +for doc in File.list(client, workspace_id=42, extension="pdf"): + doc.batch_facets(actions) +``` + +You get one `FacetResult` per action, in the order sent, so `results[i]` belongs to +`actions[i]`. Each carries a `status` (201 created, 200 already applied, 204 for +`unclassify`/`clear_attribute`) and `data`, which is `None` for those 204 actions. + +**When one action fails.** The batch is not transactional. A malformed action is +caught up front and nothing runs, but a *domain* error (an unknown content type, +setting a value before classifying, a sibling conflict) stops at that action and +leaves everything before it applied. The error carries the 0-based position, so +the offender is addressable, and every action is idempotent, so the fix is to +correct it and resend the whole list: + +```python +from lighton.exceptions import LightOnAPIError + +try: + doc.batch_facets(actions) +except LightOnAPIError as e: + print("failed at action", e.index, e.body["detail"]) + print("already applied:", actions[:e.index]) +``` + +`index` is `None` on any non-batch error. Past 50 actions `batch_facets()` raises a +`ValueError` before the request goes out: split the list yourself rather than have +the SDK guess where to cut, since chunking would restore the per-chunk reindex the +endpoint exists to avoid. + ### Building the taxonomy Starting from nothing? Adopt a starter tree from the catalog: @@ -898,6 +962,13 @@ results = ContentType.batch(client, [ print([r["status"] for r in results]) # [201, 201] ``` +This is the taxonomy-side sibling of +[`batch_facets()`](#many-writes-in-one-request), with the same fail-fast behavior +and the same 50-action cap. The entries stay raw dicts here because the taxonomy +spans five different action shapes (`adopt` takes a path list, `define_content_type` +takes code/label/parent, `define_attribute` takes seven fields), whereas a file +facet action has exactly one shape, which is what `FacetAction` models. + ## API keys Same active-record style. The plaintext secret is available **only** right after `create()`. diff --git a/lighton/__init__.py b/lighton/__init__.py index c515594..0a85e41 100644 --- a/lighton/__init__.py +++ b/lighton/__init__.py @@ -3,11 +3,20 @@ from lighton._client import LightOn from lighton.apikey import ApiKey, ApiKeyScope from lighton.batch import BatchIngest, BatchIngestJob, BatchProgress, FailedIngest -from lighton.content_type import Attribute, ContentType, Facet, Template +from lighton.content_type import ( + MAX_FACET_ACTIONS, + Attribute, + ContentType, + Facet, + FacetAction, + FacetResult, + Template, +) from lighton.enums import ( AttributeType, DownloadPurpose, ExecMode, + FacetActionType, FileStatus, JobStatus, RelevanceScoring, @@ -54,12 +63,16 @@ "ExternalMetadata", "ExtractJob", "Facet", + "FacetAction", + "FacetActionType", + "FacetResult", "FailedIngest", "File", "FileStatus", "JobStatus", "LightOn", "LightOnConfiguration", + "MAX_FACET_ACTIONS", "ParseJob", "RelevanceScoring", "ReprocessLevel", diff --git a/lighton/content_type.py b/lighton/content_type.py index 3f51501..7d0dd5a 100644 --- a/lighton/content_type.py +++ b/lighton/content_type.py @@ -7,7 +7,8 @@ `Facet` is a content type *assigned to a file* together with the file's attribute values on it (see `File.classify()` / `File.facets()`). `Attribute` is the shared -name/type/value shape used by both. +name/type/value shape used by both. `FacetAction`/`FacetResult` are the write and +result shapes of `File.batch_facets()`. """ from __future__ import annotations @@ -16,9 +17,9 @@ from builtins import list as _list from typing import TYPE_CHECKING, Any -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator -from lighton.enums import AttributeType +from lighton.enums import AttributeType, FacetActionType from lighton.utils import _compact, _path if TYPE_CHECKING: @@ -26,6 +27,9 @@ _BASE = "/api/v3/content-types" +MAX_FACET_ACTIONS = 50 +"""Actions per `File.batch_facets()` call, the API's cap. Split a longer job yourself.""" + class Attribute(BaseModel): """One attribute of a content type, a definition, or a value set on a file. @@ -320,5 +324,157 @@ class Facet(BaseModel): ) +class FacetAction(BaseModel): + """One classification write, applied by `File.batch_facets()`. + + Build these with the constructors, not the fields: each takes the same + arguments in the same order as the `File` method of the same name, so a batch + is a transcription of the single-action calls it replaces. + + doc.batch_facets([ + FacetAction.classify(nda), + FacetAction.set_attribute(nda, "jurisdiction", "FR"), + ]) + + Nothing is sent until the list reaches a file, so one list applies to many. + """ + + model_config = ConfigDict(extra="ignore") + + action: FacetActionType = Field( + description="The write to perform, the API's verb (see FacetActionType)." + ) + content_type_path: str = Field( + description="Content type the action applies to, e.g. legal:contract:nda." + ) + attribute_name: str | None = Field( + None, + description="Attribute identifier in snake_case; required by the value verbs.", + ) + value: Any = Field( + None, + description=( + "Value for set_attribute; shape follows the attribute type (string, " + "number, date 'YYYY-MM-DD', bool, or list[str] for multi-select)." + ), + ) + + @model_validator(mode="after") + def _value_actions_need_an_attribute(self) -> FacetAction: + """The API 422s a value verb with no attribute_name; refuse it locally.""" + value_verbs = (FacetActionType.set_value, FacetActionType.clear_value) + if self.action in value_verbs and not self.attribute_name: + raise ValueError(f"{self.action} needs an attribute_name") + return self + + @classmethod + def classify(cls, content_type: ContentType | str) -> FacetAction: + """Assign a content type, the batch form of `File.classify()`. + + Args: + content_type: The content type to assign (object or path string). + + Returns: + The action, unsent. + """ + return cls( + action=FacetActionType.classify, content_type_path=_path(content_type) + ) + + @classmethod + def unclassify(cls, content_type: ContentType | str) -> FacetAction: + """Remove a content-type assignment, the batch form of `File.unclassify()`. + + Args: + content_type: The content type to unassign (object or path string). + + Returns: + The action, unsent. + """ + return cls( + action=FacetActionType.unclassify, content_type_path=_path(content_type) + ) + + @classmethod + def set_attribute( + cls, content_type: ContentType | str, name: str, value: Any + ) -> FacetAction: + """Set an attribute value, the batch form of `File.set_attribute()`. + + Args: + content_type: The assigned content type (object or path string). + name: Attribute identifier (snake_case). + value: The value; shape depends on the attribute type (string, number, + date "YYYY-MM-DD", bool, or list[str] for multi-select). + + Returns: + The action, unsent. + """ + return cls( + action=FacetActionType.set_value, + content_type_path=_path(content_type), + attribute_name=name, + value=value, + ) + + @classmethod + def clear_attribute(cls, content_type: ContentType | str, name: str) -> FacetAction: + """Clear an attribute value, the batch form of `File.clear_attribute()`. + + Args: + content_type: The assigned content type (object or path string). + name: Attribute identifier to clear. + + Returns: + The action, unsent. + """ + return cls( + action=FacetActionType.clear_value, + content_type_path=_path(content_type), + attribute_name=name, + ) + + def _body(self) -> dict[str, Any]: + # The one place that knows the wire field names: the single-action methods + # on File post exactly this too, so single and batch can't drift. + body: dict[str, Any] = { + "action": self.action, + "content_type_path": self.content_type_path, + } + if self.attribute_name is not None: + body["attribute_name"] = self.attribute_name + if self.action == FacetActionType.set_value: + body["value"] = self.value # sent even when None, the server decides + return body + + +class FacetResult(BaseModel): + """What one action in a `File.batch_facets()` returned, in request order. + + Every result you receive succeeded: the endpoint fails fast, so a failing + action raises (see `LightOnAPIError.index`) and no results come back at all. A + returned list is therefore always complete and in order, so `results[i]` is the + outcome of `actions[i]`. + """ + + model_config = ConfigDict(extra="ignore") + + status: int = Field( + description=( + "Per-action status: 201 created, 200 already applied/updated, 204 for " + "the removals (unclassify, clear_attribute)." + ) + ) + data: dict[str, Any] | None = Field( + None, + description=( + "What the action returned, None for the 204 verbs. classify gives " + "{content_type_path, label}; set_attribute gives {name, value, " + "content_type_path, label}. Left a raw dict: it differs per verb, so " + "there is no one model to validate it into." + ), + ) + + ContentType.model_rebuild() # resolve the self-referential `children` forward ref Template.model_rebuild() diff --git a/lighton/enums.py b/lighton/enums.py index 1a5082a..5745f96 100644 --- a/lighton/enums.py +++ b/lighton/enums.py @@ -48,6 +48,21 @@ class AttributeType(StrEnum): rich_text = "rich-text" +class FacetActionType(StrEnum): + """The wire verb in a file-facet action (`File.batch_facets`). + + `set_value`/`clear_value` are the API's names for what `File.set_attribute()` + and `File.clear_attribute()` do. The `FacetAction` constructors keep the SDK's + names so a batch reads like the single-action calls it replaces; this enum + keeps the API's, which is what goes over the wire. + """ + + classify = "classify" + unclassify = "unclassify" + set_value = "set_value" + clear_value = "clear_value" + + class ReprocessLevel(StrEnum): """Reprocessing level queued on a File (`pending_reprocess`), `update` = replacement.""" diff --git a/lighton/exceptions.py b/lighton/exceptions.py index addb2b3..9c137fa 100644 --- a/lighton/exceptions.py +++ b/lighton/exceptions.py @@ -35,12 +35,22 @@ def __init__(self, message: str, *, body: Any = None) -> None: class LightOnAPIError(LightOnError): - """The API returned a non-2xx response.""" + """The API returned a non-2xx response. + + `index` is set only on a **batch** endpoint failure: the 0-based position of + the action that failed. Batch endpoints validate every action's fields up + front (a field-level 422 executes nothing), then run in order and stop at the + first domain error, so the actions before `index` are already applied and the + one at `index` is not. Every action is idempotent, so the fix is to correct + that one and resend the whole list. None for a single-action request, which + never carries an index. + """ def __init__(self, message: str, *, status_code: int, body: Any = None) -> None: super().__init__(message) self.status_code = status_code self.body = body + self.index: int | None = _index(body) class AuthenticationError(LightOnAPIError): @@ -133,6 +143,9 @@ def from_response(response: httpx.Response) -> LightOnAPIError: message = f"{response.status_code} {response.reason_phrase}" if detail: message = f"{message}: {detail}" + index = _index(body) + if index is not None: + message = f"{message} (action {index})" if cls is RateLimitError: return RateLimitError( message, @@ -163,6 +176,15 @@ def _retry_after(response: httpx.Response) -> float | None: return None +def _index(body: Any) -> int | None: + """0-based position of the failing action inside a batch, or None. + + `isinstance(value, int)` rather than truthiness: index 0 is a real answer. + """ + value = body.get("index") if isinstance(body, dict) else None + return value if isinstance(value, int) else None + + def _timestamp(raw: Any) -> datetime | None: """Parse an ISO-8601 timestamp, or None. The raw value stays on `.body`.""" if not isinstance(raw, str): diff --git a/lighton/file.py b/lighton/file.py index 1d9ed02..59eb26c 100644 --- a/lighton/file.py +++ b/lighton/file.py @@ -19,18 +19,23 @@ from concurrent.futures import ThreadPoolExecutor from datetime import datetime from pathlib import Path -from typing import TYPE_CHECKING, ClassVar +from typing import TYPE_CHECKING, Any, ClassVar from pydantic import Field from lighton._active_record import _ActiveRecord -from lighton.content_type import Facet +from lighton.content_type import ( + MAX_FACET_ACTIONS, + Facet, + FacetAction, + FacetResult, +) from lighton.enums import DownloadPurpose, FileStatus, ReprocessLevel from lighton.exceptions import LightOnError from lighton.tag import resolve_ids from lighton.types.api import Page from lighton.types.file import ExternalMetadata, Thumbnail -from lighton.utils import _compact, _ids, _path +from lighton.utils import _compact, _ids if TYPE_CHECKING: from lighton._client import LightOn @@ -453,12 +458,10 @@ def untag(self, tags: _list[Tag | int | str]) -> File: return self # --- content-type classification (facets) ------------------------------ - def _facet(self, action: str, content_type: ContentType | str, **extra: object): - return self._api( - "POST", - f"{_BASE}/{self.id}/facets", - json={"action": action, "content_type_path": _path(content_type), **extra}, - ) + def _facet(self, action: FacetAction): + # FacetAction._body() is the single wire encoder, shared with batch_facets, + # so the single-action and batch bodies cannot drift apart. + return self._api("POST", f"{_BASE}/{self.id}/facets", json=action._body()) def classify(self, content_type: ContentType | str) -> File: """Assign a content type to this file (ContentType object or path string). @@ -472,7 +475,7 @@ def classify(self, content_type: ContentType | str) -> File: Raises: ValueError: If this file has not been created/retrieved yet. """ - self._facet("classify", content_type) + self._facet(FacetAction.classify(content_type)) return self def unclassify(self, content_type: ContentType | str) -> File: @@ -484,7 +487,7 @@ def unclassify(self, content_type: ContentType | str) -> File: Returns: `self`. """ - self._facet("unclassify", content_type) + self._facet(FacetAction.unclassify(content_type)) return self def set_attribute( @@ -501,7 +504,7 @@ def set_attribute( Returns: `self`. """ - self._facet("set_value", content_type, attribute_name=name, value=value) + self._facet(FacetAction.set_attribute(content_type, name, value)) return self def clear_attribute(self, content_type: ContentType | str, name: str) -> File: @@ -514,9 +517,64 @@ def clear_attribute(self, content_type: ContentType | str, name: str) -> File: Returns: `self`. """ - self._facet("clear_value", content_type, attribute_name=name) + self._facet(FacetAction.clear_attribute(content_type, name)) return self + def batch_facets( + self, actions: Sequence[FacetAction | dict[str, Any]] + ) -> _list[FacetResult]: + """Apply up to 50 classification writes in one request (POST /files//facets/batch). + + The batch form of classify/unclassify/set_attribute/clear_attribute. Build + the list with `FacetAction`, whose constructors take the same arguments as + those methods, so a batch is a transcription of the calls it replaces: + + doc.batch_facets([ + FacetAction.classify("legal:contract:nda"), + FacetAction.set_attribute("legal:contract:nda", "jurisdiction", "FR"), + ]) + + The list is inert until it gets here, so the same one applies to many + files. Beyond saving round trips, the document is reindexed once per batch + instead of once per action, which is the real win on a full classification. + + **Not transactional.** Field-level mistakes are caught up front, so a + malformed action applies nothing. A domain error (unknown content type, + setting a value before classifying, a sibling conflict) stops at that + action: everything before it is already applied, and the raised error + carries its 0-based position on `.index`. Every action is idempotent, so + correct that one and resend the whole list. + + Args: + actions: The actions, in order, at most 50. `FacetAction` objects, or + raw action bodies as dicts for a verb the SDK doesn't model yet + (mix freely). Empty is a local no-op. A longer job is yours to + split: chunking here would forfeit the single reindex and report + an index relative to a batch you never wrote. + + Returns: + One FacetResult per action, in the order sent, so `results[i]` belongs + to `actions[i]`. Only ever complete: a failure raises instead. + + Raises: + ValueError: If this file has not been created/retrieved yet, or more + than 50 actions are passed (refused here to save the round trip). + LightOnAPIError: If an action fails. `.index` is the one that did, and + the actions before it are already applied. + """ + if not actions: + return [] + if len(actions) > MAX_FACET_ACTIONS: + raise ValueError( + f"a batch takes at most {MAX_FACET_ACTIONS} actions, got " + f"{len(actions)}; send them {MAX_FACET_ACTIONS} at a time" + ) + bodies = [a._body() if isinstance(a, FacetAction) else a for a in actions] + data = self._api( + "POST", f"{_BASE}/{self.id}/facets/batch", json={"actions": bodies} + ) + return [FacetResult.model_validate(r) for r in data["results"]] + def facets(self) -> _list[Facet]: """List this file's assigned content types and their attribute values. diff --git a/tests/e2e/cli.py b/tests/e2e/cli.py index ce3c58c..9434dcf 100644 --- a/tests/e2e/cli.py +++ b/tests/e2e/cli.py @@ -40,8 +40,10 @@ DownloadPurpose, ExecMode, ExternalMetadata, + FacetAction, File, LightOn, + MAX_FACET_ACTIONS, RelevanceScoring, Role, SearchMode, @@ -52,7 +54,7 @@ Workspace, wait_all, ) -from lighton.exceptions import NotFoundError +from lighton.exceptions import LightOnAPIError, NotFoundError DOCS_DIR = Path(__file__).parent / "documents" JOB_TIMEOUT = 300.0 @@ -388,6 +390,60 @@ def _sample(attr: Attribute) -> object: return None +@step +def facet_batch(c: Ctx) -> None: + """facets/batch: many writes in one request, the fail-fast index, the 50 cap.""" + f = c.uploaded() + ct = c.content_type + if ct is None: + _say("nothing classified — run with --only content_types --only facet_batch") + return + + # Only `ct`: classifying a sibling from the same tree is a 400 by design. + actions: list[FacetAction | dict[str, object]] = [FacetAction.classify(ct)] + attr = next((a for a in ct.attributes if _sample(a) is not None), None) + if attr is not None: + actions += [ + FacetAction.set_attribute(ct, attr.name, _sample(attr)), + FacetAction.clear_attribute(ct, attr.name), + # restore: facet_filters (the next step) filters on this value + FacetAction.set_attribute(ct, attr.name, _sample(attr)), + ] + + results = f.batch_facets(actions) + assert len(results) == len(actions), ( + f"{len(results)} result(s) for {len(actions)} action(s)" + ) + assert all(r.status < 300 for r in results), ( + f"batch reported {[r.status for r in results]}" + ) + _say(f"{len(actions)} action(s) in one request → {[r.status for r in results]}") + + # A domain error fails fast and names the offender; what came before it sticks. + try: + f.batch_facets( + [FacetAction.classify(ct), FacetAction.classify(f"no-such-{c.stamp}")] + ) + except LightOnAPIError as e: + assert e.index == 1, f"expected index 1, got {e.index}" + _say(f"fail-fast reported index {e.index}") + else: + raise AssertionError("an unknown content type should have failed the batch") + + assert any(x.path == ct.path for x in f.facets()), ( + "the committed prefix did not stick" + ) + + assert not f.batch_facets([]), "an empty batch should not have hit the API" + + try: + f.batch_facets([FacetAction.classify(ct)] * (MAX_FACET_ACTIONS + 1)) + except ValueError: + _say(f"over {MAX_FACET_ACTIONS} actions: refused client-side, no round trip") + else: + raise AssertionError(f"{MAX_FACET_ACTIONS + 1} actions should be refused") + + @step def facet_filters(c: Ctx) -> None: """content_type= / attribute= on search and ask (needs the content_types step).""" diff --git a/tests/test_client.py b/tests/test_client.py index 4c6b2e2..9f71750 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -66,6 +66,26 @@ def test_error_mapping(status, expected): assert excinfo.value.status_code == status +def test_api_error_has_no_index_outside_a_batch(): + """Single-action requests never carry one, which is what makes `index` readable.""" + client = make_client(lambda req: httpx.Response(404, json={"detail": "nope"})) + with pytest.raises(exc.NotFoundError) as excinfo: + client.ask("q") + assert excinfo.value.index is None + assert "action" not in str(excinfo.value) + + +def test_api_error_carries_the_batch_action_index(): + """Index 0 must survive: it is a real position, not a falsy sentinel.""" + client = make_client( + lambda req: httpx.Response(400, json={"detail": "unknown type", "index": 0}) + ) + with pytest.raises(exc.LightOnAPIError) as excinfo: + client.ask("q") + assert excinfo.value.index == 0 + assert "(action 0)" in str(excinfo.value) + + def test_rate_limit_exposes_retry_after(): client = make_client( lambda req: httpx.Response( diff --git a/tests/test_file.py b/tests/test_file.py index 3bece80..0b71a8e 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -10,8 +10,11 @@ from urllib.parse import parse_qs from lighton import ( + MAX_FACET_ACTIONS, DownloadPurpose, ExternalMetadata, + FacetAction, + FacetActionType, File, FileStatus, LightOn, @@ -21,7 +24,7 @@ ThumbnailStatus, Workspace, ) -from lighton.exceptions import NotFoundError +from lighton.exceptions import LightOnAPIError, NotFoundError from lighton.types.api import Page @@ -326,6 +329,203 @@ def handler(request: httpx.Request) -> httpx.Response: ] +def _facet_file(handler) -> File: + """A persisted File bound to a mocked client, for the facet write tests.""" + f = File(id=7, workspace_id=3) + f._client = _make_client(handler) + return f + + +def _no_request(request: httpx.Request) -> httpx.Response: + raise AssertionError(f"no request should be made, got {request.url}") + + +def test_batch_facets_posts_every_action_in_one_request(): + from lighton import ContentType + + requests = [] + + def handler(request: httpx.Request) -> httpx.Response: + requests.append(request) + return httpx.Response( + 200, json={"results": [{"status": 201, "data": None}] * 4} + ) + + f = _facet_file(handler) + # ContentType object and bare path string both accepted, as everywhere else + ct = ContentType(path="legal:contract:nda", code="nda", label="NDA") + f.batch_facets( + [ + FacetAction.classify(ct), + FacetAction.set_attribute( + "legal:contract:nda", "jurisdiction", ["FR", "DE"] + ), + FacetAction.set_attribute("legal:contract:nda", "signed", True), + FacetAction.clear_attribute(ct, "draft_note"), + ] + ) + + assert len(requests) == 1, "a batch must be exactly one round trip" + assert requests[0].url.path == "/api/v3/files/7/facets/batch" + assert json.loads(requests[0].content) == { + "actions": [ + {"action": "classify", "content_type_path": "legal:contract:nda"}, + { + "action": "set_value", + "content_type_path": "legal:contract:nda", + "attribute_name": "jurisdiction", + "value": ["FR", "DE"], + }, + { + "action": "set_value", + "content_type_path": "legal:contract:nda", + "attribute_name": "signed", + "value": True, + }, + { + "action": "clear_value", + "content_type_path": "legal:contract:nda", + "attribute_name": "draft_note", + }, + ] + } + + +def test_batch_facets_sends_the_same_bodies_as_the_single_action_methods(): + """FacetAction._body() is the one wire encoder, so the two paths can't drift.""" + single, batched = [], [] + + def handler(request: httpx.Request) -> httpx.Response: + body = json.loads(request.content) + if request.url.path.endswith("/facets/batch"): + batched.extend(body["actions"]) + return httpx.Response( + 200, json={"results": [{"status": 200, "data": None}] * 4} + ) + single.append(body) + return httpx.Response(200, json={}) + + ct = "legal:contract:nda" + f = _facet_file(handler) + f.classify(ct) + f.set_attribute(ct, "jurisdiction", "FR") + f.clear_attribute(ct, "jurisdiction") + f.unclassify(ct) + f.batch_facets( + [ + FacetAction.classify(ct), + FacetAction.set_attribute(ct, "jurisdiction", "FR"), + FacetAction.clear_attribute(ct, "jurisdiction"), + FacetAction.unclassify(ct), + ] + ) + + assert single == batched + + +def test_batch_facets_parses_results_in_order(): + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response( + 200, + json={ + "results": [ + { + "status": 201, + "data": { + "content_type_path": "legal:contract:nda", + "label": "NDA", + }, + }, + {"status": 204, "data": None}, + { + "status": 200, + "data": {"name": "jurisdiction", "value": "FR"}, + }, + ] + }, + ) + + results = _facet_file(handler).batch_facets( + [ + FacetAction.classify("legal:contract:nda"), + FacetAction.clear_attribute("legal:contract:nda", "draft_note"), + FacetAction.set_attribute("legal:contract:nda", "jurisdiction", "FR"), + ] + ) + + assert [r.status for r in results] == [201, 204, 200] + assert results[1].data is None, "the 204 verbs carry no data" + assert results[2].data == {"name": "jurisdiction", "value": "FR"} + + +def test_batch_facets_refuses_more_than_fifty_actions(): + action = FacetAction.classify("legal:contract:nda") + + with pytest.raises(ValueError, match="50"): + _facet_file(_no_request).batch_facets([action] * (MAX_FACET_ACTIONS + 1)) + + # and the boundary itself is accepted: an off-by-one here is the plausible bug + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response( + 200, json={"results": [{"status": 200, "data": None}] * MAX_FACET_ACTIONS} + ) + + assert ( + len(_facet_file(handler).batch_facets([action] * MAX_FACET_ACTIONS)) + == MAX_FACET_ACTIONS + ) + + +def test_batch_facets_is_a_local_no_op_when_empty(): + assert _facet_file(_no_request).batch_facets([]) == [] + + +def test_batch_facets_passes_raw_dicts_through(): + sent = {} + + def handler(request: httpx.Request) -> httpx.Response: + sent["body"] = json.loads(request.content) + return httpx.Response( + 200, json={"results": [{"status": 200, "data": None}] * 2} + ) + + raw = {"action": "some_future_verb", "content_type_path": "legal", "extra": 1} + _facet_file(handler).batch_facets([FacetAction.classify("legal"), raw]) + + assert sent["body"]["actions"][1] == raw, "a raw dict must reach the wire untouched" + + +def test_batch_facets_reports_the_failing_action_index(): + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(422, json={"detail": "unknown content type", "index": 2}) + + actions = [ + FacetAction.classify("legal:contract:nda"), + FacetAction.set_attribute("legal:contract:nda", "jurisdiction", "FR"), + FacetAction.classify("nope:not-a-type"), + ] + with pytest.raises(LightOnAPIError) as excinfo: + _facet_file(handler).batch_facets(actions) + + assert excinfo.value.index == 2 + assert "action 2" in str(excinfo.value), "the position belongs in the message too" + # which is what makes the offender addressable, and the prefix already applied + assert actions[excinfo.value.index].content_type_path == "nope:not-a-type" + + +def test_facet_action_rejects_a_value_action_without_an_attribute_name(): + # pydantic's ValidationError subclasses ValueError; refused before any request + with pytest.raises(ValueError, match="attribute_name"): + FacetAction( + action=FacetActionType.set_value, content_type_path="legal:contract:nda" + ) + + +def test_batch_facets_requires_a_persisted_file(): + with pytest.raises(ValueError): + File(workspace_id=3).batch_facets([FacetAction.classify("legal")]) + + def test_facets_parses_assigned_content_types(tmp_path): doc = tmp_path / "a.txt" doc.write_text("x") From fd86f8a7912f148085b47bc5de74bca2135a9be3 Mon Sep 17 00:00:00 2001 From: Emmanuel Sandorfi Date: Thu, 24 Sep 2026 18:37:29 +0200 Subject: [PATCH 2/2] update from review --- AGENTS.md | 4 ++++ README.md | 5 ++--- lighton/content_type.py | 4 +++- tests/e2e/cli.py | 4 ++-- tests/test_file.py | 23 +++++++++++++++++------ 5 files changed, 28 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c3a1777..846c97b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -372,6 +372,10 @@ models no facet fields locally. dicts are still accepted alongside `FacetAction`, so the escape hatch is what the two surfaces share. `ContentType.batch` was deliberately left untouched: it is shipped public API and retyping its return would break `r["status"]` for everyone. + `FacetAction` is the one curated model with **`extra="forbid"`**: `ignore` suits + read models (drop response noise), but on a write model it would silently drop a + misspelled field from the body, so a typo raises instead (a test pins it). Unmodelled + fields go through a raw dict. - **Naming split.** The `FacetAction` constructors carry the **SDK's** method names (`classify`/`unclassify`/`set_attribute`/`clear_attribute`) so a batch is a mechanical transcription of the one-by-one calls it replaces; `FacetActionType` diff --git a/README.md b/README.md index 8d34b53..23fbfe8 100644 --- a/README.md +++ b/README.md @@ -963,9 +963,8 @@ print([r["status"] for r in results]) # [201, 201] ``` This is the taxonomy-side sibling of -[`batch_facets()`](#many-writes-in-one-request), with the same fail-fast behavior -and the same 50-action cap. The entries stay raw dicts here because the taxonomy -spans five different action shapes (`adopt` takes a path list, `define_content_type` +[`batch_facets()`](#many-writes-in-one-request). The entries stay raw dicts here +because the taxonomy spans five different action shapes (`adopt` takes a path list, `define_content_type` takes code/label/parent, `define_attribute` takes seven fields), whereas a file facet action has exactly one shape, which is what `FacetAction` models. diff --git a/lighton/content_type.py b/lighton/content_type.py index 7d0dd5a..42a4517 100644 --- a/lighton/content_type.py +++ b/lighton/content_type.py @@ -339,7 +339,9 @@ class FacetAction(BaseModel): Nothing is sent until the list reaches a file, so one list applies to many. """ - model_config = ConfigDict(extra="ignore") + # forbid, not ignore: this is a write model, a misspelled field must fail + # loudly rather than vanish from the body. Raw dicts are the unmodelled escape. + model_config = ConfigDict(extra="forbid") action: FacetActionType = Field( description="The write to perform, the API's verb (see FacetActionType)." diff --git a/tests/e2e/cli.py b/tests/e2e/cli.py index 9434dcf..d104853 100644 --- a/tests/e2e/cli.py +++ b/tests/e2e/cli.py @@ -396,7 +396,7 @@ def facet_batch(c: Ctx) -> None: f = c.uploaded() ct = c.content_type if ct is None: - _say("nothing classified — run with --only content_types --only facet_batch") + _say("nothing classified, run with --only content_types --only facet_batch") return # Only `ct`: classifying a sibling from the same tree is a 400 by design. @@ -450,7 +450,7 @@ def facet_filters(c: Ctx) -> None: ws = c.workspace() ct = c.content_type if ct is None: - _say("nothing classified — run with --only content_types --only facet_filters") + _say("nothing classified, run with --only content_types --only facet_filters") return query = c.search_query or _topic(c) diff --git a/tests/test_file.py b/tests/test_file.py index 0b71a8e..1bb1a75 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -11,6 +11,7 @@ from lighton import ( MAX_FACET_ACTIONS, + ContentType, DownloadPurpose, ExternalMetadata, FacetAction, @@ -24,7 +25,7 @@ ThumbnailStatus, Workspace, ) -from lighton.exceptions import LightOnAPIError, NotFoundError +from lighton.exceptions import NotFoundError from lighton.types.api import Page @@ -294,7 +295,6 @@ def test_get_by_name_requires_a_persisted_workspace(): def test_classify_and_attributes_post_actions(tmp_path): doc = tmp_path / "a.txt" doc.write_text("x") - from lighton import ContentType bodies = [] @@ -341,8 +341,6 @@ def _no_request(request: httpx.Request) -> httpx.Response: def test_batch_facets_posts_every_action_in_one_request(): - from lighton import ContentType - requests = [] def handler(request: httpx.Request) -> httpx.Response: @@ -496,15 +494,17 @@ def handler(request: httpx.Request) -> httpx.Response: def test_batch_facets_reports_the_failing_action_index(): + """`index` rides on the status-mapped class, so `except NotFoundError` still works.""" + def handler(request: httpx.Request) -> httpx.Response: - return httpx.Response(422, json={"detail": "unknown content type", "index": 2}) + return httpx.Response(404, json={"detail": "unknown content type", "index": 2}) actions = [ FacetAction.classify("legal:contract:nda"), FacetAction.set_attribute("legal:contract:nda", "jurisdiction", "FR"), FacetAction.classify("nope:not-a-type"), ] - with pytest.raises(LightOnAPIError) as excinfo: + with pytest.raises(NotFoundError) as excinfo: _facet_file(handler).batch_facets(actions) assert excinfo.value.index == 2 @@ -521,6 +521,17 @@ def test_facet_action_rejects_a_value_action_without_an_attribute_name(): ) +def test_facet_action_rejects_a_misspelled_field(): + # a write model: a typo must not silently vanish from the request body + with pytest.raises(ValueError, match="atribute_name"): + FacetAction( + action=FacetActionType.clear_value, + content_type_path="legal:contract:nda", + attribute_name="jurisdiction", + atribute_name="typo", # ty: ignore[unknown-argument] + ) + + def test_batch_facets_requires_a_persisted_file(): with pytest.raises(ValueError): File(workspace_id=3).batch_facets([FacetAction.classify("legal")])