Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add TryFrom implementations for MetadataValue #990

Merged
merged 2 commits into from May 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/src/authentication/client.rs
Expand Up @@ -9,7 +9,7 @@ use tonic::{metadata::MetadataValue, transport::Channel, Request};
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let channel = Channel::from_static("http://[::1]:50051").connect().await?;

let token = MetadataValue::from_str("Bearer some-auth-token")?;
let token: MetadataValue<_> = "Bearer some-auth-token".parse()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I actually kinda prefer the look of from_str vs using parse. I wonder if we should not deprecate it and keep it. But just have the TryFrom's use it or the other way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. So, MetadataValue impls FromStr, so it will already have a from_str method from stdlib, which is equivalent to parse. Unfortunately it's not in the stdlib prelude, so if I delete the ad-hoc from_str method then old code will fail to compile (although the compiler will suggest the easy fix of "use std::str::FromStr").

It does kinda annoy me to have an adhoc method named from_str when the stdlib one exists too, just because it seems to make the API a bit larger than it needs to be. But this is a very minor gripe :) So overall I'm happy with the solution of not deprecating from_str if that's what you'd like.

(I'm pretty used to parse() because it's used very often in the stdlib IP types).

So yeah, happy to do whichever, just let me know what you think now that you've ready this silly long reply :)


