Add multipart document uploads for the 16 resources that have the route
First item on the GOALS.md roadmap. `upload_{x}(id, files, is_public)`
attaches files through `/{resource}/{id}/upload` and returns the
refreshed record. Verified against upstream that all 16 controllers
answer with `itemResponse($x->fresh())`.
Three details that look wrong but are not, each checked in the upstream
source rather than taken from the published spec:
- The request is a POST carrying `_method=PUT`. The route is registered
PUT, but PHP does not parse a multipart body on a real PUT, so the
files never arrive. Laravel's kernel calls
`enableHttpMethodParameterOverride()`, turning the spoofed method back
into a PUT before routing. The spec documents POST with `_method=POST`,
which would 405 against these routes.
- `is_public` is sent as 1/0. Laravel's `boolean` rule accepts only
true, false, 0, 1, '0' and '1' under strict comparison, so "true"
fails validation.
- Uploads are never retried. An upload adds a document, so it is not
idempotent whatever its verb, and a replay after a lost response would
attach the file twice. This falls out of reqwest's multipart body being
un-cloneable, which nothing in the types enforces, so a test pins it
for 500, 503 and 429.
An upload with no files, a nameless file or an unparseable MIME type is
refused locally (`Error::InvalidUpload`), since the server answers an
empty upload with a success that changed nothing.
Corrects two earlier claims:
- GOALS.md and COVERAGE.md said uploads would make `create_document`
possible. They do not: `DocumentController::store` is an empty stub,
and an upload is the only way a document is created. There is no
`create_document`.
- The documented method count (265) had been stale since the typed
`statics`/`search`/`health_check` commit, which added three `_as`
methods without updating it. Recounted from the source: 284 = 210 +
18 + 12 + 16 uploads + 28 hand-written.
Also fixes a README example, `let statics: Value = client.statics()`,
that stopped compiling when `statics` lost its type parameter.
`edocument/upload` is left out: it imports an expense from an
e-document rather than attaching a file, so it moves to the
import/export roadmap item.
Enabling `multipart` adds mime, mime_guess and unicase; MSRV stays 1.88,
re-verified by running the suite on it. 101 tests, up from 91.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -141,11 +141,16 @@ All 34 `Route::resource` groups are wrapped. Full coverage is the target, so
|
||||
`COVERAGE.md`'s gap list is a to-do list, not a permanent boundary. Roughly
|
||||
in order of value over effort:
|
||||
|
||||
1. **Multipart uploads** — 16 `PUT /{resource}/{id}/upload` routes plus
|
||||
`edocument/upload`. Blocked only on enabling reqwest's `multipart`
|
||||
feature. This is also what makes `create_document` possible, so it
|
||||
unblocks the one obviously missing piece of an otherwise complete
|
||||
resource.
|
||||
**Done:** multipart document uploads, for the 16 resources that have the
|
||||
route (`upload_client`, `upload_invoice`, …). This earlier claimed it would
|
||||
also make `create_document` possible, which was wrong: upstream
|
||||
`DocumentController::store` is an empty stub, and an upload is the *only*
|
||||
way a document is created. There is no `create_document` and there will not
|
||||
be one. `edocument/upload` was left out because it imports an expense from
|
||||
an e-document rather than attaching a file, so it belongs with item 6.
|
||||
|
||||
Still to do:
|
||||
|
||||
2. **Per-resource extras** — mechanical, and they reuse existing patterns:
|
||||
`webhooks/{id}/retry`, `tasks/sort`, `designs/set/default`,
|
||||
`client_gateway_tokens/{id}/setAsDefault`, `subscriptions/steps`, the
|
||||
@@ -159,9 +164,10 @@ in order of value over effort:
|
||||
5. **Public invitation-key downloads** — eight routes that take no auth
|
||||
token, so they do not fit the authenticated request path and need a
|
||||
different entry point.
|
||||
6. **Import/export and the integrations** — Stripe, QuickBooks, einvoice
|
||||
(17 routes), Yodlee, Nordigen, calendar. The largest block and the least
|
||||
SDK-shaped; worth doing last and possibly in pieces.
|
||||
6. **Import/export and the integrations** — `/import`, `/export`,
|
||||
`edocument/upload`, Stripe, QuickBooks, einvoice (17 routes), Yodlee,
|
||||
Nordigen, calendar. The largest block and the least SDK-shaped; worth
|
||||
doing last and possibly in pieces.
|
||||
|
||||
Keep `COVERAGE.md` current as these land — it names exact counts, so it goes
|
||||
stale quietly.
|
||||
@@ -178,9 +184,12 @@ These are genuinely undecided. Deciding one is a change to this file.
|
||||
eleven chart endpoints share no response shape. Per-chart structs would
|
||||
be eleven small models of unstable payloads; unclear whether that earns
|
||||
its keep.
|
||||
- **Large responses are buffered entirely into memory.** PDF downloads and
|
||||
CSV report exports go through `Vec<u8>`. Streaming would be better for
|
||||
big exports but changes those signatures.
|
||||
- **Large bodies are buffered entirely into memory.** PDF downloads and CSV
|
||||
report exports come back as `Vec<u8>`, and an uploaded `FileUpload` holds
|
||||
its whole contents until the request finishes. Streaming would be better
|
||||
for big files in both directions but changes those signatures — and for
|
||||
uploads it would also change the retry story, which currently depends on
|
||||
the multipart body being un-cloneable.
|
||||
- **`Error::PaginationLimit` has no test**, because reaching it needs
|
||||
10,001 mock round trips. Making `MAX_PAGES` injectable purely for a test
|
||||
may be worse than the gap.
|
||||
|
||||
Reference in New Issue
Block a user