fix(client): render the error envelope's sentence in inline warnings - #308
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// | ||
| /// Unlike [`print`](Self::print) it never probes the stored credential, so | ||
| /// it needs no live `Api` and adds no auth or token-scope hint. | ||
| pub fn user_message(&self) -> String { |
There was a problem hiding this comment.
nit: two other message() callers are also user-facing, not string matchers (not blocking).
src/commands/query.rs:355 builds could not fetch full result ({}) and passes it to note_preview, which prints the note above the preview rows. src/commands/query.rs:1730 asserts the same string. Both still render the raw envelope, so a persisted-result fetch that returns {"error":{"message":"…"}} shows the JSON body inline.
Switch query.rs:355 to user_message() in this PR, or drop the claim from the PR description that the remaining callers are matchers or tests.
| ApiError::Status { status, body } if body.trim().is_empty() => { | ||
| format!("HTTP {status} (empty response body)") | ||
| } |
There was a problem hiding this comment.
nit: the empty-body arm has no test (not blocking).
This arm diverges from util::api_error, which returns "unexpected empty response from server" for a blank body (src/util.rs:446). user_message returns "HTTP {status} (empty response body)" instead. The divergence is deliberate and matches format_fail_message, but nothing pins it, so a later refactor that deletes the arm keeps every test green.
Add a case to api_error_message_formats_status_and_transport in the same module, or a sibling test asserting the status reaches the string for a 502 with an empty body.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
`ApiError::message` formatted the raw response body, so every warning
that folded an API error inline printed the JSON envelope instead of
the sentence inside it. `create --attach` showed it as
warning: could not attach 'src': 409 Conflict: {"error":{"message":
"this catalog is scoped to another database…","code":"CONFLICT"}}
and the query preview fallback did the same for a failed full-result
fetch ("could not fetch full result (404 Not Found: {"error":…})").
Fix it where it lives: `message` now runs the body through the same
unwrapping the fatal path already does, via a shared
`server_explanation`, so the inline and the fatal rendering of one
response cannot drift. The status stays in the line so a non-JSON body
still says what happened. Every `message` caller gets the fix at once;
nothing has to remember to pick a second accessor.
2970763 to
6dbbeef
Compare
eddietejeda
changed the title
| /// `format_fail_message` (fatal) build on it. | ||
| fn server_explanation(status: reqwest::StatusCode, body: &str) -> String { | ||
| if body.trim().is_empty() { | ||
| format!("HTTP {status} (empty response body)") |
There was a problem hiding this comment.
nit: a bodyless response prints the status twice in inline warnings (not blocking).
Fix: return only empty response body here, and let format_fail_message build error: HTTP {status} (empty response body) from the status it already has.
ApiError::message prefixes {status}: and this branch prefixes HTTP {status} again. A bodyless 502 reaches the user through src/commands/databases.rs:1981 as warning: could not attach 'src': 502 Bad Gateway: HTTP 502 Bad Gateway (empty response body). The assertion at src/client/sdk.rs:1141 pins that duplication.
| /// The exit path ([`format_fail_message`]) shares that base and adds the | ||
| /// credential-probe hints on top, which need a live `Api` this does not | ||
| /// have. | ||
| pub fn message(&self) -> String { |
There was a problem hiding this comment.
nit: the PR description describes a different change (not blocking).
Update the description before merge.
The description says the PR adds ApiError::user_message and leaves message() alone. This commit changes message() itself, which is a wider change: every caller now sees the unwrapped text, including fail_query and attach_connection. The description also lists a test named failed_attach_reports_the_servers_sentence_not_its_envelope, which the diff does not contain. The description becomes the squash-merge commit body, so the permanent record names a method and a test that do not exist.
1 participant
Summary
ApiError::messageformatted the raw response body, so every warning that folds an API error inline printed the JSON envelope instead of the sentence inside it.create --attachshowed:and — as review pointed out — the query preview fallback did the same for a failed full-result fetch (
could not fetch full result (404 Not Found: {"error":…})), which the first version of this PR missed.Fixed at the root rather than at one call site:
messagenow runs the body through the same unwrapping the fatal path (format_fail_message) already does, via a sharedserver_explanation, so the inline and the fatal rendering of one response cannot drift apart. The status stays in the line so a non-JSON body still says what happened. Everymessagecaller gets the fix at once; nothing has to remember to reach for a second accessor. The only string-consuming caller,cross_source_hint, matches text that lives insideerror.message, so it is unaffected.Not included, deliberately: the 403
ACCESS_DENIEDtoken-scope hint stays on the fatal path only. It is a multi-line hint, andmessageis interpolated into single-line parentheticals.Noticed while working runtimedb#1382 / runtimedb#1412, which is where this error text comes from.
Test plan
api_error_message_unwraps_the_error_envelope—409 Conflict: this catalog is scoped …, no JSON.api_error_message_and_fail_message_agree_on_an_empty_body— pins the two paths to the same base text.a_stream_that_cannot_open_falls_back_to_the_previewnow mocks a real envelope and asserts the warning quotes the sentence, not{"error"— so it guardsquery.rs:355, which it did not before.cargo test— 528 unit + all integration targets pass;cargo clippy --all-targets -- -D warningsandcargo fmt --checkclean.