From b4a2cce09965f052fb68a2e766ab17e5848541c3 Mon Sep 17 00:00:00 2001 From: jgjesdal Date: Wed, 23 Sep 2026 09:36:27 +0200 Subject: [PATCH 1/2] docs: correct the statements the api no longer matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The errors/* unification and platform #131 landed on the backend, and a number of statements here describe behaviour it no longer has. Each was checked against the backend source and, where it could be, against a live backend. Refusals: - a `GET //{id}` miss carries `type: .../not-found` now; `ObjectNotFoundExceptionHandler` calls `Problems.notFound()` instead of hand-building the document. That was the last typeless refusal, so `problem_integration`'s `pending` group is empty and its one test moves to `unified` as a regression guard. - a 401 carries a full problem document naming the check that failed. - the MCP double-encoding regression is fixed, so nothing in that suite is red on purpose; same for `test_duplicate_relationship_type_conflicts`, which is a 409 now. All stay on as guards. - `geoLocation` on a plain resource and `dataSetId` on a DATASET/POLICY are 400s rather than silent drops, and a duplicate `externalId` on `/resources/create` is a clean 409 rather than a constraint violation. Reads and filters: - `/functions/byids`, `/filter` and `/search` exist; the SDK has not wired them, which is a different statement from the api lacking them. - graph reads carry `metadata`; `/datasets/filter` pages; `GET /events/{id}` is a 404; `/events/search` is an unranked substring match over externalId, description and metadata values, newest first. - a literal `externalId` on the event filter matches case-sensitively, unlike every other pattern list. - an absent event sort is `eventTime` ascending, not "no particular order". Deletes docs/api-coverage.md: a point-in-time audit baselined against a backend and an SDK from before all of this, now wrong in nearly every row — it lists /edges as uncovered, the dataset methods as panicking stubs, and omits /assets entirely. AGENTS.md carries the living per-service contract. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: jgjesdal --- AGENTS.md | 141 +++++++++++---------- docs/api-coverage.md | 212 -------------------------------- src/assets/mod.rs | 5 +- src/datasets/mod.rs | 8 +- src/events/mod.rs | 18 ++- src/filters.rs | 31 +++-- src/functions/mod.rs | 17 +-- src/http.rs | 5 +- src/mcp_integration.rs | 38 +++--- src/multi_tenant_integration.rs | 46 +++---- src/nodes.rs | 18 +-- src/problem.rs | 11 +- src/problem_integration.rs | 105 +++++++--------- src/relations/mod.rs | 9 +- src/relations/service.rs | 19 ++- src/relations/tests.rs | 19 ++- src/resources/mod.rs | 26 ++-- 17 files changed, 272 insertions(+), 456 deletions(-) delete mode 100644 docs/api-coverage.md diff --git a/AGENTS.md b/AGENTS.md index 29c77d6..da76ca8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,7 +55,7 @@ cargo test multi_tenant_integration -- --ignored --nocapture --test-threads=1 ./run_python_tests.sh -k multi_tenant ``` -They need six purpose-built Keycloak principals beyond the usual `.env` (a two-organization one, read-only/write-only/no-grant ones, and a full-access one per organization), configured through `MT_*` variables. Each test skips with a printed note when its fixture is absent, so a checkout without that realm setup is unaffected. **The Rust module's `//!` doc is the reference** — it carries the env contract, the organization-group naming convention, the realm-level mapper prerequisites, and the reason 401s here can only be asserted on status (the API drops the explanation before it reaches any client). +They need six purpose-built Keycloak principals beyond the usual `.env` (a two-organization one, read-only/write-only/no-grant ones, and a full-access one per organization), configured through `MT_*` variables. Each test skips with a printed note when its fixture is absent, so a checkout without that realm setup is unaffected. **The Rust module's `//!` doc is the reference** — it carries the env contract, the organization-group naming convention, the realm-level mapper prerequisites, and the reason a 401's cause lives in the problem's `detail` rather than in its `type` (every entry-point 401 shares the `unauthorized` slug). ## Backend @@ -65,13 +65,13 @@ The DataHub REST API this SDK targets is a separate Spring Boot project; the HTT This crate is a thin async HTTP SDK around a DataHub-style REST API. Entry point is `create_api_service()` in `src/lib.rs`, which returns an `Arc` built with `Arc::new_cyclic` so each subservice holds a `Weak` back-reference. Subservices are fields on `ApiService`: -- `time_series` (`src/timeseries/`) — `TimeSeries` + datapoint ingestion/retrieval. Neither `TimeSeries` nor `TimeSeriesUpdate` has **`securityCategories`**: it was stored, writable and returned, but nothing ever read it — no part in access control (dataset grants are Keycloak organization groups), no query filtering on it, and the backend silently dropped any id that did not already exist, so the field never round-tripped. It has been removed server-side along with its join table, and the api reads request bodies strictly, so sending it is now a 400. Files keep their own `securityCategories` (`INode` in `src/generic.rs`) — separate entity, separate question. `ListFieldU64` went with it: it was the only field of that type, so the Python wrapper class is gone too (`ListFieldStr` and `ListFieldIdCollection` remain). **`tableEngine`** went the same way. No read had returned it since the api marked it `@JsonIgnore` — which ClickHouse engine backs a series is an internal storage decision — and it has now been removed from the api entirely, so the write side that made it worth keeping as a write-only field is gone too. +- `time_series` (`src/timeseries/`) — `TimeSeries` + datapoint ingestion/retrieval. Neither `TimeSeries` nor `TimeSeriesUpdate` has **`securityCategories`**: it was stored, writable and returned, but nothing ever read it — no part in access control (dataset grants are Keycloak organization groups), no query filtering on it, and the backend silently dropped any id that did not already exist, so the field never round-tripped. It has been removed server-side along with its join table, and the api reads request bodies strictly, so sending it is now a 400. Files keep their own `securityCategories` (`INode` in `src/generic.rs`) — separate entity, separate question. `ListFieldU64` went with it: it was the only field of that type, so the Python wrapper class is gone too (`ListFieldStr` and `ListFieldIdCollection` remain). **`tableEngine`** went the same way, but is *not* a 400 everywhere: no read had returned it since the api marked it `@JsonIgnore` — which ClickHouse engine backs a series is an internal storage decision — and the write side is gone. `Timeseries` keeps it in a `@JsonIgnoreProperties` list so an older SDK's body is **accepted and the field ignored**; the update form (`TimeseriesFields`) has no such list, so sending it *there* is a 400. - `units` (`src/unit/`) - `events` (`src/events/`) — event CRUD, filter/search, plus the vocabulary endpoints (`list_types`/`search_types` and the same pair for sub-types, statuses and sources, over `EventDimension`). Those answer "what values does this tenant actually use" for the four categorical fields and back filter dropdowns; they read small server-side dimension tables rather than scanning events, so they are cheap but *eventually consistent* with the events. Note the route asymmetry the SDK hides: `/events/list/{plural}` but `/events/search/{singular}`. `EventUpdate` has **no `event_time` and no `external_id`**: both identify an event rather than describe it, and each was dropped from the api's update form after it had spent a while validating the field, echoing the new value back with a 200 and then failing to apply it — so sending either is now a 400 naming it. The events table is partitioned by `event_time`, so ClickHouse refuses that mutation outright. `externalId` maps to the *set* of event UUIDs behind it, events sharing one being the lifecycle of a single logical event, so a rename would take every sibling along — and when the caller identified the event by UUID the server had no old value to re-key with, leaving the "renamed" event resolvable under neither id. Re-key by creating a new event and deleting the old; record a corrected time the same way. - `resources` (`src/resources/`) — the generic node service. Its reads span **every** node type and answer with [`Node`](#the-polymorphic-node-type) rather than one flat shape; relationship edges live in `src/relations/` (`EdgeProxy`, `RelForm`, `RelatedNode`) - `edges` (`src/relations/service.rs`) — the `/edges` endpoints: `get`/`by_ids`/`create`/`delete` plus the relationship-type catalogue (`types`/`create_types`). Edges normally come into being through `resources.create(nodes, relations)`; this service is for linking resources that already exist and for reading or deleting an edge on its own. `get` answers an unknown id with 404 and a `problem+json` body; `by_ids`, like every batch lookup, answers 200 with the found subset and silently omits what is missing. (`get` used to be 200-and-nothing despite documenting a 404 — api #275 made single-resource by-id GETs consistently 404 and deliberately left batch lookups alone.) Two further behaviours are worth knowing, and are documented at each call site: - - `create_types` fails silently on a duplicate name — the unique-hash collision surfaces at commit, after the handler returned, so the caller gets a 200 with an empty *body*, and in a batch the valid new types are rolled back with it. This one does contradict the OpenAPI: `test_duplicate_relationship_type_conflicts` encodes the intended 409 and is red until the server-side fix lands. + - `create_types` answers a duplicate name with **409** `duplicate`, naming `name` in `fields`. It used to fail silently — the unique-hash collision surfaced at commit, after the handler returned, so the caller got a 200 with an empty *body*. `test_duplicate_relationship_type_conflicts` encoded the intended 409 while that was true and is a regression guard now. Still true, and still worth knowing: the service has **no find-or-create**, and a batch is one transaction, so a single duplicate rolls back the valid new types alongside it. - `delete` will not remove an edge that is an endpoint's only route to the graph root: `ResourceService.delete` refuses rather than orphan the node, answering **409** `would-strand` with the stranded resource in the problem's `blockedBy` (`[{"externalId": …}]`) and a message saying to include it in the deletion or keep a connecting path — read it through `ResponseError::problem()`. (It was a 400 carrying the old `{"error":{"code","fields"}}` envelope before the api unified on RFC 9457.) Practically, an edge is separately deletable only when both endpoints stay reachable without it; otherwise it goes away with the resources. The check reads the graph projection, which lags the write, so deleting too soon after creating the edge gets the *wrong answer* rather than an error — the refusal does not fire and the node is stranded. `relations::tests` measured 6/6 wrongly allowed immediately after create, 6/6 refused 500ms later; its `await_graph` helper is what the live tests wait on. - `assets` (`src/assets/`) — the typed `/assets` family: create, `get_by_id`, `by_ids`, `list`, `filter`, `search`, `update`, `delete`. Every call is the `/resources` pipeline with the `ASSET` @@ -88,9 +88,10 @@ This crate is a thin async HTTP SDK around a DataHub-style REST API. Entry point - `files` (`src/files/`) — raw-`PUT` upload via `execute_file_upload_request` (content is the body, metadata rides in `X-Datahub-*` headers), plus directory listing, get/search, `FileUpdate` (rename/move/re-dataset), trash + restore, delete, and download (`download` in memory, `download_to_path` streamed) - `subscriptions` (`src/subscriptions/`) — subscription CRUD, plus `listen.rs`: WebSocket listening against the api's subscription-listen endpoint (`tokio-tungstenite`). Reads follow the same split as every other collection: `list(limit)` over `GET /subscriptions?limit=` and `filter(form)` over `POST /subscriptions/filter`. Both are recent — `POST /subscriptions/list` was subscriptions-only (a `limit` defaulting to 100 where the api defaulted to 1000, an unvalidated sort property, and no cursor, so a tenant past one page could not reach the rest) and the api removed it rather than aliasing it, so a client that has not moved gets a 404. `SubscriptionFilterForm` is a strict subset of what `/filter` now accepts: it carries no `cursor`, and `SubscriptionFilter` has only `timeseries`, not the `id`, `externalId`, `name`, `createdTime` and `lastUpdatedTime` criteria the api's filter grew beside it. - `functions` (`src/functions/`) — `create`, `list`, `get_by_id`, `update`, `delete`, plus a - client-side `by_ids`/`by_external_id`. The api serves **no `/byids`, `/filter` or `/search`** for - functions — the only node type missing all three — so `by_ids` lists and filters locally, asking - for the largest page the api allows; a tenant past 10000 functions silently misses its oldest. + client-side `by_ids`/`by_external_id`. The api **does** serve `/functions/byids`, `/filter` and + `/search` (platform #131) — **the SDK has not wired them yet**, so `by_ids` still lists and + filters locally, asking for the largest page the api allows; a tenant past 10000 functions + silently misses its oldest. Wiring the three is the fix, not a bigger page. `Function::related_resources` is **always empty**: `FunctionTransformer` never joins the edges in, and unlike `/resources/create` the create echo is no exception, because it re-reads the rows through that same transformer. `name` is `Option` here but non-null on the api. @@ -142,12 +143,17 @@ Behaviours worth knowing, each pinned by a test in `src/nodes.rs`: which are the same shared pipeline. - **Policies never carry `value`, `template_id` or `data_set_id`** on a read, and their `metadata` can be outright `null`. -- **`Resource::geolocation` is write-only** server-side: accepted on create, never echoed. Assets +- **`Resource::geolocation` is not a resource field at all** any more: only `Asset` declares it, so + an unlabelled create body carrying `geoLocation` is a **400** naming it as an unknown field. It + used to be accepted-and-never-echoed. The shared *update* form still takes it for any type and + applies it only to assets. Assets carry it. - **Every type is creatable through `/resources/create`, timeseries included** — each element of `nodes` is dispatched by its own labels. `DATASET` and `POLICY` need the all-datasets manage - grant (403 without), and their `data_set_id` is silently dropped. A duplicate `external_id` - surfaces as a constraint violation rather than the clean 409 `/timeseries/create` gives. + grant (403 without), and their `data_set_id` is a **400** telling you to remove it — hierarchy is + expressed with `BELONGS_TO` edges — not the silent drop it once was. The grant check runs first, + so a caller without it still sees the 403. A duplicate `external_id` is a clean **409** carrying + the value in `duplicated`, the same as `/timeseries/create`; the old asymmetry is gone. In Python each variant maps to its own pyclass, so `isinstance(node, TimeSeries)` works and an object from `resources.filter()` behaves exactly like one from `timeseries.by_ids()`. The dispatch @@ -223,26 +229,26 @@ assertions are unaffected. Those answer different questions — don't reconcile them without deciding which one ingestion means. **The api's `errors/*` series has landed**, so every refusal now answers `application/problem+json` -with a `type` — with one exception, below. Before it, six shapes were live at once (full problem, +with a `type`. Before it, six shapes were live at once (full problem, typeless problem, Spring Boot whitelabel *with a stack trace*, a success-shaped `{"items":[…]}` envelope, the legacy `{"error":{…}}` wrapper, and plain text); `problem_integration`'s module doc keeps the table of what each became, because that is what its assertions are pinning against a -revert. Its three groups: `green` was true before and after, `unified` arrived with the series (red -on purpose until it merged, regression guards now), and `pending` is what is still outstanding. - -**The one gap: a `GET //{id}` miss carries no `type`.** Every by-id 404 goes through the -shared `ObjectNotFoundExceptionHandler`, which hand-builds its document with -`ProblemDetail.forStatusAndDetail` + `setTitle` instead of calling `Problems.notFound()` — which -exists, sets `type("not-found")`, and is already used by `FileController` and `TimeseriesController`. -Easy to miss because `Problems.decorate` still runs on it, so the body carries `requestId` and -`retry` and looks finished. `slug()` is `None` there, so **don't match a by-id 404 on -`"not-found"` yet**; `problem_integration::pending` is red on purpose until it is fixed. - -Two statements elsewhere in this file describe the pre-unification shape. 401s now *do* carry a -problem body, so the multi-tenant note and `src/auth_diagnostics.rs` (which reconstructs a reason -the api used to withhold) are worth re-reading against the current api before relying on them. A -refused delete is now a **409** `would-strand`/`referenced` carrying its blockers in `blockedBy`, -not the **400** with `fields` the `edges` note describes. +revert. Its groups: `green` was true before and after, and `unified` arrived with the series (red +on purpose until it merged, regression guards now). + +**The last gap closed.** A `GET //{id}` miss used to hand-build its document with +`ProblemDetail.forStatusAndDetail` + `setTitle`, so it reached the caller as `about:blank` with no +`type` while `Problems.decorate` still gave it a `requestId` and a `retry` — finished-looking and +unmatchable. `ObjectNotFoundExceptionHandler` now calls `Problems.notFound()` like every other +refusal, so a by-id 404 carries `type: …/errors/not-found` and **`slug()` matches `"not-found"`**. +`problem_integration`'s `pending` group was red on purpose until that landed; it is green now and +stays as the regression guard. + +401s carry a problem document too, with `type: …/errors/unauthorized` and a `detail` naming the +check that failed — a missing, empty, malformed or ambiguous `organization` claim. That is the +reason `src/auth_diagnostics.rs` reconstructs from the token it just sent, so the SDK now appends +a near-duplicate of what the server already said. Note every entry-point 401 shares the +`unauthorized` slug, so the cause is in the prose and cannot be branched on. ### Filters (`src/filters.rs`) @@ -267,11 +273,11 @@ state between them. Before 0.3.0 the Python side had three shapes at once — an and datasets, a flat `TimeSeriesFilterForm` that was really the criteria, and bare keywords on resources — and `timeseries.search` silently discarded the paging fields of the form it was handed. -The four `/{entity}/filter` endpoints share one contract. `NodeFilter` (`src/filters.rs`) is the criteria every node type can be filtered by — `id`, `externalId`, `name`, `source`, `labels`, `metadata`, `createdTime`, `lastUpdatedTime` — and `ResourceFilter`, `TimeSeriesFilter` and `DatasetFilter` each `#[serde(flatten)]` it, so on the wire its fields sit alongside the type-specific ones. `EventFilter` deliberately does **not** extend it (events are not nodes: no `name` column, a UUID id) but matches it field for field wherever ClickHouse can back it. +The `/{entity}/filter` endpoints share one contract — `resources`, `timeseries`, `datasets` and `events` here, plus `assets` and `functions`, which reuse the resource and node criteria rather than declaring their own. `NodeFilter` (`src/filters.rs`) is the criteria every node type can be filtered by — `id`, `externalId`, `name`, `source`, `labels`, `metadata`, `createdTime`, `lastUpdatedTime` — and `ResourceFilter`, `TimeSeriesFilter` and `DatasetFilter` each `#[serde(flatten)]` it, so on the wire its fields sit alongside the type-specific ones. `EventFilter` deliberately does **not** extend it (events are not nodes: no `name` column, a UUID id) but matches it field for field wherever ClickHouse can back it. The rules, which every one of them obeys: -- **Patterns.** `externalId`, `name`, `source` — plus `unit`/`unitExternalId` on timeseries and `type`/`subType`/`status` on events — are pattern lists. `*` and `%` are both wildcards; `_` is **literal**, because identifiers here are built out of underscores and raw `LIKE` would make `sap_work_orders` also match `sapXwork_orders`. Matching is case-insensitive. An entry with no wildcard matches exactly, and resolves through the indexed hash where one exists. +- **Patterns.** `externalId`, `name`, `source` — plus `unit`/`unitExternalId` on timeseries and `type`/`subType`/`status` on events — are pattern lists. `*` and `%` are both wildcards; `_` is **literal**, because identifiers here are built out of underscores and raw `LIKE` would make `sap_work_orders` also match `sapXwork_orders`. Matching is case-insensitive, with **one exception**: a no-wildcard `externalId` entry on the *event* filter matches case-sensitively, because event writers hash the external id verbatim where the node filters lowercase first. An entry with no wildcard matches exactly, and resolves through the indexed hash where one exists. - **Singular names, list values.** Every criterion above is a list, and every one is named in the singular, because the api declares them `@SingleOrList`: a bare value is accepted wherever a list is, and one value is the common case. `labels` and `relatedResources` keep their plurals — their entries AND rather than OR, so those fields really are about a set. The SDK always sends the list form. - **AND across fields, OR within a list** — except `labels` and `metadata`, where every entry must be present. - **Empty means no restriction**, and so do blank entries and `None`: an empty `IN` is not valid SQL, and a caller who built a list and found nothing to put in it means "no restriction" far more often than "match nothing". @@ -298,7 +304,8 @@ list-like, so existing code is unaffected, but carrying `.next_cursor`. - **One** sort property, plus `id` appended. The tie-breaker is what makes the order *total*: a sort column alone is not a position unless it is unique, so a page boundary inside a run of equal values repeats or drops exactly those rows. An unrecognised property falls back to the default - rather than erroring; anything that is not exactly `desc` sorts ascending. + rather than erroring; the property is trimmed first, and anything that is not `desc` (compared + case-insensitively) sorts ascending. - **Defaults differ.** Nodes: `createdTime` descending, sortable by `id`, `externalId`, `name`, `source`, `description`, `createdTime`, `lastUpdatedTime`, `dataSetId`. Events: `eventTime` **ascending** — the order the cursor pages in — sortable also by `type`, `subType`, `status`. @@ -322,16 +329,15 @@ It spans **every** node type — assets, timeseries, functions, resources, data narrowed by `nodeType` (`["resource", "timeseries"]`, case-insensitive; omitted = all; a list of only unknown names matches *nothing*). It behaved this way before by omission, with no discriminator and single-table inheritance doing the rest; the breadth is now stated and narrowable. What comes -back is typed per row — see [`Node`](#the-polymorphic-node-type). The other three endpoints stay -typed. `DatasetFilter` is consequently just the shared criteria — its `writeProtected` and +back is typed per row — see [`Node`](#the-polymorphic-node-type). The other endpoints stay typed. `DatasetFilter` is consequently just the shared criteria — its `writeProtected` and `deactivated` flags were removed server-side as inert. -#### The four `/search` endpoints share one contract +#### The `/search` endpoints share one contract -`{ "search": { "query": … }, "filter": … , "limit": … }` — the same shape on all four, so one type covers them: `SearchAndFilterForm`, generic over the filter alone, mirroring the api's `SearchBody`. `DatasetSearch` and `EventSearch` were the same three fields written twice more and are gone; the duplication is what let the dataset one keep claiming its filter was ignored for a release after that stopped being true. In Python there is no form class at all — every `search` takes `query`, `filter` and `limit` directly, the way `datasets.search` always did. +`{ "search": { "query": … }, "filter": … , "limit": … }` — the same shape on every one, so one type covers them: `SearchAndFilterForm`, generic over the filter alone, mirroring the api's `SearchBody`. `DatasetSearch` and `EventSearch` were the same three fields written twice more and are gone; the duplication is what let the dataset one keep claiming its filter was ignored for a release after that stopped being true. In Python there is no form class at all — every `search` takes `query`, `filter` and `limit` directly, the way `datasets.search` always did. - **The phrase selects, the filter only removes, `limit` caps what survives.** A filter can never widen a search, and omitting it (`None`) returns the phrase's hits as found; it is skipped rather than sent empty. `search.query` is the one field that is *not* optional — see below. -- **All four apply it.** Resources, datasets and events used to accept a filter and drop it on the floor; `python_tests/test_filter_search_bodies.py` carried those as `xfail(strict=True)` and they went green when the gap closed. +- **All of them apply it.** Resources, datasets and events used to accept a filter and drop it on the floor; `python_tests/test_filter_search_bodies.py` carried those as `xfail(strict=True)` and they went green when the gap closed. - **`search.query` is the only free-text field, and it is required** — 3–140 characters, and nothing else. It used to carry a charset too (`^[\p{IsLatin}\p{Zs}\p{Nd}]+`), which rejected the `._:+=-` that external ids are built from and every non-Latin script with them; it was never a safety control (the phrase is a bound parameter) but a way of guaranteeing the tsquery at least one lexeme, and the empty case is guarded directly now. `name` and `description` used to sit beside it, honoured only by the timeseries search and only one of the three at a time, with `name` matching by *exact equality* under an endpoint documented as full-text. Both are gone from the api: `query` already covers the description column, and the filter's `name` is a case-insensitive pattern list, which is strictly more than the old field could do. `TimeSeriesService::search_by_name`/`search_by_description` went with them — match a name through `filter`. - **Hits are ranked.** The three node searches order by `ts_rank` and tie-break on id, so the first item is the best match and repeating a request returns the same page rather than a different slice of an equally-scored block. Ranking costs the index's early exit — every match is scored before `limit` applies. `/events/search` is the exception: it is newest-first (`eventTime` descending), not scored. - **`limit` defaults to 100, caps at 1000** (the *filter* endpoints default to 1000 and cap at 10000 — different numbers, easy to conflate), and `<= 0` falls back to the default rather than returning nothing. @@ -363,9 +369,9 @@ collection that had them. and `python_tests/test_plain_listings.py`. - **`resources.list` is typed like every other `/resources` read** — it spans all six node types and answers each row as its own [`Node`](#the-polymorphic-node-type) variant. -- **The listing is not a way to fetch everything.** `FunctionsService::by_ids` has no `/byids` - endpoint behind it and filters a listing client-side; that listing used to be uncapped, so it now - asks for 10000 and a tenant past that silently misses its oldest functions. +- **The listing is not a way to fetch everything.** `FunctionsService::by_ids` does not yet call + the api's `/functions/byids` and filters a listing client-side; that listing used to be uncapped, + so it now asks for 10000 and a tenant past that silently misses its oldest functions. Names that are gone rather than aliased, the way the filter refactors handled theirs: `POST /datasets/list` (a stale caller gets **405** — `GET /datasets/{id}` matches the path and @@ -403,14 +409,14 @@ Two transport details are easy to get wrong and cost a confusing failure each: - **`Accept` must be `application/json, text/event-stream`, byte for byte.** The transport compares it with `MediaType.equals`, so offering only `application/json` is a bare 400 with nothing pointing at the header. -- **The response must not be double-encoded.** This has regressed to the envelope being written as a - `String` which Spring then serializes *as JSON*, so the body is a quoted, escaped document and the - obvious `parse(body)["result"]` yields a string. `unwrap_envelope` **rejects** that rather than - parsing twice. Accommodating it would leave the suite green against a wire format no conformant MCP - client can read, so while it is present nearly every test here fails — which is the accurate - report, because no tool is reachable. `mcp_response_is_a_json_object` is what names the cause; the - handful that still pass are the ones that never parse an envelope (the auth rejections, the - malformed-body check). +- **The response must not be double-encoded.** The envelope was once written as a `String` which + Spring then serialized *as JSON*, so the body was a quoted, escaped document and the obvious + `parse(body)["result"]` yielded a string. `unwrap_envelope` **rejects** that rather than parsing + twice — accommodating it would have left the suite green against a wire format no conformant MCP + client can read, so while the fault was present nearly every test here failed, which was the + accurate report. `StrictJacksonJsonHttpMessageConverter` now declines to read or write + `CharSequence`/`byte[]`, closing both directions; `mcp_response_is_a_json_object` and + `mcp_accepts_application_json` are the guards. ### Cleanup @@ -483,27 +489,30 @@ Behaviours worth knowing, each pinned by an assertion: `unitExternalId: "pressure_bar"`. Take the id from `unit_list` in tests — one that is merely absent from the tenant fails as "Unknown unit externalId", which is a different path from supplying neither. -### Tests that are red on purpose - -One encodes intended behaviour the api does not yet provide, in the same spirit as -`test_duplicate_relationship_type_conflicts`: it stays red until the server-side fix lands rather -than being softened to match the bug. - -- `mcp_response_is_a_json_object` — whenever the double-encoding regression above is present, along - with every other test that parses an envelope. Both directions are the same underlying fault: the - transport moves the JSON-RPC payload as a `String` and lets content negotiation's JSON converter - handle it. Reading, the converter is asked to bind an object *into* a `String` and refuses — a - **500 on the spec-mandated `Content-Type: application/json`**, which locks out every off-the-shelf - client, guarded by `mcp_accepts_application_json`. Writing, it is handed a `String` to emit *as* - `application/json` and escapes it. Note the api's own MockMvc test (`McpEndpointTest`) asserts only - on the security gate, so neither direction was covered there. - -Two others stood here and are resolved, their tests staying on as regression guards: -`unitExternalId` as an alternative to `unit` on `timeseries_create`, and -`mcp_event_update_by_uuid_reindexes_the_external_id` — renaming an event by UUID wrote the new -`externalId` without reindexing it. The api settled the second by deleting `newExternalId` from -`event_update` rather than making the rename work, so that test is gone rather than green: there is -no rename left to assert on. Nothing in the SDK's MCP suite sends `newExternalId` to an event. +### Tests that were red on purpose + +**Nothing in this suite is red today.** The convention stands — an intended behaviour is worth more +as a failing test than softened to match the bug — and everything written that way has since landed +server-side. The tests stay on as regression guards: + +- `mcp_response_is_a_json_object` and `mcp_accepts_application_json` — the two directions of the + double-encoding fault above, one underlying cause: the transport moved the JSON-RPC payload as a + `String` and let content negotiation's JSON converter handle it. Reading, the converter was asked + to bind an object *into* a `String` and refused — a **500 on the spec-mandated + `Content-Type: application/json`**, which locked out every off-the-shelf client. Writing, it was + handed a `String` to emit *as* `application/json` and escaped it. The api's own MockMvc test + (`McpEndpointTest`, in `datahub-analysis`) asserts only on the security gate, so neither + direction was covered there; `StrictJacksonConverterBoundaryTest` and + `StrictJacksonJsonHttpMessageConverterTest` in `datahub-api` now cover both. +- `unitExternalId` as an alternative to `unit` on `timeseries_create`. +- `test_duplicate_relationship_type_conflicts` (in `relations::tests`, not this module) — a + duplicate relationship type is a 409 now. + +One was resolved by deletion rather than by a fix: `mcp_event_update_by_uuid_reindexes_the_external_id` +— renaming an event by UUID wrote the new `externalId` without reindexing it. The api settled it by +removing `newExternalId` from `event_update` rather than making the rename work, so that test is gone +rather than green: there is no rename left to assert on. Nothing in the SDK's MCP suite sends +`newExternalId` to an event. ## Python bindings (`datahub_python_bindings/`) diff --git a/docs/api-coverage.md b/docs/api-coverage.md deleted file mode 100644 index e5875dc..0000000 --- a/docs/api-coverage.md +++ /dev/null @@ -1,212 +0,0 @@ -# API coverage audit - -Endpoint-by-endpoint comparison of the DataHub REST API against this SDK. - -Baselines: -- Backend: `datahub-platform` **`origin/master`** @ `2fd92d2c`, all `@RestController` mappings under - `datahub-api/.../api/controllers/` plus the handlers registered in `api/websocket/WebSocketConfig.java`. -- SDK: this branch, merged up to `origin/main` @ `5632fbb`. - -`/stream/**` is deliberately out of scope and not listed. - -The main table reflects `origin/master` only. Unpushed backend work that will change it is in -[In-flight backend work](#in-flight-backend-work) at the end — read that before acting on this. - -## Summary - -| Controller | Endpoints | Covered | -|---|---|---| -| `/datasets` | 7 | 3 | -| `/edges` | 5 | 0 | -| `/events` | 16 | 8 | -| `/files` | 9 | 9 | -| `/functions` | 3 | 3 | -| `/governance` | 2 | 0 | -| `/labels` | 5 | 5 | -| `/policies` | 7 | 0 | -| `/resources` | 9 | 6 | -| `/stats` | 1 | 0 | -| `/subscriptions` | 3 | 3 | -| `/tenant` | 1 | 0 | -| `/timeseries` | 12 | 11 | -| `/units` | 3 | 3 | -| WebSocket | 2 | 1 | -| **Total** | **85** | **52** | - -## Broken, not merely missing - -These are worse than a gap — the SDK exposes a method that cannot work. - -| SDK | Problem | -|---|---| -| `datasets.filter()` | POSTs `/datasets/filter`. No such route. The backend's structured-list endpoint is `POST /datasets/list`. The body the SDK already sends (`DatasetFilterForm` → `{filter, cursor, limit}`) matches the server's `DataSetRetreiver` exactly — only the path is wrong. | -| `datasets.search()` | Also POSTs `/datasets/filter`, not `/datasets/search`, so search is unreachable even though the backend implements it. | -| `datasets.list()` | `todo!()` — panics. | -| `datasets.update()` | `todo!()` — panics, while `POST /datasets/update` exists. | -| `datasets.policies()` | `todo!()` — panics, while `GET /datasets/policies` exists. | - -`blocking.rs:242` already notes the dataset stubs, so the blocking mirror inherits the same holes. -These five are the only remaining stubs — the event stubs (`retrieve`/`search`/`update`) were -replaced with real calls in `add-event-endpoints` (#60). - -## Entirely uncovered controllers - -| Route | Purpose | -|---|---| -| `GET /edges/{id}` | Fetch one relationship edge | -| `POST /edges/byids` | Batch-fetch edges | -| `GET /edges/types` | List relationship types | -| `POST /edges/types/create` | Create relationship types | -| `POST\|DELETE /edges/delete` | Delete edges | -| `GET /policies` | List policies | -| `GET /policies/types` | List policy types | -| `POST /policies/create` | Create policy | -| `POST /policies/update` | Update policy | -| `POST\|DELETE /policies/delete` | Delete policies | -| `GET /policies/{policyNodeId}` | Get policy by id | -| `POST /policies/naming/check` | Validate a name against naming policy | -| `GET /governance/templates` | List governance templates | -| `GET /governance/templates/{templateId}` | Get template by id | -| `GET /stats` | Instance stats (optional `keys` param) | -| `GET /tenant/features` | Tenant feature flags | - -`src/relations/` holds the edge *DTOs* (`EdgeProxy`, `RelForm`) used when creating resources and -reading `fetch-related` results, but there is no service that talks to `/edges` directly — edges can -only be created as a side effect of `resources.create()`/`update()`, never listed, fetched, or -deleted on their own. Relationship *types* cannot be created or enumerated at all. - -## Partial gaps - -### `/events` — 8 of 16 - -Covered: `byids`, `filter`, `search`, `create`, `update`, `delete`, `GET /{id}`, `count`. - -Missing: the eight metadata-enumeration routes — `GET /list/{types,sub-types,statuses,sources}` -(distinct values) and `GET /search/{type,sub-type,status,source}` (typeahead over the same -vocabularies). All take a `limit` param, default 1000, capped at 10000. - -### `/resources` — 6 of 9 - -Covered: `byids`, `search`, `fetch-related`, `create`, `update`, `delete`. - -Missing: `GET /resources/{id}` (single fetch by numeric id), `POST /resources/filter` (structured -filtering), `POST /resources/fetch-nearest`. Note that events' `advancedFilter` is now a plain -expression string and carries no reusable type, so a resource equivalent would start from the -api's filter language rather than from anything in `src/filters.rs`. - -### `/timeseries` — 11 of 12 - -Only `GET /timeseries/recommend-value-type/{unitExternalId}` is missing (returns a -`ValueTypeRecommendation` for a unit). - -`POST /timeseries/filter` **is** covered and correct: `TimeSeriesFilterForm` / `TimeSeriesFilter` -serialize to exactly the server's `TimeseriesRetreiver` / `TimeseriesFilter` -(`{limit, filter:{dataSetId, unit, unitExternalId, metadataKey, metadataValue}}`). - -### WebSocket - -| Route | Status | -|---|---| -| `/timeseries/datapoints/subscription/listen/**` | ✅ `subscriptions.listen()` | -| `/timeseries/datapoints/listen` | ❌ live, cursorless datapoint stream — no SDK support | - -The second handler is a separate, non-durable live feed (`DatapointListenWebSocketHandler`); it is -`permitAll` at the security-config level and validates its token internally. - -## Fully covered - -`/files` (9/9), `/units` (3/3), `/labels` (5/5), `/functions` (3/3), `/subscriptions` (3/3). - -`/files` was completed in this branch — `get_by_id`, `get_by_external_id`, `search`, `list_trash`, -`restore`, `update`, `download` and `download_to_path` were added alongside the existing upload, -list and delete, and mirrored into the blocking client and the Python bindings. Three things worth -knowing, found while testing against a live backend: - -- **`delete` returned the wrong type.** `FileService::delete` was declared - `Result, _>` — a copy-paste from the events service. It is now - `DataWrapper`. The endpoint answers 204 with no body, so nothing deserialized either way, - but the signature was a lie. This is a breaking change for anyone who named the type. -- **Downloads need an `Accept` override.** The shared `http_client` sets a default - `Accept: application/json`, and `/files/download/{id}` only `produces` - `application/octet-stream` — so Spring returned 406 before the handler ran. The new - `execute_get_stream_request` helper sends `Accept: */*`. -- **A file's `path` comes back doubled.** `FileTransformer.transform` in the backend sets a FILE's - DTO path to `node.getPath() + "/" + node.getName()`, but a file's stored path already ends in its - name — so `/lifecycle/sola.jpg` is returned as `/lifecycle/sola.jpg/sola.jpg`. This affects every - file endpoint that returns an `IndexNode`, not just the new ones, and is a **backend** bug. The - tests assert the folder prefix rather than the full path so they survive the fix. -- **Restore only works by numeric id.** `POST /files/restore` hashes a supplied external id through - `ExternalIds.hash`, which lowercases, while a trashed id is `DELETED___` with - the stored hash not lowercased — so the lookup misses and the call 404s. Also a **backend** bug; - restoring by id sidesteps the hash entirely and works. - -## Suggested priority - -1. **Fix `datasets.filter()` / `datasets.search()`** — they point at a route that doesn't exist, so - dataset filtering and search are silently unusable today. The bodies are already right; this is a - two-line path change plus tests. -2. **Replace the three remaining `todo!()` stubs** (`datasets.list/update/policies`) with real calls - or remove them; a panicking public method is worse than an absent one. -3. ~~`GET /files/download/{id}`~~ — **done**, along with the rest of `/files`. -4. **`/edges` service** — needed for any relationship management that isn't a resource-create side - effect, and for discovering relationship types. -5. **`/resources/filter`** — needs both the call and whatever filter shape the api settles on for - resources; the events filter language is a string, with no Rust-side types to reuse. -6. **`/policies` and `/governance`** — newer surfaces; wire up when the platform needs them. -7. Lower value: events vocabulary enumeration, `/stats`, `/tenant/features`, - `recommend-value-type`, the live datapoint WebSocket. - -## In-flight backend work - -Three branches in the primary backend checkout are ahead of `origin/master` and unpushed. Two of -them change the contract this SDK depends on. - -### `fix/files-upload-indexnode-and-eventtime-schema` (+1) - -No endpoint added or removed, but it changes what `PUT /files` **returns**: today the handler leaks -the raw JPA entity `INode` instead of the `IndexNode` DTO every other file endpoint uses. - -This already affects us. `files.upload_file()` deserializes into `crate::generic::INode`, which is -shaped like the server's `IndexNode` (`type`, `parentId`, string id). Against current master the -response instead carries nested `nodeType`/`parent` objects, a **base64** checksum, and a numeric -id — so today upload silently yields `type: None` and `parent_id: None`, and a `checksum` in the -wrong encoding. It does not error: every affected field is `Option`, and `opt_string_id` accepts a -JSON number as well as a string. The pending fix aligns the response with what the SDK already -expects, so no SDK change is needed — but any test asserting on those fields will flip behaviour -when it lands. - -The same branch corrects `EventModel.eventTime`'s OpenAPI schema from a (never-emitted) numeric -epoch to `string`/`date-time`. No wire change. - -### `fix/events-related-resources` (+2) - -Collapses `EventModel`'s parallel `relatedResourceIds: List` and -`relatedResourceExternalIds: List` into a single `relatedResources: List` -(`[{"id": 34, "externalId": "sensor_abc"}]`), with the API resolving whichever side is omitted and -always returning both. `EventFilterForm.relatedResources` already had that shape; this makes the entity -match, and tightens the filter's documented semantics to "related to **all** of these". - -The Rust SDK's `Event` still carries the two parallel fields (`get_related_resource_ids()` / -`get_related_resource_external_ids()`). The matching SDK-side work is already underway on the -`feature/event-related-resources-single-field` branch in the sibling worktree. - -### `bugfix/datasets-in-event-filter-broken` (+3) - -The only branch that changes the endpoint set. It adds a `PolicyFindingController` (also mapped -under `/policies`) with two new endpoints and relocates a third: - -| Route | Change | -|---|---| -| `GET /policies/findings` | **new** — list policy findings | -| `POST /policies/findings/{findingId}/resolve` | **new** — resolve a finding | -| `POST /policies/naming/check` | moved from `PolicyController` to `PolicyFindingController` | - -When this lands `/policies` becomes **9 endpoints, still 0 covered**. The branch also deletes the -console's `ChatApiController` and trims `ResourceController`/`TimeseriesController`, but without -changing any mapping path. - -### Reference - -The backend carries an uncommitted `datahub-api-model/WIRE_FORMAT_AUDIT.md` cataloguing wire-contract -inconsistencies (the two fixes above are its B1 and B2), plus structural issues left unfixed. Worth -reading before making assumptions about envelope or field-level shapes. diff --git a/src/assets/mod.rs b/src/assets/mod.rs index 3665caa..185d2c8 100644 --- a/src/assets/mod.rs +++ b/src/assets/mod.rs @@ -26,8 +26,9 @@ use std::sync::Weak; /// leaving a caller's `["timeseries"]` in place would widen a request to `/assets` into a mixed /// query. /// -/// `/assets` is the newest of the typed families. What it does *not* share with `/functions` is -/// worth knowing: functions have no `/byids`, `/filter` or `/search` on the api at all. +/// What it does *not* share with `/functions` is worth knowing: the api serves the same three +/// reads there, but the SDK's [`FunctionsService`](crate::functions::FunctionsService) has not +/// wired them yet and still filters a listing client-side. pub struct AssetsService { pub(crate) api_service: Weak, base_url: String, diff --git a/src/datasets/mod.rs b/src/datasets/mod.rs index 800e829..d844314 100644 --- a/src/datasets/mod.rs +++ b/src/datasets/mod.rs @@ -73,8 +73,9 @@ impl DatasetsService { /// `POST /datasets/filter` — datasets matching [`DatasetFilterForm`], newest first. /// /// Every criterion on the filter is honoured server-side. Results are capped by the form's - /// `limit` (default 100, max 10000) and there is no paging, so a filter broad enough to exceed - /// the cap is silently truncated — narrow it rather than trying to page. + /// `limit` (default 100 on the form, 1000 server-side when unset; max 10000). A broader match + /// than the cap is paged, not truncated: set [`sort`](DatasetFilterForm) and echo the + /// envelope's `next_cursor` back as `cursor` to walk it, under that same `sort`. pub async fn filter( &self, filter: &DatasetFilterForm, @@ -102,8 +103,7 @@ impl DatasetsService { /// scored before `limit` applies. /// /// The whole form reaches the server: `filter` narrows the phrase's hits and can never widen - /// them. No match is an empty item list, not an error — the 404 the OpenAPI annotation still - /// advertises was removed server-side. + /// them. No match is an empty item list, not an error. /// /// `query` is validated at 3–140 characters and nothing else. It used to be held to /// `^[\p{IsLatin}\p{Zs}\p{Nd}]+` as well, which rejected every snake_case external id and diff --git a/src/events/mod.rs b/src/events/mod.rs index f17def2..c347e29 100644 --- a/src/events/mod.rs +++ b/src/events/mod.rs @@ -205,8 +205,11 @@ impl EventsService { .await } - /// Look up a single event by its UUID `id` (`GET /events/{id}`). The result is an empty - /// `items` if no event with that id exists (or it belongs to a tenant you can't read). + /// Look up a single event by its UUID `id` (`GET /events/{id}`). + /// + /// A miss is a **404** carrying a `not-found` problem, not an empty `items`. An event outside + /// the caller's readable datasets answers the same way, so a 404 says "not an event you can + /// read" and never leaks that the id exists. pub async fn get(&self, id: &Uuid) -> Result, ResponseError> { let path = &format!("{}/{}", self.base_url, id); self.execute_get_request(path, None::<&str>).await @@ -227,9 +230,14 @@ impl EventsService { .await } - /// Free-text search over event descriptions (`POST /events/search`). Matching is fuzzy and - /// word-aware; results are ranked by relevance. For structured filters (time ranges, types, - /// related resources) use [`filter`](Self::filter) instead — it is faster and more predictable. + /// Free-text search over events (`POST /events/search`). + /// + /// A plain case-insensitive **substring** match — not fuzzy, not word-aware — across + /// `externalId`, `description` and the metadata *values*. Events live in ClickHouse rather + /// than the node table and have no full-text index, so results are **not ranked**: they come + /// back newest first by `eventTime`, unlike the three node searches. For structured filters + /// (time ranges, types, related resources) use [`filter`](Self::filter) instead — it is faster + /// and more predictable. pub async fn search( &self, search: &SearchAndFilterForm, diff --git a/src/filters.rs b/src/filters.rs index 838c649..9c57963 100644 --- a/src/filters.rs +++ b/src/filters.rs @@ -78,7 +78,8 @@ pub struct NodeFilter { /// [`external_id`](Self::external_id), [`source`](Self::source), [`metadata`](Self::metadata), /// [`created_time`](Self::created_time), [`last_updated_time`](Self::last_updated_time) and /// [`data_set_id`](Self::data_set_id) carry the same names and semantics they have there — -/// including the `*` / `%` wildcards and the case-insensitive, OR-within-a-list matching. +/// including the `*` / `%` wildcards and the OR-within-a-list matching. The one divergence is +/// that a literal [`external_id`](Self::external_id) entry matches case-sensitively here. /// /// Note the filter has no `id` field. Events are keyed by a UUID; use /// [`EventsService::by_ids`](crate::events::EventsService::by_ids) to look one up. @@ -87,8 +88,12 @@ pub struct NodeFilter { #[derive(Debug, Serialize, Deserialize, Clone, Default)] #[serde(rename_all = "camelCase")] pub struct EventFilter { - /// Events matching any of these external ids — literal or wildcard, exactly as - /// [`NodeFilter::external_id`]. Replaced the old single-valued `externalIdPrefix`: + /// Events matching any of these external ids, literal or wildcard. + /// + /// **Not quite [`NodeFilter::external_id`]:** a literal entry here matches + /// **case-sensitively**. Event writers hash the external id verbatim, while the node filters + /// lowercase before hashing and so match either case. A wildcard entry goes through `ILIKE` + /// instead and is case-insensitive on both. Replaced the old single-valued `externalIdPrefix`: /// `"work_order_*"` says the same thing and composes with exact ids in one list. #[serde(default, skip_serializing_if = "Option::is_none")] pub external_id: Option>, @@ -415,10 +420,10 @@ impl EventFilterForm { /// one. An unreadable cursor is refused with a 400. /// /// Send it with the **same** [`sort`](Self::set_sort) that produced it: a cursor is a position - /// in one particular order, and continuing it under another is a **400** rather than a page - /// that is quietly short. Sorting by `subType` or `status` cannot be paged at all — those - /// columns may be null, and a keyset boundary on them would skip the events that have no - /// value — so a cursor sent with either is refused. + /// in one particular order, and continuing it under another is a **400** naming both sorts, + /// rather than a page that is quietly short. The nullable event sorts `subType` and `status` + /// page like any other — the keyset predicate handles the null block explicitly, so a null is + /// a position rather than a missing one. pub fn set_cursor(&mut self, cursor: impl Into) -> &mut Self { self.cursor = Some(cursor.into()); self @@ -431,12 +436,14 @@ impl EventFilterForm { pub fn sort(&self) -> Option<&DataSort> { self.sort.as_ref() } - /// Order the result page. Ignored when a [cursor](Self::set_cursor) is set. + /// Order the result page. A [cursor](Self::set_cursor) must be sent with the sort that + /// produced it, so this is honoured rather than overridden — a mismatch is a **400**. pub fn set_sort(&mut self, sort: DataSort) -> &mut Self { self.sort = Some(sort); self } - /// Drop the ordering, letting the backend return the page in no particular order. + /// Drop the ordering, falling back to the server default of `eventTime` ascending with an + /// `id` tie-break. Never "no order": the query always emits an `ORDER BY`. pub fn clear_sort(&mut self) -> &mut Self { self.sort = None; self @@ -736,9 +743,9 @@ mod tests { /// The sort and cursor half of the request bodies, which the four filter endpoints share. /// -/// Kept separate from the criteria tests above because these are the fields a *wrong* shape breaks -/// silently: an unknown sort property falls back to the default rather than erroring, and an -/// unreadable cursor restarts the walk from page one. Neither raises anything a caller would see. +/// Kept separate from the criteria tests above because the two fields fail in opposite ways: an +/// unknown sort property falls back to the default rather than erroring, so it is silent, while an +/// unreadable cursor — or one continued under a different sort — is a **400** naming the field. #[cfg(test)] mod paging_serde { use super::*; diff --git a/src/functions/mod.rs b/src/functions/mod.rs index 55efa81..fb51a85 100644 --- a/src/functions/mod.rs +++ b/src/functions/mod.rs @@ -67,8 +67,8 @@ impl FunctionsService { /// more. A node of another type is not a function and is reported the same way. /// /// Unlike [`by_ids`](Self::by_ids), which omits what it cannot find, this is an error. Prefer - /// it to `by_ids` when you already have the numeric id: `by_ids` has no endpoint behind it and - /// pages the whole listing to filter client-side. + /// it to `by_ids` when you already have the numeric id: `by_ids` does not yet call the api's + /// `/functions/byids` and pages the whole listing to filter client-side. pub async fn get_by_id(&self, id: u64) -> Result, ResponseError> { let path = &format!("{}/{}", self.base_url, id); self.execute_get_request::, ()>(path, None) @@ -105,14 +105,15 @@ impl FunctionsService { .await } - /// Look up functions by id or externalId. The backend has no `/byids` endpoint for - /// functions — nor `/filter` or `/search`, which every other node type has — so this is - /// implemented client-side by listing and filtering. + /// Look up functions by id or externalId, implemented client-side by listing and filtering. + /// + /// **The SDK has not wired the real endpoint yet.** The api grew `/functions/byids`, + /// `/functions/filter` and `/functions/search` in platform #131, which is what this should + /// call; until it does, the client-side walk stands. /// /// It asks for the largest page the api allows, because a client-side filter can only match - /// what the listing returned. That listing used to be uncapped; a tenant past 10000 functions - /// now silently misses the oldest ones here, and needs a real `/byids` endpoint rather than - /// a bigger number. + /// what the listing returned — so a tenant past 10000 functions silently misses the oldest + /// ones here. The fix is to call `/functions/byids`, not to ask for a bigger page. pub async fn by_ids( &self, ids: &[IdAndExtId], diff --git a/src/http.rs b/src/http.rs index 6ee3eeb..0c1f625 100644 --- a/src/http.rs +++ b/src/http.rs @@ -89,8 +89,9 @@ impl ResponseError { /// The RFC 9457 problem document the server explained itself with, or `None` when it did not. /// - /// `None` covers every body that is not a problem — an empty 401, a plain-text refusal, Spring - /// Boot's whitelabel error JSON — and every error raised before a response existed. So this + /// `None` covers every body that is not a problem — an empty body, a plain-text refusal, + /// Spring Boot's whitelabel error JSON — and every error raised before a response existed + /// (a connect failure, a client-side refusal, an unobtainable token). So this /// answers "did the api explain this failure in the documented way", which is the question a /// caller wants; [`get_message`](Self::get_message) keeps the raw body either way. /// diff --git a/src/mcp_integration.rs b/src/mcp_integration.rs index 411c1ef..632a8ec 100644 --- a/src/mcp_integration.rs +++ b/src/mcp_integration.rs @@ -23,12 +23,12 @@ //! - **`Accept` must be `application/json, text/event-stream`, byte for byte.** The transport //! compares it with `MediaType.equals`, so offering only `application/json` is a bare 400 with //! nothing pointing at the header. -//! - **The response must not be double-encoded.** The envelope has regressed to being written as a -//! `String` which Spring then serializes *as JSON*, so the body is a quoted, escaped document and -//! the obvious `parse(body)["result"]` yields a string. [`unwrap_envelope`] rejects that rather -//! than parsing twice: accommodating it would leave the suite green against a wire format no -//! conformant client can read. While it is present, every test here fails — which is the honest -//! report, because no tool is reachable. +//! - **The response must not be double-encoded.** The envelope was once written as a `String` +//! which Spring then serialized *as JSON*, so the body was a quoted, escaped document and the +//! obvious `parse(body)["result"]` yielded a string. [`unwrap_envelope`] rejects that rather +//! than parsing twice: accommodating it would have left the suite green against a wire format +//! no conformant client can read. `StrictJacksonJsonHttpMessageConverter` now declines to +//! read or write `CharSequence`/`byte[]` at all, which closes both directions. //! //! # Every tool is driven once //! @@ -43,19 +43,23 @@ //! reference data, and a relationship type cannot be deleted once created (see AGENTS.md) — minting //! a set per test would grow the tenant's catalogue on every run. //! -//! # Tests that are red on purpose +//! # Tests that were red on purpose //! -//! One encodes intended behaviour the api does not yet provide, in the same spirit as -//! `test_duplicate_relationship_type_conflicts`: it stays red until the server-side fix lands -//! rather than being softened to match the bug. +//! Nothing here is red today. Four assertions were written against intended behaviour the api did +//! not yet provide — red rather than softened to match the bug — and all four have landed. They +//! stay on as regression guards: //! -//! - [`mcp_response_is_a_json_object`] — whenever the double-encoding regression is present. It names -//! the fault directly; [`unwrap_envelope`] independently fails every other test for the same cause. -//! -//! Two others were red here and have been resolved server-side, the tests staying on as regression -//! guards: `unitExternalId` as an alternative to `unit` on `timeseries_create`, and renaming an -//! event by UUID — which the api settled by removing `newExternalId` from `event_update` outright, -//! an event's externalId being its identity rather than a property (see [`EventUpdateFields`]). +//! - [`mcp_response_is_a_json_object`] and [`mcp_accepts_application_json`] — the two directions of +//! the double-encoding fault above. Reading, the converter was asked to bind an object *into* a +//! `String` and refused, so `POST /mcp` answered **500** on the spec-mandated +//! `Content-Type: application/json` and locked out every off-the-shelf client. Writing, it +//! escaped the envelope. Note the api's own `McpEndpointTest` asserts only on the security gate, +//! so neither direction was covered there; `StrictJacksonConverterBoundaryTest` and +//! `StrictJacksonJsonHttpMessageConverterTest` now cover them server-side. +//! - `unitExternalId` as an alternative to `unit` on `timeseries_create`. +//! - Renaming an event by UUID — which the api settled by removing `newExternalId` from +//! `event_update` outright, an event's externalId being its identity rather than a property (see +//! [`EventUpdateFields`]). That test is gone rather than green: there is no rename to assert on. //! //! [`EventUpdateFields`]: crate::events::EventUpdateFields diff --git a/src/multi_tenant_integration.rs b/src/multi_tenant_integration.rs index 4b0cb8a..69baa2b 100644 --- a/src/multi_tenant_integration.rs +++ b/src/multi_tenant_integration.rs @@ -59,16 +59,18 @@ //! let beta = ApiService::new(config); // a second client, a second tenant //! ``` //! -//! **The server's reason never reaches the client.** The API installs a custom authentication -//! entry point that emits a bare `WWW-Authenticate: Bearer realm="Restricted Content"` with no -//! `error_description`, and a body that is Spring's generic error JSON; the descriptive message -//! goes only to the server log. So nothing these tests assert about a 401 comes from the response. +//! **The server explains its 401s.** The authentication entry point answers +//! `application/problem+json` with `type: .../errors/unauthorized` and a `detail` naming the +//! branch the organization validator hit — a missing, empty, malformed or ambiguous claim — so +//! the multi-organization test can assert on a message naming both organizations, and it is the +//! server's own words it reads. `WWW-Authenticate` still carries no `error_description`; the +//! reason is in the body. //! -//! What the SDK does instead is reconstruct the reason from the token it just sent — see -//! [`crate::auth_diagnostics`], which decodes the `organization` claim and reports which of the -//! validator's branches it would have hit. That is why the multi-organization test can assert on a -//! message naming both organizations even though the wire carries none. Everything else is -//! asserted on **status alone**, with the cases told apart by how the fixture is built. +//! [`crate::auth_diagnostics`] reconstructs the same reason from the token the SDK just sent and +//! appends it, so the message a caller sees can carry both. Every 401 here shares the +//! `unauthorized` slug, so the cause is only in the prose: a caller cannot branch on `type` to +//! tell "no token" from "ambiguous organization". Cases that differ only by fixture are asserted +//! on **status alone**. //! //! (403s are different: those carry a real RFC 9457 `problem+json` body from the server, with //! `dataSetId` and `permission`, which the ACL tests assert on directly.) @@ -461,10 +463,10 @@ async fn multi_tenant_multi_org_principal_with_wildcard_scope_is_rejected( "a token naming two organizations", ); - // The server sends no reason at all, so this message is one the SDK reconstructs from the - // token it just used (`crate::auth_diagnostics`). Asserting it here — against a real - // two-organization token from a real realm — is what proves the diagnosis fires on the path - // that actually matters, rather than only on the hand-built tokens in its unit tests. + // The reason reaches the caller two ways now: the server writes it into the problem's + // `detail`, and `crate::auth_diagnostics` appends its own from the token just used. Asserting + // it against a real two-organization token from a real realm is what proves the explanation + // survives the whole path, entry point to `ResponseError`. let message = error.get_message(); assert!( message.contains("names 2 organizations"), @@ -1004,16 +1006,16 @@ async fn acl_dataset_management_requires_a_blanket_write_grant() -> Result<(), R /// The child is linked with `connectedDataSets`, which the backend reads as "the dataset this one /// is part of" and stores as the `BELONGS_TO` edge the ACL closure walks. /// -/// **Currently unreachable through this SDK**, and the test skips rather than failing. Two gaps -/// meet here: +/// **The test still skips**, but only one gap is left: `POST /datasets/create` with a non-empty +/// `connectedDataSets` answers **200 with an empty body and creates nothing at all** — no error, +/// no entity. With the field empty it returns the created dataset as normal, so the parent link +/// cannot be set at create time. /// -/// - `POST /datasets/create` with a non-empty `connectedDataSets` answers **200 with an empty -/// body and creates nothing at all** — no error, no entity. With the field empty it returns the -/// created dataset as normal. So the parent link cannot be set at create time. -/// - [`DatasetsService::update`](crate::datasets::DatasetsService::update) is `todo!()`, so it -/// cannot be set afterwards either. -/// -/// The skip resolves itself the moment either gap closes. +/// `DatasetsService::update` is implemented now (it was `todo!()` when this was written), and +/// [`EdgesService`](crate::relations::EdgesService) can create the edge directly. Note the +/// direction if you wire this up: the row is stored `from = parent, to = child` even though the +/// relationship is named `BELONGS_TO`, and the closure query descends `rel_start -> rel_end` — +/// `connected_data_sets` does not create it. See `python_tests/filter_fixtures.py`. #[tokio::test] #[ignore] async fn acl_a_parent_dataset_grant_covers_descendants() -> Result<(), ResponseError> { diff --git a/src/nodes.rs b/src/nodes.rs index f8f9eea..3281d9c 100644 --- a/src/nodes.rs +++ b/src/nodes.rs @@ -19,11 +19,13 @@ //! - **Flat reads** (`get_by_id`, `by_ids`, `filter`, `search`) are fully populated *except* //! `related_resources`, which is always empty — the api does not join the edges in. //! - **Graph reads** ([`ResourceService::fetch_related`](crate::resources::ResourceService::fetch_related), -//! `fetch_nearest`) are typed, and now very nearly 1-1 with a flat read: a [`TimeSeries`] -//! from there carries its `unit`, `unit_external_id` and `value_type`. The one field still -//! lost is **`metadata`**, which the api writes to Neo4j as flattened `metadata_*` properties -//! and never reassembles on the way out, so it is silently empty rather than absent. -//! `related_resources` runs the other way — populated here, always `[]` on a flat read. +//! `fetch_nearest`) are typed and 1-1 with a flat read: a [`TimeSeries`] from there carries its +//! `unit`, `unit_external_id` and `value_type`, and **`metadata`** too — the api writes it to +//! Neo4j as flattened `metadata_*` properties and now strips the prefix back off on the way +//! out. Assert this by diffing against a flat read rather than by naming fields; the projection +//! has filled in gradually. `related_resources` runs the other way — populated here, always +//! `[]` on a flat read. An asset's geometry is reconstructed as a Point, so a stored Polygon +//! comes back wrong. //! Type-specific fields stay `Option` regardless: a node written before a given field was //! projected reports it absent, and absent must not read as a default. //! - **Policies** never carry `value`, `template_id` or `data_set_id` on any read — the api's @@ -185,7 +187,8 @@ pub fn to_snake_upper_cased(s: &str) -> String { /// /// Field-identical to [`Resource`] plus a meaningful `geolocation`: the api models both on the /// same node base and distinguishes them only by the `ASSET` type-label. `geolocation` is the one -/// practical difference, since a plain resource accepts it on write but never echoes it back. +/// practical difference — only an asset declares it, so a plain resource create carrying one is a +/// 400 naming it as an unknown field. #[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] #[serde(rename_all = "camelCase")] pub struct Asset { @@ -418,8 +421,7 @@ impl Node { on_node!(self, n => n.source.as_deref()) } - /// The node's metadata map. Empty rather than absent on a node read through the graph — that - /// path does not carry the column at all. + /// The node's metadata map, carried on both the flat and the graph read paths. pub fn metadata(&self) -> Option<&HashMap> { match self { Node::Asset(n) => n.metadata.as_ref(), diff --git a/src/problem.rs b/src/problem.rs index c44f5ac..d57ad1d 100644 --- a/src/problem.rs +++ b/src/problem.rs @@ -1,11 +1,11 @@ //! RFC 9457 `application/problem+json` — the shape the api describes a failure in. //! -//! The api is converging every refusal on this one document (its `Problems` helper is the single -//! place that builds them). Until that lands a caller meets three kinds of body, so -//! [`ProblemDetail`] is deliberately lenient about what it is handed: +//! The api has converged every refusal on this one document — its `Problems` helper is the single +//! place that builds them, so every refusal now carries a `type`. [`ProblemDetail`] stays +//! deliberately lenient about what it is handed, because a caller can still meet: //! //! - a full problem — `type`, `title`, `status`, `detail`, `instance`, plus extension members; -//! - a bare Spring `ProblemDetail` with **no `type`** (today's `GET /resources/{id}` 404); +//! - a bare `ProblemDetail` with **no `type`**, from an older api or a service in front of it; //! - not a problem at all — Spring Boot's whitelabel `{"timestamp","status","error","trace"}`, a //! success-shaped `{"items":[…]}` envelope, the legacy `{"error":{…}}` wrapper, plain text, or //! nothing. `src/problem_integration.rs` has the full catalogue of what answers what. @@ -36,7 +36,8 @@ use serde_json::{Map, Value}; /// caller cannot otherwise tell a problem the api typed from one it did not. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct ProblemDetail { - /// The stable identifier for *what kind* of failure this is. Absent on the api's bare 404s. + /// The stable identifier for *what kind* of failure this is. Every refusal the api mints + /// carries one; `None` means the body was not typed, not that the failure had no kind. #[serde(rename = "type", default, skip_serializing_if = "Option::is_none")] pub type_uri: Option, diff --git a/src/problem_integration.rs b/src/problem_integration.rs index 7b88650..35a5b81 100644 --- a/src/problem_integration.rs +++ b/src/problem_integration.rs @@ -19,7 +19,7 @@ //! | Shape, before | Example | Now | //! |---|---|---| //! | full problem+json with a `type` | unknown field, malformed JSON, bad cursor, bad expression | unchanged | -//! | problem+json with **no** `type` | `GET /resources/{id}` on a missing id | **still typeless** — see [`pending`] | +//! | problem+json with **no** `type` | `GET /resources/{id}` on a missing id | `not-found`, like every other refusal | //! | Spring Boot whitelabel + **stack trace** | `POST /datasets/list`, a `@Valid` search failure | typed problem, no trace | //! | a **success-shaped** `{"items":[…]}` envelope | `POST /timeseries/create` validation | `validation-failed` with `fields` | //! | the legacy `{"error":{"code","message",…}}` wrapper | a duplicate `externalId` | `duplicate` with `duplicated` | @@ -29,15 +29,14 @@ //! [`ProblemDetail::parse`] discriminates on structure rather than on the header. That is still the //! right call: it is what lets these tests state "this is *not* a problem document" as a fact. //! -//! The three groups are kept apart because which assertions were aspirational is worth remembering: +//! The two groups are kept apart because which assertions were aspirational is worth remembering: //! //! - [`green`] — true before the unification and after it. A failure here means the refactor //! changed something it did not mean to. //! - [`unified`] — arrived *with* the unification. Every one was red on purpose until the api -//! merged; they are regression guards now, and a failure means a revert. -//! - [`pending`] — the one refusal still without a `type`. Red on purpose, per the convention -//! [`crate::mcp_integration`] uses: an intended behaviour is worth more as a failing test than -//! softened to match the bug. +//! merged; they are regression guards now, and a failure means a revert. The by-id 404 was the +//! last to land — it sat in its own `pending` group, red on purpose rather than softened to +//! match the bug, and moved here when `ObjectNotFoundExceptionHandler` started typing it. //! //! # Why the assertions avoid prose //! @@ -408,6 +407,43 @@ mod unified { use super::{Probe, Refusal}; use crate::problem::{ProblemDetail, Retry}; + /// Every `GET //{id}` miss answers through one shared advice, and it was the last + /// refusal in the api with no `type`. + /// + /// It was easy to miss, because the body already looked finished: `Problems.decorate` ran on + /// it, so it carried `requestId` and `retry` like every other problem, and only `type` was + /// absent — the one member clients branch on. `ObjectNotFoundExceptionHandler` now calls + /// `Problems.notFound(detail)` rather than hand-building the document, so a caller matching + /// on `"not-found"` no longer falls through to its default arm on one of the commonest + /// errors there is. + #[tokio::test] + async fn a_missing_id_is_typed_like_every_other_refusal() { + let refusal = Probe::new().get("/resources/999999999").await; + + assert_eq!(refusal.status, 404); + let problem = refusal + .problem() + .unwrap_or_else(|| panic!("a 404 should be a problem, got {}", refusal.slug_or_shape())); + + // Pinned so that if the shared advice is migrated, this test notices rather than the + // decoration silently masking it: these already arrive, only `type` does not. + assert!( + problem.request_id().is_some() && problem.retry().is_some(), + "the advice already decorates this problem: {}", + refusal.preview() + ); + + assert_eq!( + problem.slug(), + Some("not-found"), + "a by-id miss should carry a `type` under {} like every other refusal — got {:?}. \ + ObjectNotFoundExceptionHandler builds its document by hand instead of calling \ + Problems.notFound().", + ProblemDetail::TYPE_BASE, + problem.type_uri + ); + } + /// Asserts the one invariant the whole refactor exists to establish. fn assert_is_a_typed_problem(refusal: &Refusal, what: &str) -> ProblemDetail { assert!( @@ -513,9 +549,10 @@ mod unified { ); } - /// An unauthenticated call answers 401 with `Content-Length: 0` today. That is why - /// [`crate::auth_diagnostics`] exists at all — the SDK reconstructs a reason from the token it - /// just sent, because the api sends none. A body here would make that guesswork unnecessary. + /// An unauthenticated call answers 401 with a full problem document — `type`, `detail`, + /// `requestId` and `retry` — rather than the empty body it used to send. That empty body is + /// why [`crate::auth_diagnostics`] exists, reconstructing the reason from the token just + /// sent; the server now says it directly. #[tokio::test] async fn an_unauthenticated_call_says_why() { let refusal = Probe::new().get_unauthenticated("/timeseries?limit=1").await; @@ -666,53 +703,3 @@ mod unified { ); } } - -/// What the unification has not reached yet. -/// -/// **Red on purpose**, like [`crate::mcp_integration`]'s `mcp_response_is_a_json_object`. When the -/// api closes this, the test goes green and moves to [`unified`]. -mod pending { - use super::Probe; - use crate::problem::ProblemDetail; - - /// Every `GET //{id}` miss answers through one shared advice, and it is the last - /// refusal in the api with no `type`. - /// - /// Easy to miss, because the body looks finished: `Problems.decorate` still runs on it, so it - /// carries `requestId` and `retry` like every other problem, and only `type` is absent. But - /// `type` is the member clients branch on — `slug()` is `None` here, so a caller matching on - /// `"not-found"` silently falls through to its default arm on one of the most common errors - /// there is. - /// - /// The fix is one line: `ObjectNotFoundExceptionHandler` hand-builds its document with - /// `ProblemDetail.forStatusAndDetail` + `setTitle`, where `Problems.notFound(detail)` — which - /// exists, sets `NOT_FOUND = type("not-found")`, and is already used by `FileController` and - /// `TimeseriesController` — would do it. - #[tokio::test] - async fn a_missing_id_is_typed_like_every_other_refusal() { - let refusal = Probe::new().get("/resources/999999999").await; - - assert_eq!(refusal.status, 404); - let problem = refusal - .problem() - .unwrap_or_else(|| panic!("a 404 should be a problem, got {}", refusal.slug_or_shape())); - - // Pinned so that if the shared advice is migrated, this test notices rather than the - // decoration silently masking it: these already arrive, only `type` does not. - assert!( - problem.request_id().is_some() && problem.retry().is_some(), - "the advice already decorates this problem: {}", - refusal.preview() - ); - - assert_eq!( - problem.slug(), - Some("not-found"), - "a by-id miss should carry a `type` under {} like every other refusal — got {:?}. \ - ObjectNotFoundExceptionHandler builds its document by hand instead of calling \ - Problems.notFound().", - ProblemDetail::TYPE_BASE, - problem.type_uri - ); - } -} diff --git a/src/relations/mod.rs b/src/relations/mod.rs index 993f7cf..1b5aeb8 100644 --- a/src/relations/mod.rs +++ b/src/relations/mod.rs @@ -40,8 +40,9 @@ pub struct EdgeProxy { /// Request-side edge form, mirroring server-side `RelForm`. Used when creating /// resources with relations. The server snake-uppercases `relationship_type` -/// (e.g. `"flows_to"` -> `"FLOWS_TO"`) and snake-lowercases external ids before -/// persisting, so callers can pass either case. +/// (e.g. `"flows_to"` -> `"FLOWS_TO"`). External ids are stored **verbatim** — what you send is +/// what reads back, byte for byte — but they are hashed lowercased for lookup, so either case +/// resolves to the same node. #[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] #[serde(rename_all = "camelCase")] pub struct RelForm { @@ -175,7 +176,9 @@ impl RelForm { /// A relationship type in the tenant's catalogue (`GET /edges/types`). /// /// Types are normally created on demand the first time a name is used; this is what one looks -/// like once stored. `hash` is the server's lookup key for the normalised name. +/// like once stored. Only `id`, `name`, `description` and `i18n_code` cross the wire: the server +/// hides `hash`, `date_created` and `last_updated`, so those three are always at their defaults +/// here however the stored row looks. #[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] #[serde(rename_all = "camelCase")] pub struct RelationshipType { diff --git a/src/relations/service.rs b/src/relations/service.rs index 57fefd0..591d205 100644 --- a/src/relations/service.rs +++ b/src/relations/service.rs @@ -135,17 +135,14 @@ impl EdgesService { /// Names are case-insensitive and normalised to uppercase snake case (`Flows To` → /// `FLOWS_TO`); a blank name, or one that normalises to nothing, is a 400. /// - /// **A name that already exists currently fails silently.** The service has no find-or-create: - /// it saves a fresh entity unconditionally, so a duplicate collides on the unique name hash at - /// *commit* time — after the handler has returned — and the caller gets a **200 with an empty - /// body** rather than the documented "existing ones returned unchanged". Worse in a batch: - /// every form is saved in one transaction, so a single duplicate rolls back the valid new - /// types alongside it and the response still says 200. - /// - /// Until that is fixed, treat a 200 with no items as "something in this batch already - /// existed and *nothing* was created", and use [`types`](Self::types) to see the real state. - /// The intended behaviour is a 409, matching [`create`](Self::create) on a duplicate edge; - /// `test_duplicate_relationship_type_conflicts` encodes that and is red until then. + /// **A name that already exists is a 409**, matching [`create`](Self::create) on a duplicate + /// edge. The problem names `name` in its `fields` extension. This used to escape the handler + /// as a commit-time constraint collision and reach the caller as a 200 with an empty body; + /// `test_duplicate_relationship_type_conflicts` stands as the regression guard. + /// + /// **A batch is still all-or-nothing.** The service has no find-or-create and saves every + /// form in one transaction, so a single duplicate rolls back the valid new types alongside + /// it — now with a 409 to say so. Use [`types`](Self::types) to see the real state. pub async fn create_types( &self, data: &I, diff --git a/src/relations/tests.rs b/src/relations/tests.rs index 18cc2d4..549a61f 100644 --- a/src/relations/tests.rs +++ b/src/relations/tests.rs @@ -646,18 +646,17 @@ mod live { /// Re-registering a relationship type that already exists must conflict, the way every other /// duplicate in this API does. /// - /// **Currently red, on purpose.** `EdgeService.saveRelationshipType` has no find-or-create: it - /// builds a fresh entity and saves it unconditionally, so a duplicate name collides on - /// `relationship_hash_key` at *commit* time — after the handler has returned. The - /// `DataIntegrityViolationException` escapes past the handler's `catch`, and the caller gets a - /// **200 with an empty body**: the status of a success with the body of a crash. + /// **A regression guard.** This was red on purpose for a while: + /// `EdgeService.saveRelationshipType` has no find-or-create, so a duplicate name collided on + /// `relationship_hash_key` at *commit* time — after the handler had returned. The + /// `DataIntegrityViolationException` escaped past the handler's `catch` and the caller got a + /// **200 with an empty body**: the status of a success with the body of a crash. It is now + /// mapped to **409** `duplicate`, naming `name` in `fields`, like `POST /edges/create` on a + /// duplicate edge. /// - /// The batch case is the damaging one. `createRelationshipTypes` saves every form in one + /// The batch case is still worth knowing: `createRelationshipTypes` saves every form in one /// transaction, so a single duplicate rolls the whole thing back — valid new types in the same - /// request are discarded too, and the response still says 200. Nothing tells the caller. - /// - /// This test encodes the intended behaviour (409, like `POST /edges/create` on a duplicate - /// edge) and will pass once the server-side fix lands. + /// request are discarded too, now with a 409 rather than a silent 200. #[tokio::test] async fn test_duplicate_relationship_type_conflicts() -> Result<(), Box> { diff --git a/src/resources/mod.rs b/src/resources/mod.rs index fcd44c6..f9df949 100644 --- a/src/resources/mod.rs +++ b/src/resources/mod.rs @@ -53,12 +53,14 @@ impl ResourceService { /// /// - **`DATASET` and `POLICY` nodes need the all-datasets manage grant**, and are a 403 /// without it. - /// - **`data_set_id` is silently dropped on those two types.** A data set or policy that - /// belonged to a data set would orphan its own ACL grant, so the api refuses to set it - /// rather than erroring. + /// - **`data_set_id` on those two types is a 400**, not a silent drop. A data set or policy + /// that belonged to a data set would orphan its own ACL grant, so the api says so rather + /// than dropping the field. Hierarchy is expressed with `BELONGS_TO` edges instead. The + /// grant check runs first, so a caller without the manage grant still sees the 403. /// - /// Note also that a duplicate `external_id` surfaces here as a constraint violation rather - /// than the clean 409 `/timeseries/create` answers with — a known asymmetry in the api. + /// A duplicate `external_id` is a clean **409** carrying the offending value in the problem's + /// `duplicated` extension, the same as `/timeseries/create` — the old asymmetry, where this + /// path surfaced a raw constraint violation, is gone. pub async fn create>( &self, nodes: Vec, @@ -240,9 +242,11 @@ pub struct Resource { /// [`geojson::Geometry::new_point`] and friends (re-exported as /// [`crate::Geometry`]). /// - /// **Write-only on a plain resource.** The api accepts it on create/update but never echoes - /// it back on a `Resource`, so this is always `None` on a read. A node created with the - /// `ASSET` type-label comes back as [`crate::nodes::Asset`], which does carry it. + /// **Not a field of a plain resource.** Only an asset declares a location, so sending this in + /// an unlabelled `/resources/create` body is a **400** naming `geoLocation` as an unknown + /// field, and it is always `None` on a read. Label the node `ASSET` and it binds to + /// [`crate::nodes::Asset`], which does carry it. Update is the exception: the shared form + /// accepts [`ResourceUpdateFields::geolocation`] for any node type and applies it to assets. #[serde(rename = "geoLocation", skip_serializing_if = "Option::is_none")] pub geolocation: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -586,8 +590,10 @@ pub struct ResourceFilter { /// Request body for [`ResourceService::fetch_nearest`] (`POST /resources/fetch-nearest`). /// -/// Note the endpoint reads `id` only: it does not resolve `external_id`, so start from a numeric -/// id (resolve one with [`by_ids`](ResourceService::by_ids) if that is all you have). +/// The endpoint takes `id` **or** `externalId` and resolves either, answering 404 for an unknown +/// one. **This form carries only `id`** — the SDK has not added the `externalId` side yet — so +/// start from a numeric id, resolving one with [`by_ids`](ResourceService::by_ids) if that is all +/// you have. #[derive(Debug, Serialize, Deserialize, Clone, Default)] #[serde(rename_all = "camelCase")] pub struct FetchNearestResourcesForm { From 734ad14a07d1d240cdddd39558691dedf81e55df Mon Sep 17 00:00:00 2001 From: JosteinGj <66584533+JosteinGj@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:07:43 +0200 Subject: [PATCH 2/2] Update mod.rs --- src/assets/mod.rs | 45 +++++++-------------------------------------- 1 file changed, 7 insertions(+), 38 deletions(-) diff --git a/src/assets/mod.rs b/src/assets/mod.rs index 185d2c8..1d06b92 100644 --- a/src/assets/mod.rs +++ b/src/assets/mod.rs @@ -14,21 +14,6 @@ use std::sync::Weak; /// Client for the `/assets` endpoints — the typed view of the `ASSET`-labelled corner of the /// resource graph. /// -/// Every call here is the generic `/resources` pipeline with the `ASSET` discriminator pinned -/// server-side, so the two paths cannot drift apart on ACLs, naming policy or status codes. The -/// difference is the shape that comes back: [`Asset`] rather than the polymorphic -/// [`Node`](crate::nodes::Node), so `geolocation` and `is_root` are reachable without a match. -/// -/// The filter and search bodies are the resource ones — [`ResourceFilterForm`] and -/// [`SearchAndFilterForm`] — because the api declares the same Java types on both -/// endpoint families. A `node_type` set on either is **replaced**, not merged: the api pins -/// `["asset"]` over whatever the caller asked for, since `node_type` entries OR together and -/// leaving a caller's `["timeseries"]` in place would widen a request to `/assets` into a mixed -/// query. -/// -/// What it does *not* share with `/functions` is worth knowing: the api serves the same three -/// reads there, but the SDK's [`FunctionsService`](crate::functions::FunctionsService) has not -/// wired them yet and still filters a listing client-side. pub struct AssetsService { pub(crate) api_service: Weak, base_url: String, @@ -51,16 +36,6 @@ impl AssetsService { /// `POST /assets/create` — create one or more assets. Each needs a unique `external_id` and a /// `name`; both are required server-side. - /// - /// Unlike [`ResourceService::create`](crate::resources::ResourceService::create), the `ASSET` - /// type-label does not have to be set by the caller: the endpoint deserializes straight into - /// the api's `Asset`, whose constructor forces the label in. [`Asset::new`] leaves `labels` - /// unset for that reason, and any domain labels you do set are kept alongside it. - /// - /// Relations are not creatable here — this takes nodes only. Use - /// [`ResourceService::create`](crate::resources::ResourceService::create) to build assets and - /// the edges between them in one call, or [`EdgesService`](crate::relations::EdgesService) to - /// link assets that already exist. pub async fn create(&self, data: &I) -> Result, ResponseError> where for<'a> &'a I: Into>, @@ -71,12 +46,7 @@ impl AssetsService { } /// `GET /assets/{id}` — one asset by its numeric id. - /// - /// **404 does not mean the id is free.** A node that exists but is not an asset, and an asset - /// the caller may not read, are both reported as missing — the api hides existence rather than - /// answering 403. So a 404 here says "not an asset you can read", nothing more. - /// - /// Unlike [`by_ids`](Self::by_ids), which omits what it cannot find, this is an error. + /// Errors: 404 on a missing or inaccessible asset. pub async fn get_by_id(&self, id: u64) -> Result, ResponseError> { let path = &format!("{}/{}", self.base_url, id); self.execute_get_request::, ()>(path, None) @@ -97,22 +67,21 @@ impl AssetsService { .await } - /// `GET /assets?limit=N` — the first `limit` assets you may read, newest created first. + /// `GET /assets?limit=N` /// /// `None` sends no `limit` and leaves the server's default of 1000 in place; the maximum is - /// 10000, above which the server answers 400 rather than clamping. + /// 10000, above which the server answers 400 rather than clamping. Newest created first. /// - /// Assets are the bulk of a tenant, so this is a sample rather than an inventory, and there is - /// no paging: the api nulls `next_cursor` here deliberately, because a walk needs a `sort` and - /// a `cursor` and both live in a request body. Narrow with [`filter`](Self::filter) rather - /// than raising the number. + /// There is no paging: the api nulls `next_cursor` here deliberately, because a walk needs a `sort` and + /// a `cursor` and both live in a request body. Narrow with [`filter`](Self::filter) rather + /// than raising the number. pub async fn list(&self, limit: Option) -> Result, ResponseError> { let query = limit.map(|limit| [("limit", limit)]); self.execute_get_request::, _>(&self.base_url, query.as_ref()) .await } - /// `POST /assets/filter` — the structured query, narrowed to assets. + /// `POST /assets/filter` /// /// Takes the resource filter body unchanged, so [`ResourceFilter::is_root`] and /// [`ResourceFilter::data_set_id`] are available here and the shared