Json: add from_bytes method, use that in 'impl FromRequest'

[waynr]

I find this necessary in order to both deserialize the body of my service's
request into a Json<T> AND preserve the exact byte representation of the
input. The risk of not taking this approach for me is that the spec I am
implementing expects the exact byte representation to be preserved in order to
later serve the exact same content back to the client without a potentially
lossy serialized-deserialized-serialized transformation.

I could almost certainly deserialize these request bodies without the Json<T>
but I really would like to take advantage of the error reporting it offers.

Signed-off-by: wayne warren [email protected]

Motivation

Solution

[waynr]

Hmm, it just occurred to me that an alternative to achieve what I'm aiming for here would be to first parse the Request into Bytes, clone it, then reconstruct a new Request from the cloned Bytes while preserving the original as the canonical representation that gets passed through to the storage layer of my application.

However, this would come with two problems:

• it would potentially increase the memory usage of my route handler unless I can construct a Request<&Bytes> for the sake of extracting a Json<MyType>, but the FromRequest trait's from_request method has a second parameter that i don't understand from reading the docstrings -- the state: &'life0 S
• it seems unnecessarily complicated -- i would have to duplicate the headers from the original request some how to add them to the new one (well, the Content-Type anyway).

[jplatte]

A few notes:

• Cloning Bytes does not do a deep copy, it just increases a refcount (see its documentation)
• serde_json's RawValue type could also be used to implement what you want today, but with more memory usage (deserialize to Json<Box<RawValue>> first, then deserialize raw_value.get() bytes into whatever you want to inspect its contents)
• If you want to echo back the original JSON as part of a larger JSON document, &RawValue (can be obtained from &Bytes through serde_json::from_slice) and axum_extra::ErasedJson (to produce the response from borrowed data) are your friends

[davidpdrsn]

I'm not sure but I kinda feel like we should remove this paragraph. I worry it might be confusing or potentially misleading.

[waynr]

Okay, fair enough.

[waynr]

@jplatte thanks for the info re: Bytes, I didn't realize it was a smart pointer (obviously didn't look closely at its docs). I'll also think on the &RawValue suggestion, but I think as long as this gets merged and cherry-picked into the v0.6.x branch I should be good.

[waynr]

Also, I'm curious what maintainers think about potentially making a FromRequest impl that's generic across TryFrom<&Bytes>? If that were possible then Json<T> shouldn't need its own explicit FromRequest impl. It happens not to be possible for Json<T> because of the existence of impl From<T> for Json<T> (ie i found that impl TryFrom<Bytes> for Json<T> conflicts with the more general impl when i initially tried it) and rust's lack of support for specialization so the impl From<T> for Json<T> would have to be removed and considered a breaking change so not suitable for v0.6.x.

[davidpdrsn]

What’s the advantage to having such a generic implementation? Do you have a concrete use case in mind where you need to be generic over multiple types?

My initial reaction is that we probably don’t need it and that it might make things harder.

[waynr]

What’s the advantage to having such a generic implementation? Do you have a concrete use case in mind where you need to be generic over multiple types?

Is there anything other than Json<T> that implements FromRequest whose internal implementation detail first involves collecting the request into Bytes? I guess I assumed there would be more but the only other one I can find is here: https://docs.rs/axum/0.6.20/src/axum/form.rs.html#77-98

And it probably makes less sense to preserve the byte representation of a Form than it does a Json body.

My thinking was that making a FromRequest impl that's generic over a type that implements From<Bytes> would encourage other types to implement From<Bytes> for use cases like mine.

[waynr]

I definitely get the "that sounds too complicated to maintain" aspect so I'm not trying to push for it, just suggesting with no vested interest in getting a "yes" out of you :)

[davidpdrsn]

Is there anything other than Json that implements FromRequest whose internal implementation detail first involves collecting the request into Bytes?

Yaml, protobuf, string, or basically any other format would require that.

I don’t think we should add any generic impls like that without a concrete use case. It makes it easy to paint ourselves into a corner with overlapping impls. More generics aren’t always better.

[jplatte]

One more nitpick, otherwise LGTM (but will let David decide when to merge)

[waynr]

Thanks for getting to this so quickly! Is this something y'all are willing to merge into the v0.6.x branch also? If so, I can open a quick PR with the contents of this PR squashed and cherry-picked against it.

[davidpdrsn]

Looks good!

Wanna add it to the changelog as well?

[waynr]

@davidpdrsn hey, thanks for the review! Any idea when I can expect this to be included in a release?

[davidpdrsn]

I’ll take another look at this week. Sorry for the delay. Been busy at work and traveling.