Skip to content

Commit 2970763

Browse files
committed
fix(databases): unwrap the error envelope in the create --attach warning
`create --attach` folds a failed attach into a `warning:` line and carries on to the next spec. It built that line with `ApiError::message`, which formats the raw response body, so the user read the JSON envelope: warning: could not attach 'src': 409 Conflict: {"error":{"message": "this catalog is scoped to another database…","code":"CONFLICT"}} Adds `ApiError::user_message`, the non-exiting counterpart that runs the body through `util::api_error` the way `format_fail_message` already does for the print-and-exit path, and uses it here. No credential probing, so it needs no live `Api` and adds no auth hint.
1 parent 2848a8a commit 2970763

2 files changed

Lines changed: 68 additions & 1 deletion

File tree

‎src/client/sdk.rs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,28 @@ impl ApiError {
285285
}
286286
}
287287

288+
/// The server's own explanation, unwrapped from its JSON envelope.
289+
///
290+
/// Like [`message`](Self::message), this is for errors folded inline into
291+
/// another line rather than printed-and-exited. It differs in reaching for
292+
/// [`util::api_error`], so `{"error":{"message":"…"}}` renders as the
293+
/// sentence inside it instead of the raw body. Use this wherever the text
294+
/// lands in front of a person; `message` stays the right choice when the
295+
/// status code carries the meaning, or when something downstream reads the
296+
/// string rather than a reader.
297+
///
298+
/// Unlike [`print`](Self::print) it never probes the stored credential, so
299+
/// it needs no live `Api` and adds no auth or token-scope hint.
300+
pub fn user_message(&self) -> String {
301+
match self {
302+
ApiError::Status { status, body } if body.trim().is_empty() => {
303+
format!("HTTP {status} (empty response body)")
304+
}
305+
ApiError::Status { body, .. } => util::api_error(body.clone()),
306+
ApiError::Transport(msg) => msg.clone(),
307+
}
308+
}
309+
288310
/// Print the standard error (without exiting), reproducing
289311
/// `ApiClient::fail_response`'s formatting.
290312
///

‎src/commands/databases.rs‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1932,7 +1932,10 @@ fn attach_connection(
19321932
alias: Option<&str>,
19331933
) -> Result<String, String> {
19341934
let connection_id = crate::commands::connections::try_resolve_connection_id(api, connection)?;
1935-
send_attach(api, database_id, connection_id.clone(), alias).map_err(|e| e.message())?;
1935+
// `user_message`, not `message`: this string is interpolated straight into
1936+
// a `warning:` line the user reads, so it wants the sentence out of the
1937+
// error envelope rather than the raw JSON body.
1938+
send_attach(api, database_id, connection_id.clone(), alias).map_err(|e| e.user_message())?;
19361939
Ok(connection_id)
19371940
}
19381941

@@ -4477,4 +4480,46 @@ mod tests {
44774480
other => panic!("expected the 500 to propagate, got {other:?}"),
44784481
}
44794482
}
4483+
4484+
/// `create --attach` folds a failed attach into a `warning:` line and keeps
4485+
/// going. That line is read by a person, so it must carry the server's
4486+
/// sentence — not the JSON envelope carrying it.
4487+
#[test]
4488+
fn failed_attach_reports_the_servers_sentence_not_its_envelope() {
4489+
let mut server = mockito::Server::new();
4490+
// Resolution succeeds: a `conn`-prefixed argument is probed as a raw id
4491+
// first, so this never reaches the listing or the active-database path.
4492+
let probe = server
4493+
.mock("GET", "/v1/connections/conn_src")
4494+
.with_status(200)
4495+
.with_header("content-type", "application/json")
4496+
.with_body(
4497+
r#"{"id":"conn_src","name":"src","source_type":"managed",
4498+
"synced_table_count":0,"table_count":0}"#,
4499+
)
4500+
.create();
4501+
let attach = server
4502+
.mock("POST", "/v1/databases/db_dst/catalogs")
4503+
.with_status(409)
4504+
.with_header("content-type", "application/json")
4505+
.with_body(
4506+
r#"{"error":{"message":"this catalog is scoped to another database and cannot be attached here","code":"CONFLICT"}}"#,
4507+
)
4508+
.create();
4509+
4510+
let api = Api::test_new(&server.url(), "k", Some("ws"));
4511+
let err = attach_connection(&api, "db_dst", "conn_src", None)
4512+
.expect_err("the 409 must surface as an Err the caller can warn with");
4513+
4514+
assert_eq!(
4515+
err, "this catalog is scoped to another database and cannot be attached here",
4516+
"the warning should read as a sentence, not a JSON body"
4517+
);
4518+
assert!(
4519+
!err.contains('{') && !err.contains("CONFLICT"),
4520+
"no envelope punctuation or error code should reach the user; got: {err}"
4521+
);
4522+
probe.assert();
4523+
attach.assert();
4524+
}
44804525
}

0 commit comments

Comments
 (0)