Expect a 400, not a 404, for a record that is gone
Running the password tier against a disposable 5.13.50 instance for the first time failed its last check. The purge itself was fine — the row really was gone — but the assertion looked for a 404 and the server answers 400. `Handler.php` maps Laravel's `ModelNotFoundException` to a 400 carrying `No query results for model [App\Models\X].` and reserves 404 for a route that does not exist, so a not-found check written against 404 never fires. The assertion now matches the status together with the message, so a 400 raised for some other reason still fails, and `Error::Api` says so for callers, who would otherwise write the same dead branch. Also adds jq to the devshell, which the MSRV recheck in CLAUDE.md pipes through but which was only ever resolving from the host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -158,8 +158,9 @@ if a typed row lost a key (`keys kept: <resource>`, names only, never
|
|||||||
values), and the write tier creates a purchase order, location, webhook and
|
values), and the write tier creates a purchase order, location, webhook and
|
||||||
recurring invoice from bare `Default` models, since those are what broke.
|
recurring invoice from bare `Default` models, since those are what broke.
|
||||||
With `IN_API_PASSWORD` set as well, a password tier merges two throwaway
|
With `IN_API_PASSWORD` set as well, a password tier merges two throwaway
|
||||||
clients and purges the survivor (a real, permanent delete); it has not yet
|
clients and purges the survivor (a real, permanent delete); it was run
|
||||||
been run against an instance. `.github/workflows/smoke.yml` runs it on demand (`workflow_dispatch` only,
|
against a disposable 5.13.50 instance on 2026-10-24 and all four checks
|
||||||
|
pass. `.github/workflows/smoke.yml` runs it on demand (`workflow_dispatch` only,
|
||||||
needing `IN_BASE_URL` and `IN_API_TOKEN` secrets); the write tier runs only if
|
needing `IN_BASE_URL` and `IN_API_TOKEN` secrets); the write tier runs only if
|
||||||
the `write_host` input names the instance's host.
|
the `write_host` input names the instance's host.
|
||||||
|
|
||||||
@@ -311,6 +312,12 @@ changes, doc tweaks) does not need an entry.
|
|||||||
decode, API error responses (with per-field validation `errors` map), and
|
decode, API error responses (with per-field validation `errors` map), and
|
||||||
client construction errors. `ApiErrorBody` mirrors Invoice Ninja's error
|
client construction errors. `ApiErrorBody` mirrors Invoice Ninja's error
|
||||||
JSON shape.
|
JSON shape.
|
||||||
|
- **A missing record is a `400`, not a `404`.** `Handler.php` maps Laravel's
|
||||||
|
`ModelNotFoundException` to a 400 carrying `No query results for model
|
||||||
|
[App\Models\X].`, and keeps 404 for a route that does not exist or a method
|
||||||
|
it does not support. So don't write a not-found check against 404 — the
|
||||||
|
smoke test's post-purge assertion did, and passed only because it had
|
||||||
|
never been run. Match the status together with the message.
|
||||||
|
|
||||||
## Coverage
|
## Coverage
|
||||||
|
|
||||||
|
|||||||
+6
-6
@@ -274,12 +274,12 @@ session through without the header). Most are **irreversible**, and all are
|
|||||||
| `POST /users/{id}/purge` | `purge_user` | `()` (the server answers 204) |
|
| `POST /users/{id}/purge` | `purge_user` | `()` (the server answers 204) |
|
||||||
| `GET /users/create` | `new_user_template` | a blank `User`, nothing stored |
|
| `GET /users/create` | `new_user_template` | a blank `User`, nothing stored |
|
||||||
|
|
||||||
A merge keeps the first id and folds the second into it. These are verified
|
A merge keeps the first id and folds the second into it. The smoke test's
|
||||||
against the upstream controllers and mock servers only: running them needs the
|
optional password tier (merge two throwaway clients, purge the survivor) was
|
||||||
account password, so the smoke test's optional password tier (merge two
|
run against a disposable 5.13.50 instance on 2026-10-24 and passes; it also
|
||||||
throwaway clients, purge the survivor) has not yet been run against a real
|
established that a purged record reads back as a **400**, not a 404. A
|
||||||
instance. A password containing a control character cannot be a header value
|
password containing a control character cannot be a header value and goes out
|
||||||
and goes out as `X-API-PASSWORD-BASE64` instead.
|
as `X-API-PASSWORD-BASE64` instead.
|
||||||
|
|
||||||
## Per-resource extras
|
## Per-resource extras
|
||||||
|
|
||||||
|
|||||||
@@ -813,9 +813,14 @@ async fn password_checks(client: &InvoiceNinjaClient, report: &mut Report, passw
|
|||||||
}
|
}
|
||||||
|
|
||||||
// A purge is permanent, so the record must now be gone, not just flagged.
|
// A purge is permanent, so the record must now be gone, not just flagged.
|
||||||
|
// A missing record answers 400, not 404: the server maps Laravel's
|
||||||
|
// ModelNotFoundException to a 400 and keeps 404 for a missing route. The
|
||||||
|
// message is checked too, so a 400 raised for some other reason fails.
|
||||||
match client.get_client(&keep).await {
|
match client.get_client(&keep).await {
|
||||||
Err(Error::Api { status, .. }) if status.as_u16() == 404 => {
|
Err(Error::Api { status, message, .. })
|
||||||
report.pass("get_client after purge", "404, the row is gone")
|
if status.as_u16() == 400 && message.contains("No query results") =>
|
||||||
|
{
|
||||||
|
report.pass("get_client after purge", "400 no query results, the row is gone")
|
||||||
}
|
}
|
||||||
Err(e) => report.fail("get_client after purge", describe(&e)),
|
Err(e) => report.fail("get_client after purge", describe(&e)),
|
||||||
Ok(_) => report.fail("get_client after purge", "still readable after a purge"),
|
Ok(_) => report.fail("get_client after purge", "still readable after a purge"),
|
||||||
|
|||||||
@@ -37,7 +37,10 @@
|
|||||||
msrvToolchain = toolchainFor msrvVersion [ ];
|
msrvToolchain = toolchainFor msrvVersion [ ];
|
||||||
|
|
||||||
shellFor = toolchain: pkgs.mkShell {
|
shellFor = toolchain: pkgs.mkShell {
|
||||||
packages = [ toolchain pkgs.pkg-config pkgs.openssl ];
|
# jq is for the MSRV recheck in CLAUDE.md, which pipes `cargo
|
||||||
|
# metadata` through it; the rest is the toolchain plus what
|
||||||
|
# openssl-sys needs to link.
|
||||||
|
packages = [ toolchain pkgs.pkg-config pkgs.openssl pkgs.jq ];
|
||||||
RUST_SRC_PATH = "${toolchain}/lib/rustlib/src/rust/library";
|
RUST_SRC_PATH = "${toolchain}/lib/rustlib/src/rust/library";
|
||||||
};
|
};
|
||||||
in
|
in
|
||||||
|
|||||||
@@ -35,6 +35,15 @@ pub enum Error {
|
|||||||
excerpt: String,
|
excerpt: String,
|
||||||
},
|
},
|
||||||
|
|
||||||
|
/// A non-2xx response: the server's message, plus any per-field
|
||||||
|
/// validation errors.
|
||||||
|
///
|
||||||
|
/// **A missing record is `400`, not `404`.** Invoice Ninja's exception
|
||||||
|
/// handler maps Laravel's `ModelNotFoundException` to a 400 whose message
|
||||||
|
/// is `No query results for model [App\Models\Client].`, and reserves 404
|
||||||
|
/// for a route that does not exist or a method that route does not
|
||||||
|
/// support. Matching on 404 to mean "not found" will never fire — match
|
||||||
|
/// the status together with the message.
|
||||||
#[error("api returned {status}: {message}")]
|
#[error("api returned {status}: {message}")]
|
||||||
Api {
|
Api {
|
||||||
status: reqwest::StatusCode,
|
status: reqwest::StatusCode,
|
||||||
|
|||||||
Reference in New Issue
Block a user