search

http-rs / http-types

Common types for HTTP operations
https://docs.rs/http-types
Apache License 2.0
200 stars 84 forks source link

Request: allow Deserialize<'de> for .query() #333

Closed Fishrock123 closed 3 years ago

Fishrock123 commented 3 years ago

Allows us to avoid extra allocations from querystring parsing.

I don't think this is breaking? But maybe I don't understand the trait inheritance here correctly.

Will also necessitate similar changes in Tide & Surf.

yoshuawuyts commented 3 years ago

I remember trying this in the early days of surf, and not being able to allocate created all sorts of gnarly lifetime issues. I'm not against this change, but I'd like to see this paired with some extra tests to ensure it in fact does cover all scenarios we want to cover. The risk is that we make this change, and people now need to work around our serde methods because they now only cover a subset of uses they did before.

Fishrock123 commented 3 years ago

I don't follow. As far as I am aware DeserializeOwned is more restrictive than Deserialize<'de>...?

There is an existing owned test case with deserializing into a hashmap of Strings. This adds a borrowing test case. Owned deserializes still work just fine.

Fishrock123 commented 3 years ago

(See also: https://serde.rs/lifetimes.html)

Fishrock123 commented 3 years ago

From the aforelinked docs, particularly that last line (emphasis mine):

The Deserialize<'de> lifetime

This lifetime records the constraints on how long data borrowed by this type must be valid.

Every lifetime of data borrowed by this type must be a bound on the 'de lifetime of its Deserialize impl. If this type borrows data with lifetime 'a, then 'de must be constrained to outlive 'a.

If this type does not borrow any data from the Deserializer, there are simply no bounds on the 'de lifetime. Such types automatically implement the DeserializeOwned trait.

Fishrock123 commented 3 years ago

All signs point to this being fine. Merging.