search

martinthomson / ohttp

Rust library for encapsulating HTTP messages in a cryptographic wrapper
Apache License 2.0
30 stars 23 forks source link

bhttp 0.5.2 contains a breaking change for MSRV 1.63.0 #69

Open DanGould opened 4 months ago

DanGould commented 4 months ago

https://github.com/martinthomson/ohttp/commit/d5543a390b00327108aaf3a9157e9125872a5ada included in bhttp 0.5.2 has bhttp::ControlData::status() return the new StatusCode type. Previously StatusCode was an alias for u16, and now you need to call StatusCode::code() to get the u16 out of it.

Since this was a patch bump this broke CI and broke builds downstream without pinning the old dependency or updating the source to call StatusCode::code(). So this change will break my old releases for MSRV 1.63.0 without pinning.

I'm not sure why exactly newer versions can do the conversion and the older version can't. Perhaps newer versions of rust are smart enough to deref the inner value but 1.63 is not?

Not a huge deal since this is v0.* software. I figured you'd like to know anyhow. Thanks for maintaining ohttp & bhttp.

DanGould commented 4 months ago

Another odd break is that bhttp::Message::response(status: StatusCode) used to be callable downstream because StatusCode was just a u16 alias, but now bhttp::StatusCode can only be created by calling try_from(value: u64) since its inner u16 is a private tuple member. This seems odd to me. Why u64?

bhttp::Message::response(bhttp::StatusCode::try_from(parts.status.as_u16() as u64)?) is a very odd way to ohttp encapsulate an response made with hyper. Especially compared to the previous bhttp::Message::response(parts.status.as_u16()). Perhaps you intended the library to be used some other way?

martinthomson commented 4 months ago

Ugh, my bad. I missed that I had those changes staged. It should have been an 0.6 release for the change though.

The intent was to make StatusCode safe by construction, so that you could construct correct responses with more direct feedback. That should have included a TryFrom<u16> implementation at a minimum (the TryFrom<u64> is an implementation detail, not something that should have been exposed like that.

There will be changes for you, which I guess are unavoidable now in any case (sorry again). You'll need to call Message::response(StatusCode::try_from(parts.status.as_u16())?). With future changes (see #67), that might reduce to something nicer, like Message::response(response.status()) if we teach this code to understand hyper status codes. That's a future project though.

For now, I'm adding the TryFrom<u16> and impl From<StatusCode> for u16, which should smooth the bumps a little better.

DanGould commented 4 months ago

In the future if a patch break is found it'd be nice to have that yanked and replaced with a minor version bump. But like I said, not a huge deal. Sounds like you intended an 0.6 release soon enough.