http-compat: add support for convert between bhttp binary and http crate

[imlk0]

This PR is a re-open of #82

[martinthomson]

Thanks for doing this. And apologies for delays. I've been busy.

I have NOT looked at the decoder. You might find some of the comments from the encoder transfer over (particularly Poll::Pending handling); I'll try to get back to this soon.

[martinthomson]

Suggested change

	/// An HTTP status code.
	/// The framing indicator ([Section 3.3 of RFC 9292](https://datatracker.ietf.org/doc/html/rfc9292#section-3.3))

[martinthomson]

Suggested change

	#[derive(Clone, Copy, Debug)]
	/// An HTTP status code.
	#[derive(Clone, Copy, Debug)]

[martinthomson]

Suggested change

let request = t == 0 || t == 2;

[martinthomson]

Suggested change

	let framing_indicator = if request {
	FramingIndicator::Request(mode)
	} else {
	FramingIndicator::Response(mode)
	};
	Ok(framing_indicator)
	match t {
	0 | 2 => Ok(FramingIndicator::Request(mode)),
	1 | 3 => Ok(FramingIndicator::Response(mode)),
	_ => unreachable!("checked by Mode::try_from()"),
	}

[martinthomson]

Does this work?

Suggested change

	pub fn reader_ref(&self) -> &S {
	pub(crate) fn reader_ref(&self) -> &S {

I say that because accessing a reference to the internals might lead to errors if outside code needs to do that.

[martinthomson]

If you avoid the Option you can extend always.

[martinthomson]

Avoiding the option will simplify more.

[martinthomson]

Consider using HeaderMap::from_iter to remove the mut.

Suggested change

	let mut trailers = HeaderMap::new();
	trailers.insert("Trailer-Key", "Trailer-Value".parse().unwrap());
	let trailers = HeaderMap::from_iter([
	("Trailer-Key", "Trailer-Value".parse().unwrap())
	]);

[martinthomson]

Consider parsing (synchronous is fine), then test for the trailers.

[martinthomson]

Consider parsing (synchronous is fine), then test for the various pieces you encoded..

[martinthomson]

The decoder parts are pretty good. Just a few little nits in that part.

[martinthomson]

I think that you want this:

Suggested change

	if let Some(method) = control_data.method() {
	builder = builder.method(method);
	}
	let Some(method) = control_data.method() else {
	return Err(Error::Something);
	};
	builder = builder.method(method);

All requests have a method.

[martinthomson]

Again, the scheme is mandatory, so you can safely do something like this:

Suggested change

	if let Some(scheme) = control_data.scheme() {
	if !scheme.is_empty() {
	uri_builder = uri_builder.scheme(scheme);
	}
	}
	let Some(scheme) = control_data.scheme() else {
	return Err(Error::Something);
	}
	if scheme.is_empty() {
	return Err(Error::Something);
	}
	uri_builder = uri_builder.scheme(scheme);

[martinthomson]

Suggested change

	uri_builder = uri_builder.authority(std::str::from_utf8(authority_bytes)?);
	uri_builder = uri_builder.authority(str::from_utf8(authority_bytes)?);

[martinthomson]

As above, I think that you can use let Some(auth) = control_data.authority() else { return Err(...); };

One thing that this block fails to catch is the None case.

[martinthomson]

You might want to filter out pseudo-headers here. Most of these are in the control data, but the format allows for others. I don't know if the http crate is OK with field names that start with ':', but it looks like they will produce an error at some level, quite possibly a panic.

[martinthomson]

See below.

[martinthomson]

Suggested change

	std::str::from_utf8(&body_data).unwrap(),
	"Hello World! My content includes a trailing CRLF.\r\n"
	&body_data,
	b"Hello World! My content includes a trailing CRLF.\r\n"

No point converting.

[martinthomson]

As above:

Suggested change

	assert_eq!(
	std::str::from_utf8(&body_data).unwrap(),
	"This content contains CRLF.\r\n"
	);
	assert_eq!(
	&body_data,
	b"This content contains CRLF.\r\n"
	);

[martinthomson]

Not sure why you are pulling the scheme again. That should already be settled.

If your goal is to use host as a backup, I would do this:

let Some(auth) = control_data.authority() else {
    return Err(Error::Something);
};
// Add your comment here.
let authority = if auth.is_empty() {
    let Some(host) = headers.get("host") else {
        return Err(Error::Something);
    };
} else {
    auth;
};
uri_builder.authority(authority.to_str()?);

[martinthomson]

This should use let Some() ... else { return Err(...); }; as well.