http
http copied to clipboard
hyperium
Add serde support for StatusCode
This adds a feature serde that for now just enables serialization and deserialization of StatusCode.
Other types can easily be added later based on this.
Resolves #273.
I'm OK with adding serde support via feature flag.
Thoughts @seanmonstar?
@carllerche Is this going to get merged eventually? I would also like to see Serialize and Deserialize implemented on Uri as well. I can open a follow-up PR for that once this gets merged.
My personal feelings are that I'd prefer to not add a public dependency to another crate here.
- We want
httpto be as stable as possible, since we want to essentially be glue for the ecosystem. Any breaking-changes releases that have to be made can split the ecosystem, which Very Bad. - It's straight-forward to use
#[serde(with = "http_serde")]or similar.
I realize my feelings are extremely conservative here, but the http crate feels fundmental enough to maybe deserve it.
@seanmonstar That seems like a reasonable compromise to me, personally. The url crate already does something similar with url_serde.
Unfortunately, there are limitations to that if your field is not directly a StatusCode but for example an Option<StatusCode>.
To my knowledge, you then have to fallback to wrapping StatusCode in a crate-local type again which is a bit annoying :(
While I understand your reasoning very well, I have the feeling that serde is quite safe to depend on in regards to stability.
For our Lambda runtime, requests from Application Load Balancers and API Gateway come in as JSON blobs (an example, if you're curious). Some feature along these lines would be nice, but I'm not entirely sure how to tackle this correctly, especially whenever Lambda supports non-UTF8 formats.
For the record, I lightly disagree w/ @seanmonstar in that I consider the risk fairly low if the feature flag is not enabled by default. First, it would be on serde to maintain backwards compat, and I believe they do a pretty good job. Second, if they release a 2.0, then we can add a serde2 feature flag.
That said, I also see sean's point in that the bar for http to add external deps as part of the public API should be very high. So, I'm not going to push back strongly against it.
For the record, I lightly disagree w/ @seanmonstar in that I consider the risk fairly low if the feature flag is not enabled by default. First, it would be on serde to maintain backwards compat, and I believe they do a pretty good job. Second, if they release a 2.0, then we can add a
serde2feature flag.
I think those conditions are reasonable.
I don't feel strongly about the feature-flag approach.
It just that I couldn't find a different way for enabling (de-)serialization that also works in more complex cases. As already mentioned in my last response, it gets tricky once StatusCode is not the primary type that is (de)-serialized but a type argument as in Option<StatusCode>, Result<StatusCode, ...> or Vec<StatusCode>.
Approaches like #[serde(with = "...")] fail here unless a http_serde crate provides a module for common container types like Option and Result alongside one that directly (de-)serializes a StatusCode.
This is because of the differences in the method signatures: serialize_with needs a function with signature fn<S>(&T, S) -> Result<S::Ok, S::Error> where S: Serializer but if the field is a Option<StatusCode>, T would have to be Option<StatusCode> instead of just StatusCode. As a result, you would need one pair of serialize and deserialize functions per different T.
On the contrary, if StatusCode just implements Serialize and Deserialize, container types like Option and Result just work.
But maybe this is just a problem of serde not exposing a way of applying configurations like #[serde(with = "...")] to type arguments of containers like Option?
@carllerche @seanmonstar what are your final thoughts on this? It would be good to have a decision here in order to start working on alternative solutions, like an http_serde crate, even though it is more work to cover all the cases with the most common container types from std.
As part of the next breaking release https://github.com/hyperium/http/issues/312, we can make use of the new feature in cargo to name this optional feature serde1. I'd be comfortable with that.
I am not sure if I understand 100%.
Do you want me to update the PR so that the feature is named serde1 and then it is good to be merged?
That would be exciting news :)
@thomaseizinger At the risk of misspeaking on Sean's behalf, I think renaming this feature to be serde1 is sufficient. I think other types in this crate can be handled in subsequent PRs.
I renamed the feature, added tests and enabled them on Travis aswell. Let me know if there is more to be done (roundtrip test all of the status codes?)
With newer versions of Cargo, it's possible to rename the crate in the Cargo.toml, like so:
serde1 = { package = "serde", .. }
We can start a 0.2 branch to start merging these breaking changes.
With newer versions of Cargo, it's possible to rename the crate in the
Cargo.toml, like so:serde1 = { package = "serde", .. }We can start a 0.2 branch to start merging these breaking changes.
Ah yeah, I remember! Good idea, that gets rid of the weird feature aliasing. I didn't like that one anyway :D
Unfortunately, that doesn't work because serde_derive seems to expect the crate to be imported under a specific name: serde:
I think if you use at least serde v1.0.90 it should be possible using to the new #[serde(crate = "...")] attribute.
I've created a 0.2.x branch that can include breaking changes (like increasing the minimum Rust version).
I think if you use at least serde v1.0.90 it should be possible using to the new
#[serde(crate = "...")]attribute.
Unfortunately, the whole "rename" idea doesn't quite work. I would have to rename the test dependency as-well, which would mean changing all imports of extern crate serde in the doc tests. I'd consider that to be rather undesired because it can cause quite some confusion for users.
@seanmonstar What is your preference?
It seems like you'd want StatusCode to serialize and deserialize directly from and to the raw number anyway, so it seems like a manual implementation would be easier and avoid these issues.
Yea, I think skipping the derive is best for several reasons:
- The rename is problematic.
- Removes syn dependency, which is heavy.
- The derived Deserialize would allow for any u16, but that's not correct. It should go through
from_u16.
The rename is problematic.
Are you saying we want no renaming at all, i.e. the feature should be named serde?
Manually implementing Serialize and Deserialize sounds like a good idea!
Regarding the tests:
Do we want full roundtrip tests for all codes or are the current tests sufficient? Do we even need tests if the only delegate through other than assertions that Serialize and Deserialize are implemented?
Are you saying we want no renaming at all, i.e. the feature should be named serde?
No, we definitely want the feature to be serde1, no optional serde.
We could live without the tests (dropping another dependency \o/).
No, we definitely want the feature to be serde1, no optional serde.
So through using the cargo rename feature right? I am not my computer right now but I think that forces us to rename it in the [devDependencies] section aswell (the Rust compiler complains otherwise that the same crate cannot be dependent on through two different names).
Since doc-tests make use of [devDependencies] this means we will have to rename all extern crate serde in the doc-tests to extern crate serde1. That is a bit odd since it will also show up here for example.
We could live without the tests (dropping another dependency \o/).
Are you referring to serde_json? That is unfortunately used in the doc-tests and hence can't be removed :(
Bleck, I'd personally opt for us just remove those examples from the docs then.
Bleck, I'd personally opt for us just remove those examples from the docs then.
I'll give it a shot and see what the impact is.