Log first 200 bytes of response when MIME type is unexpected #14

Activity

1. HusseinAdeiza commented on Sep 28, 2026

   

   HusseinAdeiza

   on Sep 28, 2026

   More actions

   Taking this, since @james-callahan flagged it as a good first issue and it's still open on current main.

   The exact gap in client/src/lib.rs::send is that the body is already read into text a few lines before the check, but the UnexpectedMimeType arm only carries the mime type:

   if content_type != mime::APPLICATION_JSON {
       return Err(TurnkeyClientError::UnexpectedMimeType(
           content_type.to_string(),
       ));
   }

   So a captive portal returning text/html with "you must be logged in" in the body gives you the mime type and nothing else, which is exactly the case the comment describes.

   Plan: log the first 200 bytes of text at debug when the mime type is unexpected, and widen the error to carry a truncated body alongside the mime type so the test can assert on it. Existing test test_unexpected_mime_type at L788 asserts on the single-field variant, so I'll update it.

   One question before I open it: would you rather the body only be logged (no error-variant change, smaller diff), or carried in the error? Logging alone fixes the debugging problem; carrying it in the error is better for tests and for anyone matching on it, at the cost of a breaking change to a public enum variant.

   I'll go with logging-only unless you prefer the error change, since TurnkeyClientError is public and widening a variant is technically breaking.
2. r-n-o commented on Oct 1, 2026

   

   r-n-o

   on Oct 1, 2026

   ContributorAuthor

   More actions

   I'd rather have a smaller diff, so I'd go for a log-only change (it's indeed useful for debugging but not much else)
3. r-n-o commented on Oct 1, 2026

   

   r-n-o

   on Oct 1, 2026

   ContributorAuthor

   More actions

   Actually scratch that. Carrying the debug info in the error is important for users of the SDK. Otherwise it'll be hard to guarantee visibility and this becomes less useful for debugging.

   The scenario typically would be:

   • customer of Turnkey implements app with Rust SDK, logs errors if they happen
   • user is on a captive portal, uses app
   • they need to see the first 200 bytes of the response to understand they're on a captive portal (as opposed to blaming the app or Turnkey)

   So having the error default serialization display the first 200 bytes is important (typically devs will use that to present errors to users)
4. HusseinAdeiza

   added a commit that references this issue on Oct 1, 2026

   client: carry the response body head on an unexpected MIME type

   ...

   b1665c0
5. HusseinAdeiza commented on Oct 1, 2026

   

   HusseinAdeiza

   on Oct 1, 2026

   More actions

   Agreed, and I reworked #285 to match — the earlier version was log-only.

   UnexpectedMimeType now carries the body head as a second field, so it shows up in the error's Display output and reaches anyone stringifying the error:

   #[error("HTTP response MIME type is not application/json (found {0}); response body head: {1}")]
   UnexpectedMimeType(String, String),

   Two things I checked while reworking it:

   • This is consistent with what the enum already does. UnexpectedHttpStatus(u16, String) and Decode(String, serde_json::Error) both carry the response body today, so this makes the MIME arm match its neighbours rather than introducing a new pattern.
   • Dropping the debug log meant the tracing dep I had added to client/Cargo.toml is no longer needed, so that came back out. No new dependencies.

   The preview is capped at 200 characters and cut on a character boundary, so a large HTML page doesn't flood the message and a multi-byte character is never split. Tests cover the captive-portal case specifically, asserting the body survives both matching on the variant and to_string().

   It is a breaking change to a public variant, as you noted it would be. Flagged in the PR description in case you'd rather I gate it differently.