-
Notifications
You must be signed in to change notification settings - Fork 525
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: TypeUrl
trait + Any
encoding support
#858
Conversation
@@ -33,6 +35,39 @@ pub use protobuf::*; | |||
const NANOS_PER_SECOND: i32 = 1_000_000_000; | |||
const NANOS_MAX: i32 = NANOS_PER_SECOND - 1; | |||
|
|||
impl Any { | |||
/// Serialize this message proto as [`Any`]. | |||
pub fn from_message<M>(msg: &M) -> Result<Self, EncodeError> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these might benefit from a shorter from_msg
/to_msg
naming, but I went with existing precedent which seems to spell out the whole word message
.
1bde196
to
67ec6da
Compare
As discussed in tokio-rs#299, adds a `TypeUrl` trait which associates a type URL constant with a `Message` type. This enables adding methods to `Any` for decoding/encoding messages: - `Any::from_message`: encodes a given `Message`, returning `Any`. - `Any::to_message`: decodes `Any::value` as the given `Message`, first validating the message type has the expected type URL.
/// ```text | ||
/// /foo.bar.baz.MyTypeName | ||
/// ``` | ||
const TYPE_URL: &'static str; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just went with a &'static str
for now, rather than trying to use a special type which validates the URL.
In my mind the goal is to get prost-build
to spit these out eventually, in which case they should be well-formed. But maybe down the road it would be possible to add a URL type with a const fn
parser.
/// For example: | ||
/// | ||
/// ```text | ||
/// /foo.bar.baz.MyTypeName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the slash is part of the type URL, the type name is foo.bar.baz.MyTypeName
which then gets encoded into the Any message as type.googleapis.com/foo.bar.baz.MyTypeName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess this particular encoding is an artifact of the type URLs I'm working with and not the standard encoding.
I'm still unclear though if all type URLs are expected to be prefixed with https://type.googleapis.com/
or just Google's own well-known types, which is what the example is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is difference between type url and full type name of a protobuf message.
For instance the full type name is not expected to have slashes.
The Type URL in a google.Protobuf.Any
is expected to have at least one slash.
So if this Type URL mentioned here is supposed to end up in an any then by specification, it should contain at least one slash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this TYPE URL we are talking about in this PR is supposed to be used to compose the TYPE URL of an any then we shouldn't be talking about TYPE URL but we should be talking about FULLNAME of a protobuf message, they're different concepts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the type url in Any should have a slash, but the type name that we attach to the trait should just be the full type name without a slash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this trait should not be called TypeURL
it should be called Fullname
.
And the way we match a type URL is not the correct one:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case is populating the type_url
field of an Any
message.
If you're saying the type URL can always be computed from the fullname by prepending a slash, that's fine. That's what the implementation I'm using is expecting.
In addition to a TypeUrl trait, It would be pretty nice to get a |
As discussed in tokio-rs#299 and tokio-rs#858, adds a `Name` trait which associates a type name and package constants with a `Message` type. It also provides `full_name` and `type_url` methods. The `type_url` method is used by newly added methods on the `Any` type which can be used for decoding/encoding messages: - `Any::from_msg`: encodes a given `Message`, returning `Any`. - `Any::to_msg`: decodes `Any::value` as the given `Message`, first validating the message type has the expected type URL.
Based on the feedback from this PR, I've opened #896 which provides similar functionality, but uses a Will go ahead and close this PR out. |
* feat: `Name` trait + `Any` encoding support As discussed in #299 and #858, adds a `Name` trait which associates a type name and package constants with a `Message` type. It also provides `full_name` and `type_url` methods. The `type_url` method is used by newly added methods on the `Any` type which can be used for decoding/encoding messages: - `Any::from_msg`: encodes a given `Message`, returning `Any`. - `Any::to_msg`: decodes `Any::value` as the given `Message`, first validating the message type has the expected type URL. * Add private `TypeUrl` type Implements the basic rules for parsing type URLs as documented in: https://github.com/protocolbuffers/protobuf/blob/a281c13/src/google/protobuf/any.proto#L129C2-L156C50 Notably this extracts the final path segment of the URL which contains the full name of the type, and uses that for type comparisons. * CI: bump test toolchain to 1.64 This is the MSRV of `petgraph` now: error: package `petgraph v0.6.4` cannot be built because it requires rustc 1.64 or newer, while the currently active rustc version is 1.63.0 * Add `Name` impls for well-known protobuf types Also adds tests for `Any::{from_msg, to_msg}`. * Fix no_std --------- Co-authored-by: Lucio Franco <[email protected]>
As discussed in #299, adds a
TypeUrl
trait which associates a type URL constant with aMessage
type.This enables adding methods to
Any
for decoding/encoding messages:Any::from_message
: encodes a givenMessage
, returningAny
.Any::to_message
: decodesAny::value
as the givenMessage
, first validating the message type has the expected type URL.