Wrap all 34 resource groups, then harden the request plumbing
Expands coverage to every `Route::resource` group in the v5 API plus `users` (265 methods), and fixes several defects in the shared request path that the new breadth made more exposed. Coverage: - 25 new models, each keeping the `extra` flatten map and the `null_to_default` convention so unmodeled fields round-trip. - `crud_methods!` split out of `resource_methods!` for the three groups with no `/bulk` route, where a generated `bulk_*` would only 405. - COVERAGE.md added as the detailed inventory, verified against `routes/api.php` on v5-stable. Fixes, each confirmed against upstream source or an observed request: - `list_all_*` silently truncated. `BaseController` caps page size at `min(abs(per_page), 5000)`, so the "short page means last page" rule stopped early and returned a partial list as if complete. Termination now prefers `meta.pagination.total_pages` (decoded but previously discarded), and `per_page` is clamped so the fallback stays sound. - `per_page(0)` paged forever: the check `got < 0` has no solution. It now falls back to the default size, with a MAX_PAGES backstop that raises `Error::PaginationLimit` rather than quietly truncating. - Ids were interpolated into paths raw, so a `/` or `?` in an id retargeted the request. All sites now use `seg(id)?`. Escaping alone is insufficient: the WHATWG rules `url` implements decode `%2E` to `.` before resolving dot segments, so an escaped `..` still collapses the path. Empty and dot-only ids are rejected. - No request timeout. reqwest applies none, so a stalled connection hung the caller indefinitely. Adds a 60s default and `with_timeout`. - Base URL is validated at construction; a missing scheme used to 404 on every call instead of failing once. Adds tests/client_requests.rs: 26 tests over the request plumbing against a wiremock loopback server, including regressions for the truncation and non-termination bugs. No network or credentials needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -29,37 +29,112 @@ IN_API_TOKEN=... cargo run --example list_clients
|
||||
```
|
||||
|
||||
Tests live in `tests/` (integration-style, no network access required);
|
||||
`src/` has no `#[test]` modules.
|
||||
`src/` has no `#[test]` modules. Two files: `model_roundtrip.rs` covers the
|
||||
serde layer, and `client_requests.rs` covers the request plumbing
|
||||
(pagination, path escaping, headers, timeouts, error mapping) against a
|
||||
`wiremock` server on loopback — no credentials or internet needed.
|
||||
|
||||
## Architecture
|
||||
|
||||
- `src/client.rs` — `InvoiceNinjaClient` holds a `reqwest::Client` (with the
|
||||
`X-Api-Token` auth header baked in) and a `base_url`. Low-level helpers
|
||||
(`get_list`, `get_one`, `post_one`, `put_one`, `delete_one`) handle request
|
||||
dispatch and decode `ListResponse<T>` / `ItemResponse<T>` envelopes, mapping
|
||||
non-2xx responses through `api_error` into `Error::Api`.
|
||||
(`get_list`, `get_one`, `post_one`, `put_one`, `delete_one`, `post_list`)
|
||||
handle request dispatch and decode `ListResponse<T>` / `ItemResponse<T>`
|
||||
envelopes, mapping non-2xx responses through `api_error` into `Error::Api`.
|
||||
- Not every endpoint answers with a JSON envelope, so there are three other
|
||||
helper families: `get_raw`/`post_raw` return the body as bytes (PDF streams
|
||||
and CSV report exports), `get_unwrapped`/`post_unwrapped` decode a bare
|
||||
JSON body with no `data` wrapper (`/statics`, `/charts/*`, `/search`), and
|
||||
`get_discard` just checks the status (`/ping`).
|
||||
- Per-resource methods (`list_clients`, `get_invoice`, `create_product`, etc.)
|
||||
are generated by the `resource_methods!` macro at the bottom of
|
||||
`client.rs`, which expands to
|
||||
`list`/`list_all`/`get`/`create`/`update`/`delete` for a given REST path and
|
||||
model type. **To add a new resource**: add a model under `src/models/`,
|
||||
re-export it from `src/models/mod.rs`, and add one `resource_methods!`
|
||||
invocation in `client.rs`. Note the macro takes six method names before the
|
||||
path and model — adding a resource means passing all six.
|
||||
`list`/`list_all`/`get`/`create`/`update`/`delete`/`bulk` for a given REST
|
||||
path and model type. **To add a new resource**: add a model under
|
||||
`src/models/`, re-export it from `src/models/mod.rs`, and add one
|
||||
`resource_methods!` invocation in `client.rs`. Note the macro takes seven
|
||||
method names before the path and model — adding a resource means passing
|
||||
all seven.
|
||||
- `resource_methods!` delegates the six CRUD methods to `crud_methods!` and
|
||||
adds `bulk` on top. **Use `crud_methods!` directly for resources with no
|
||||
`/bulk` route** — currently `client_gateway_tokens`, `companies`, and
|
||||
`system_logs`. Check `routes/api.php` before assuming a resource has one;
|
||||
a generated `bulk_*` against a missing route just 405s.
|
||||
- Every request is built through the private `req(method, path)` helper,
|
||||
which is the single place the optional `X-API-PASSWORD` header and the
|
||||
per-request timeout are attached. **Don't call `self.http` directly in a
|
||||
new helper** — it would silently skip both. Only `req` itself should touch
|
||||
`self.http`.
|
||||
- **Interpolate ids into paths with `seg(id)?`, never raw.** It escapes the
|
||||
value into a single path segment and rejects ids that are empty or
|
||||
dot-only. Both halves matter: escaping stops a `/` or `?` in an id from
|
||||
retargeting the request, but escaping *alone* is not enough, because the
|
||||
WHATWG rules the `url` crate follows decode `%2E` back to `.` before
|
||||
resolving dot segments — so an escaped `..` still collapses the path. The
|
||||
exceptions are `report` and `chart`, which take a multi-segment tail by
|
||||
design (`chart("project_burnup/<id>", …)`) and must stay unescaped.
|
||||
- `entity_action_methods!` generates three methods — `{x}_action` and
|
||||
`download_{x}` for the `GET /{resource}/{id}/{action}` route, plus
|
||||
`download_{plural}` for the bulk merged-PDF action. **Only invoices,
|
||||
quotes, credits, and purchase orders have the action route** — don't add it
|
||||
elsewhere without checking `routes/api.php`. In particular the official PHP
|
||||
SDK puts bulk `download` on *every* resource, which is wrong; only
|
||||
PDF-rendering entities support it.
|
||||
- Non-CRUD endpoints (documents, reports, charts, search, statics, emails,
|
||||
refunds, ping/health) are hand-written in the last `impl` block rather than
|
||||
macro-generated, because their request and response shapes are all
|
||||
different.
|
||||
- `list_*` returns one page; `list_all_*` goes through `get_all_pages`, which
|
||||
follows `page`/`per_page` until a page returns fewer rows than requested.
|
||||
- `src/models/` — one file per resource (`client.rs`, `invoice.rs`,
|
||||
`payment.rs`, `product.rs`, `expense.rs`, `project.rs`, `task.rs`,
|
||||
`vendor.rs`), plus `common.rs` for shared envelope/pagination types
|
||||
(`ListParams`, `ListResponse`, `ItemResponse`, `Meta`, `Pagination`).
|
||||
Invoice Ninja stores quotes and credits in the same table as invoices (an
|
||||
`entity_type` field distinguishes them), so `Quote` and `Credit` are type
|
||||
aliases for `Invoice` (`src/models/invoice.rs`) and share the
|
||||
`/quotes`, `/credits`, and `/invoices` paths via the macro.
|
||||
terminates on `meta.pagination.total_pages` and only falls back to the
|
||||
short-page heuristic when the server sends no metadata. **The heuristic is
|
||||
not sound on its own**: `BaseController` caps `per_page` at
|
||||
`min(abs(per_page), 5000)`, so a page can be shorter than requested while
|
||||
records remain — which is why `per_page` is clamped to 5000 before the
|
||||
walk starts. `per_page(0)` falls back to the default size (it would make
|
||||
the short-page check `got < 0`, which never terminates), the page counter
|
||||
advances locally rather than trusting the echoed `current_page`, an empty
|
||||
page always ends the walk, and a `MAX_PAGES` backstop raises
|
||||
`Error::PaginationLimit` rather than quietly returning a truncated list.
|
||||
- `src/models/` — one file per resource, plus `common.rs` for shared
|
||||
envelope/pagination types (`ListParams`, `ListResponse`, `ItemResponse`,
|
||||
`Meta`, `Pagination`).
|
||||
- Several endpoints share a schema, so some models are type aliases rather
|
||||
than separate structs: Invoice Ninja keeps quotes and credits in the
|
||||
invoices table (an `entity_type` field distinguishes them), so `Quote` and
|
||||
`Credit` alias `Invoice` (`src/models/invoice.rs`); likewise
|
||||
`RecurringQuote` aliases `RecurringInvoice`
|
||||
(`src/models/recurring_invoice.rs`), whose fields are a strict superset.
|
||||
Each alias still gets its own path via the macro.
|
||||
- Every model struct has a `#[serde(flatten)] pub extra: Map<String, Value>`
|
||||
field. This preserves any API fields not explicitly modeled, so a `get`
|
||||
followed by an `update` round-trips without silently dropping data. Keep
|
||||
this pattern when adding fields or new models.
|
||||
- **Every non-`Option` field carries
|
||||
`#[serde(default, deserialize_with = "super::common::null_to_default")]`,
|
||||
not a bare `#[serde(default)]`.** `default` alone only covers a *missing*
|
||||
key; a key present with an explicit `null` fails with `invalid type: null,
|
||||
expected a string`. Invoice Ninja returns `null` for many
|
||||
nominally-string fields, so a bare `default` would break decoding of the
|
||||
whole response. `Option<T>` fields keep the plain `#[serde(default)]` —
|
||||
they already map `null` to `None`, and that distinction matters (an unset
|
||||
`archived_at` should stay `None`, not become `Some(0)`).
|
||||
- Models deliberately cover a curated subset of each schema and let `extra`
|
||||
carry the rest — `custom_value1..4` and `custom_surcharge1..4` are
|
||||
intentionally unmodeled. Fields whose JSON type is ambiguous or
|
||||
inconsistent in the upstream spec (e.g. `Subscription.currency_id`,
|
||||
`webhook_configuration`, `due_date_days`) are also left to `extra`, since a
|
||||
wrong concrete type is a hard deserialize error whereas `extra` is lossless.
|
||||
- Field names and types come from Invoice Ninja's own OpenAPI spec
|
||||
(`openapi/api-docs.yaml` on the `v5-stable` branch of the invoiceninja
|
||||
repo). Check it before adding or retyping a field; the same name can differ
|
||||
in type across resources (`frequency_id` is a number on recurring invoices
|
||||
and expenses but a string on subscriptions).
|
||||
- **The published spec is incomplete for routes**: it documents `/{id}` and
|
||||
action paths only for clients, locations, and tags, and omits them for
|
||||
invoices and everything else. `routes/api.php` in the same repo is the
|
||||
authoritative route list, and the controllers are authoritative for what a
|
||||
given action returns. Use the spec for *schemas*, the routes and
|
||||
controllers for *endpoints*.
|
||||
- `src/error.rs` — `Error` enum (`thiserror`) covering transport, JSON
|
||||
decode, API error responses (with per-field validation `errors` map), and
|
||||
client construction errors. `ApiErrorBody` mirrors Invoice Ninja's error
|
||||
@@ -67,6 +142,42 @@ Tests live in `tests/` (integration-style, no network access required);
|
||||
|
||||
## Coverage
|
||||
|
||||
Clients, Invoices, Quotes, Credits, Payments, Products, Expenses, Projects,
|
||||
Tasks, and Vendors, each with `list`, `list_all`, `get`, `create`, `update`,
|
||||
and `delete`.
|
||||
**All 34 of the API's `Route::resource` groups are wrapped**, plus `users`
|
||||
(which isn't one). 265 methods: 30 resources with CRUD + bulk, 3 with CRUD
|
||||
only, 4 with entity actions, and 25 hand-written.
|
||||
|
||||
Plus the non-CRUD surface: documents, entity actions and PDF downloads for
|
||||
the four action-capable resources, `report`, `chart`, `search`, `statics`,
|
||||
`send_email`, `refund_payment`, `ping`, and `health_check`.
|
||||
|
||||
**`COVERAGE.md` is the detailed inventory** — every wrapped endpoint, every
|
||||
gap, and the reason for each gap. The main remaining gaps are multipart
|
||||
uploads (needs a reqwest feature), a handful of per-resource extras
|
||||
(`clients/{id}/purge`, `company_gateways/{id}/test`, …), and the
|
||||
integration/auth routes.
|
||||
|
||||
Keep `COVERAGE.md` current when adding or removing an endpoint: it names
|
||||
exact counts and route groups, so it goes stale quietly. The three coverage
|
||||
summaries — here, `README.md`, and `COVERAGE.md` — should agree;
|
||||
`COVERAGE.md` is the one with the specifics, the other two summarize and
|
||||
link to it.
|
||||
|
||||
## Relationship to the official PHP SDK
|
||||
|
||||
[invoiceninja/sdk-php](https://github.com/invoiceninja/sdk-php) is the
|
||||
first-party wrapper and a useful cross-check, but it is thinner than this
|
||||
crate — each endpoint class is just a URI over a shared `BaseEntity`. See the
|
||||
comparison table in `COVERAGE.md` for what each side covers.
|
||||
|
||||
Two divergences are deliberate and shouldn't be "fixed" to match it:
|
||||
|
||||
- **We send JSON; it sends `form_params`.** Form encoding bracket-encodes
|
||||
nested structures, which is bad for payloads like an invoice's `line_items`.
|
||||
The API accepts both.
|
||||
- **We keep the whole `errors` map on `Error::Api`;** it surfaces only the
|
||||
first validation message.
|
||||
|
||||
And two of its behaviours are bugs not to copy: it sends `stream: false` in
|
||||
download payloads (no controller reads a `stream` input), and it exposes
|
||||
`download` on every resource even though only PDF-rendering entities support
|
||||
that action.
|
||||
|
||||
Reference in New Issue
Block a user