let mut client = EchoClient::with_interceptor(channel, move |mut req: Request<()>| {
req.metadata_mut().insert("authorization", token.clone());
Expand Down
2 changes: 1 addition & 1 deletion examples/src/authentication/server.rs
Expand Up @@ -59,7 +59,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
}

fn check_auth(req: Request<()>) -> Result<Request<()>, Status> {
let token = MetadataValue::from_str("Bearer some-secret-token").unwrap();
let token: MetadataValue<_> = "Bearer some-secret-token".parse().unwrap();

match req.metadata().get("authorization") {
Some(t) if token == t => Ok(req),
Expand Down
2 changes: 1 addition & 1 deletion examples/src/gcp/client.rs
Expand Up @@ -22,7 +22,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
.ok_or_else(|| "Expected a project name as the first argument.".to_string())?;

let bearer_token = format!("Bearer {}", token);
let header_value = MetadataValue::from_str(&bearer_token)?;
let header_value: MetadataValue<_> = bearer_token.parse()?;

let certs = tokio::fs::read("examples/data/gcp/roots.pem").await?;

Expand Down
2 changes: 1 addition & 1 deletion interop/src/client.rs
Expand Up @@ -342,7 +342,7 @@ pub async fn unimplemented_service(

pub async fn custom_metadata(client: &mut TestClient, assertions: &mut Vec<TestAssertion>) {
let key1 = "x-grpc-test-echo-initial";
let value1 = MetadataValue::from_str("test_initial_metadata_value").unwrap();
let value1: MetadataValue<_> = "test_initial_metadata_value".parse().unwrap();
let key2 = "x-grpc-test-echo-trailing-bin";
let value2 = MetadataValue::from_bytes(&[0xab, 0xab, 0xab]);

Expand Down
209 changes: 179 additions & 30 deletions tonic/src/metadata/value.rs
Expand Up @@ -7,6 +7,7 @@ use super::key::MetadataKey;

use bytes::Bytes;
use http::header::HeaderValue;
use std::convert::TryFrom;
use std::error::Error;
use std::hash::{Hash, Hasher};
use std::marker::PhantomData;
Expand Down Expand Up @@ -86,9 +87,6 @@ impl<VE: ValueEncoding> MetadataValue<VE> {
/// For Binary metadata values this method cannot fail. See also the Binary
/// only version of this method `from_bytes`.
///
/// This function is intended to be replaced in the future by a `TryFrom`
/// implementation once the trait is stabilized in std.
///
/// # Examples
///
/// ```
Expand All @@ -105,11 +103,9 @@ impl<VE: ValueEncoding> MetadataValue<VE> {
/// assert!(val.is_err());
/// ```
#[inline]
#[deprecated = "Use TryFrom instead"]
pub fn try_from_bytes(src: &[u8]) -> Result<Self, InvalidMetadataValueBytes> {
VE::from_bytes(src).map(|value| MetadataValue {
inner: value,
phantom: PhantomData,
})
Self::try_from(src)
}

/// Attempt to convert a `Bytes` buffer to a `MetadataValue`.
Expand All @@ -122,15 +118,10 @@ impl<VE: ValueEncoding> MetadataValue<VE> {
/// error is returned. In use cases where the input is not base64 encoded,
/// use `from_bytes`; if the value has to be encoded it's not possible to
/// share the memory anyways.
///
/// This function is intended to be replaced in the future by a `TryFrom`
/// implementation once the trait is stabilized in std.
#[inline]
#[deprecated = "Use TryFrom instead"]
pub fn from_shared(src: Bytes) -> Result<Self, InvalidMetadataValueBytes> {
VE::from_shared(src).map(|value| MetadataValue {
inner: value,
phantom: PhantomData,
})
Self::try_from(src)
}

/// Convert a `Bytes` directly into a `MetadataValue` without validating.
Expand Down Expand Up @@ -282,6 +273,165 @@ impl<VE: ValueEncoding> MetadataValue<VE> {
}
}

/// Attempt to convert a byte slice to a `MetadataValue`.
///
/// For Ascii metadata values, If the argument contains invalid metadata
/// value bytes, an error is returned. Only byte values between 32 and 255
/// (inclusive) are permitted, excluding byte 127 (DEL).
///
/// For Binary metadata values this method cannot fail. See also the Binary
/// only version of this method `from_bytes`.
///
/// # Examples
///
/// ```
/// # use tonic::metadata::*;
/// # use std::convert::TryFrom;
/// let val = AsciiMetadataValue::try_from(b"hello\xfa").unwrap();
/// assert_eq!(val, &b"hello\xfa"[..]);
/// ```
///
/// An invalid value
///
/// ```
/// # use tonic::metadata::*;
/// # use std::convert::TryFrom;
/// let val = AsciiMetadataValue::try_from(b"\n");
/// assert!(val.is_err());
/// ```
impl<'a, VE: ValueEncoding> TryFrom<&'a [u8]> for MetadataValue<VE> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this lifetime be elided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be, but then Rustdoc generates docs with a signature like TryFrom<&'0 str>. If I name the lifetime, then it outputs the signature TryFrom<&'a str> which I think is more readable, more what Rust programmers are used to seeing.

I copied this style of doing it from the hyper docs -- happy to change it to the elided lifetime if you prefer, but I thought matching Hyper's docs is a good tiebreaker when I wasn't sure which was better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool didn't know that, seems fine to me!

type Error = InvalidMetadataValueBytes;

#[inline]
fn try_from(src: &[u8]) -> Result<Self, Self::Error> {
VE::from_bytes(src).map(|value| MetadataValue {
inner: value,
phantom: PhantomData,
})
}
}

/// Attempt to convert a byte slice to a `MetadataValue`.
///
/// For Ascii metadata values, If the argument contains invalid metadata
/// value bytes, an error is returned. Only byte values between 32 and 255
/// (inclusive) are permitted, excluding byte 127 (DEL).
///
/// For Binary metadata values this method cannot fail. See also the Binary
/// only version of this method `from_bytes`.
///
/// # Examples
///
/// ```
/// # use tonic::metadata::*;
/// # use std::convert::TryFrom;
/// let val = AsciiMetadataValue::try_from(b"hello\xfa").unwrap();
/// assert_eq!(val, &b"hello\xfa"[..]);
/// ```
///
/// An invalid value
///
/// ```
/// # use tonic::metadata::*;
/// # use std::convert::TryFrom;
/// let val = AsciiMetadataValue::try_from(b"\n");
/// assert!(val.is_err());
/// ```
impl<'a, VE: ValueEncoding, const N: usize> TryFrom<&'a [u8; N]> for MetadataValue<VE> {
type Error = InvalidMetadataValueBytes;

#[inline]
fn try_from(src: &[u8; N]) -> Result<Self, Self::Error> {
Self::try_from(src.as_ref())
}
}

/// Attempt to convert a `Bytes` buffer to a `MetadataValue`.
///
/// For `MetadataValue<Ascii>`, if the argument contains invalid metadata
/// value bytes, an error is returned. Only byte values between 32 and 255
/// (inclusive) are permitted, excluding byte 127 (DEL).
///
/// For `MetadataValue<Binary>`, if the argument is not valid base64, an
/// error is returned. In use cases where the input is not base64 encoded,
/// use `from_bytes`; if the value has to be encoded it's not possible to
/// share the memory anyways.
impl<VE: ValueEncoding> TryFrom<Bytes> for MetadataValue<VE> {
type Error = InvalidMetadataValueBytes;

#[inline]
fn try_from(src: Bytes) -> Result<Self, Self::Error> {
VE::from_shared(src).map(|value| MetadataValue {
inner: value,
phantom: PhantomData,
})
}
}

/// Attempt to convert a Vec of bytes to a `MetadataValue`.
///
/// For `MetadataValue<Ascii>`, if the argument contains invalid metadata
/// value bytes, an error is returned. Only byte values between 32 and 255
/// (inclusive) are permitted, excluding byte 127 (DEL).
///
/// For `MetadataValue<Binary>`, if the argument is not valid base64, an
/// error is returned. In use cases where the input is not base64 encoded,
/// use `from_bytes`; if the value has to be encoded it's not possible to
/// share the memory anyways.
impl<VE: ValueEncoding> TryFrom<Vec<u8>> for MetadataValue<VE> {
type Error = InvalidMetadataValueBytes;

#[inline]
fn try_from(src: Vec<u8>) -> Result<Self, Self::Error> {
Self::try_from(src.as_slice())
}
}

/// Attempt to convert a string to a `MetadataValue<Ascii>`.
///
/// If the argument contains invalid metadata value characters, an error is
/// returned. Only visible ASCII characters (32-127) are permitted. Use
/// `from_bytes` to create a `MetadataValue` that includes opaque octets
/// (128-255).
impl<'a> TryFrom<&'a str> for MetadataValue<Ascii> {
type Error = InvalidMetadataValue;

#[inline]
fn try_from(s: &'a str) -> Result<Self, Self::Error> {
s.parse()
}
}

/// Attempt to convert a string to a `MetadataValue<Ascii>`.
///
/// If the argument contains invalid metadata value characters, an error is
/// returned. Only visible ASCII characters (32-127) are permitted. Use
/// `from_bytes` to create a `MetadataValue` that includes opaque octets
/// (128-255).
impl<'a> TryFrom<&'a String> for MetadataValue<Ascii> {
type Error = InvalidMetadataValue;

#[inline]
fn try_from(s: &'a String) -> Result<Self, Self::Error> {
s.parse()
}
}

/// Attempt to convert a string to a `MetadataValue<Ascii>`.
///
/// If the argument contains invalid metadata value characters, an error is
/// returned. Only visible ASCII characters (32-127) are permitted. Use
/// `from_bytes` to create a `MetadataValue` that includes opaque octets
/// (128-255).
impl TryFrom<String> for MetadataValue<Ascii> {
type Error = InvalidMetadataValue;

#[inline]
fn try_from(s: String) -> Result<Self, Self::Error> {
s.parse()
}
}

// is_empty is defined in the generic impl block above
#[allow(clippy::len_without_is_empty)]
impl MetadataValue<Ascii> {
Expand All @@ -292,9 +442,6 @@ impl MetadataValue<Ascii> {
/// `from_bytes` to create a `MetadataValue` that includes opaque octets
/// (128-255).
///
/// This function is intended to be replaced in the future by a `TryFrom`
/// implementation once the trait is stabilized in std.
///
/// # Examples
///
/// ```
Expand All @@ -311,14 +458,10 @@ impl MetadataValue<Ascii> {
/// assert!(val.is_err());
/// ```
#[allow(clippy::should_implement_trait)]
#[deprecated = "Use TryFrom or FromStr instead"]
#[inline]
pub fn from_str(src: &str) -> Result<Self, InvalidMetadataValue> {
HeaderValue::from_str(src)
.map(|value| MetadataValue {
inner: value,
phantom: PhantomData,
})
.map_err(|_| InvalidMetadataValue::new())
src.parse()
}

/// Converts a MetadataKey into a MetadataValue<Ascii>.
Expand All @@ -330,8 +473,9 @@ impl MetadataValue<Ascii> {
///
/// ```
/// # use tonic::metadata::*;
/// # use std::convert::TryFrom;
/// let val = AsciiMetadataValue::from_key::<Ascii>("accept".parse().unwrap());
/// assert_eq!(val, AsciiMetadataValue::try_from_bytes(b"accept").unwrap());
/// assert_eq!(val, AsciiMetadataValue::try_from(b"accept").unwrap());
/// ```
#[inline]
pub fn from_key<KeyVE: ValueEncoding>(key: MetadataKey<KeyVE>) -> Self {
Expand Down Expand Up @@ -402,8 +546,8 @@ impl MetadataValue<Binary> {
/// ```
#[inline]
pub fn from_bytes(src: &[u8]) -> Self {
// Only the Ascii version of try_from_bytes can fail.
Self::try_from_bytes(src).unwrap()
// Only the Ascii version of try_from can fail.
Self::try_from(src).unwrap()
}
}

Expand Down Expand Up @@ -501,7 +645,7 @@ mod from_metadata_value_tests {

assert_eq!(
map.get("accept").unwrap(),
AsciiMetadataValue::try_from_bytes(b"hello-world").unwrap()
AsciiMetadataValue::try_from(b"hello-world").unwrap()
);
}
}
Expand All @@ -511,7 +655,12 @@ impl FromStr for MetadataValue<Ascii> {

#[inline]
fn from_str(s: &str) -> Result<MetadataValue<Ascii>, Self::Err> {
MetadataValue::<Ascii>::from_str(s)
HeaderValue::from_str(s)
.map(|value| MetadataValue {
inner: value,
phantom: PhantomData,
})
.map_err(|_| InvalidMetadataValue::new())
}
}

Expand Down Expand Up @@ -730,7 +879,7 @@ fn test_debug() {
];

for &(value, expected) in cases {
let val = AsciiMetadataValue::try_from_bytes(value.as_bytes()).unwrap();
let val = AsciiMetadataValue::try_from(value.as_bytes()).unwrap();
let actual = format!("{:?}", val);
assert_eq!(expected, actual);
}
Expand Down Expand Up @@ -760,7 +909,7 @@ fn test_is_empty() {

#[test]
fn test_from_shared_base64_encodes() {
let value = BinaryMetadataValue::from_shared(Bytes::from_static(b"Hello")).unwrap();
let value = BinaryMetadataValue::try_from(Bytes::from_static(b"Hello")).unwrap();
assert_eq!(value.as_encoded_bytes(), b"SGVsbG8");
}

Expand Down
2 changes: 1 addition & 1 deletion tonic/src/request.rs
Expand Up @@ -285,7 +285,7 @@ impl<T> Request<T> {
///
/// [the spec]: https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md
pub fn set_timeout(&mut self, deadline: Duration) {
let value = MetadataValue::from_str(&duration_to_grpc_timeout(deadline)).unwrap();
let value: MetadataValue<_> = duration_to_grpc_timeout(deadline).parse().unwrap();
self.metadata_mut()
.insert(crate::metadata::GRPC_TIMEOUT_HEADER, value);
}
Expand Down