Response method body_string may use wrong character encoding

[Hawk777]

The body_string method in the Response class is as follows:

    pub async fn body_string(&mut self) -> Result<String, Exception> {
        let bytes = self.body_bytes().await?;
        Ok(String::from_utf8(bytes).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?)
    }

This just grabs the body as a byte array, and then decodes it with UTF-8. If it wasn’t originally encoded in UTF-8, this is wrong. What I think most people would expect body_string to do (I certainly did) is extract the character encoding from the charset parameter of the Content-Type header and decode the bytes to a string using that encoding.

While there is a note that “If the body cannot be interpreted as valid UTF-8, an Err is returned.” I found this confusing. I didn’t think it meant that the declared encoding was ignored, I thought it actually meant if the body could not be represented in UTF-8 (perhaps because it was binary data not matching any, or the declared, encoding). I am not sure whether this method, as currently written, is really all that useful? I found it rather misleading myself.

[yoshuawuyts]

@Hawk777 Thanks for filing this; I hear you.

I think I built it this way because Rust strings are always guaranteed to be valid utf8. But if the encoding of the incoming request is different we should probably convert it first. Does that sound like it would be the right way to go?

Additionally we should probably indeed update the docs with more details of what we're doing. I hadn't really thought of it, but now that you mention it it's not surprising it can be confusing to people. Apologies!

[Hawk777]

Yes, that is exactly what I would expect. It might also be worth deciding and documenting what the behaviour is (panic? return error?) if the response body is not actually text (e.g. application/octet-stream or some other non-textual type), though perhaps that could just fall under the “return error if the encoding is invalid” rule, and rely on callers to not call this function for binary content?

[goto-bus-stop]

I think an Err() is the correct way to go if the encoding listed in the Content-Type header is unsupported or is incorrect. In that case the Err() should ideally (imo) contain the byte array so you can decide to do something else, like passing it around as a byte array for octet-stream, or using a crate like chardet to try to fall back an encoding that does work.

Use Content-Type or fall back example

Could be something like:

let response = surf::get("https://some.website/whatever").await?;
let body = match response.body_string().await {
    Ok(body) => body,
    Err(BodyStringError::DecodeError(bytes)) => try_detect_decode(bytes)?,
    Err(err) => Err(err)?,
};

do_something_with(body);

fn try_detect_decode(bytes: Vec<u8>) -> Result<String, SomeError> {
    let (encoding_name, _confidence) = chardet::detect(&bytes);
    if let Some(encoding) = encoding_rs::Encoding::for_label(encoding_name) {
        let (decoded, _used_encoding, failed) = encoding.decode(&bytes);
        if failed {
            Err("could not decode body")
        } else {
            Ok(decoded.to_string())
        }
    } else {
        return Err("could not detect encoding")
    }
}

encoding_rs is very nice—perhaps it could be an optional dependency, so folks talking to an HTTP API that they know will return utf8 do not need to ship its conversion tables.

[goto-bus-stop]

Should be addressed by #108! It tries to use the encoding from the content-type and returns an Err() if that doesn't work.