feat: `Name` trait + `Any` encoding support #896

tarcieri · 2023-08-14T16:48:25Z

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.

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.

LucioFranco · 2023-08-15T14:07:02Z

@tarcieri thanks for putting this together. Just curious if you've tried to use this with any other protobuf language impls to see if the type_url matching is supported? From what I remember the type url stuff always felt a bit fuzzy and felt like a google internal impl detail that was leaked.

prost-types/src/lib.rs

tarcieri · 2023-08-15T15:42:43Z

@LucioFranco I'm trying to interop with a Go reference implementation which makes pretty extensive use of Any, though that said, I am also trying to figure out which things are standard conventions and which are idioms of the particular implementation I'm trying to interop with, as we observed in #858.

That said you did make me take a second look and I did notice something a bit problematic in the type URL comparison. I'm not sure how other implementations handle the leading domain component of a type URL, i.e. if it mismatches, should the full type URL mismatch. Should type URLs with leading / be treated as "relative"?

All that said, I deliberately made type_url a provided method so it implements a convention but can be overridden if desired.

We might also consider an associated constant for the domain component, which could be blank.

LucioFranco · 2023-08-25T18:24:22Z

src/name.rs

+use alloc::{format, string::String};
+
+/// Associate a type name with a [`Message`] type.
+pub trait Name: Message {


Can this just live in prost_types, why does it need to live in the core crate?

I was hoping it could eventually be autogenerated by prost-build, at least optionally. We're hand annotating them right now and it's getting quite cumbersome.

It seems like prost-build currently only refers to types/traits in prost. If it were optional and it could refer to types/traits in prost-types, I'd be fine with the Name trait living there.

I see your point, do we just want to go ahead and do the work to also add the prost-build parts? The next release will be breaking and I would be ok if we add something like that.

LucioFranco · 2023-08-25T18:25:10Z

@tarcieri this looks good besides my one question, I am okay with merging mostly because a lot of this code can be kept outside the core crate.

journaux · 2023-08-25T21:55:10Z

nice !!!

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.

tarcieri · 2023-08-25T23:21:48Z

Build failure seems unrelated:

https://github.com/tokio-rs/prost/actions/runs/5981227819/job/16228748609?pr=896#step:7:112

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
Error: Process completed with exit code 101.

Edit: pushed 46e73e0 to address the test MSRV

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

tarcieri · 2023-08-26T13:38:23Z

Regardless of what crate Name ends up living in, I think it'd also be good to impl this trait for the Message types in prost-types.

That would also make it possible to test Any::{from_msg, to_msg}.

Edit: went ahead and did this in cfe8a99

Also adds tests for `Any::{from_msg, to_msg}`.

tarcieri · 2023-08-27T17:46:41Z

Aside from the question of where the Name trait should live, this should be ready to merge

tarcieri · 2023-08-27T17:47:24Z

src/name.rs

+    /// Type URL for this message, which by default is the full name with a
+    /// leading slash, but may also include a leading domain name, e.g.
+    /// `type.googleapis.com/google.profile.Person`.


Perhaps this could include some of the documentation I added to the internal TypeUrl type.

tarcieri · 2023-08-27T17:48:57Z

src/name.rs

+    /// leading slash, but may also include a leading domain name, e.g.
+    /// `type.googleapis.com/google.profile.Person`.
+    fn type_url() -> String {
+        format!("/{}", Self::full_name())


This could potentially benefit from an easier way to customize the URL authority (i.e. hostname+port). It was a bit tricky to do this for the "well known" types:

https://github.com/tokio-rs/prost/pull/896/files#diff-fe4eba36fffb306e25d27bf29ddea9cd1634ea4ad86705045c1cb13a3aa27346R536-R540

LucioFranco · 2023-08-28T14:17:31Z

src/name.rs

+
+    /// Full name of this message type containing both the package name and
+    /// type name, e.g. `google.protobuf.TypeName`.
+    fn full_name() -> String {


Should these be &'static str, is there a case where these are not statically defined with the generated proto?

If prost-build filled them in they potentially could be all computed at build-time.

Otherwise it's not really possible to construct a &'static str as a concatenation of Self::PACKAGE + '.' + Self::NAME AFAIK.

LucioFranco · 2023-08-28T14:18:35Z

@tarcieri sorry for dragging this a long, would you be interested in adding the prost-build part too then we can keep the trait in prost. I merged the msrv fix in another PR so would be good to back that out of this one. We could also merge this part now (once the msrv is backed out and this is rebased) and then we can add the prost-build stuff in a follow up.

tarcieri · 2023-08-28T14:25:50Z

I can attempt to add the prost-build part, although I'm not sure how difficult that will be. Hopefully it has all of the needed information a priori

LucioFranco · 2023-08-29T13:53:34Z

@tarcieri ok cool, let me know if I can help (feel free to ping on discord). I think I want to wait on this for the next (breaking) release which will basically happen after we get this merged.

LucioFranco · 2023-09-01T14:07:54Z

I'd like to get things merged and released today so I will go ahead and get this merged and figure out what else we need today and then we can add more stuff in point releases since they shouldn't be breaking changes.

sillykelvin · 2023-09-19T14:00:52Z

Sorry to disturb, is there any progress on the prost-build part? Currently we are writing impl prost::Name for Foo { ... } manually, however it's awful and error-prone, it would be great if we can do this with prost-build.

LucioFranco · 2023-09-25T15:38:34Z

@tarcieri were you able to follow up on this? If not if anyone else is interested in picking this up?

tarcieri · 2023-09-25T15:39:43Z

Not yet, and I'm on vacation. Would be great if someone else could pick it up.

LucioFranco · 2023-09-25T15:43:15Z

@tarcieri enjoy! Sounds good!

Well if someone wants to pick up this work I opened #926 and we can continue discussing there.

The `Name` trait added upstream in `prost` can be used to compute the type URL using `Name::type_url`: tokio-rs/prost#896 The new `Any::{from_msg, to_msg}` methods can be used to serialize `Message` types to/from messages. This commit switches from the `TypeUrl` trait to `Name` and relies more on upstream functionality in `prost`. Unfortunately it shipped with a bug in the default `Name::full_name` method, so a workaround is temporarily used until a fix ships: tokio-rs/prost#923

This was referenced Aug 14, 2023

feat: TypeUrl trait + Any encoding support #858

Closed

Serializing a Message to Any #299

Open

tony-iqlusion mentioned this pull request Aug 14, 2023

Add additional derive macros to Prost generated types cosmos/cosmos-rust#409

Closed

tarcieri commented Aug 15, 2023

View reviewed changes

prost-types/src/lib.rs Outdated Show resolved Hide resolved

LucioFranco reviewed Aug 25, 2023

View reviewed changes

tarcieri added 2 commits August 25, 2023 17:10

Merge branch 'master' into name-trait

c41a155

CI: bump test toolchain to 1.64

46e73e0

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

tarcieri added 2 commits August 26, 2023 09:57

Add Name impls for well-known protobuf types

cfe8a99

Also adds tests for `Any::{from_msg, to_msg}`.

Fix no_std

fe92a0f

tarcieri force-pushed the name-trait branch from 53dbdd0 to fe92a0f Compare August 26, 2023 16:03

tarcieri commented Aug 27, 2023

View reviewed changes

LucioFranco reviewed Aug 28, 2023

View reviewed changes

Merge branch 'master' into name-trait

b783481

LucioFranco approved these changes Sep 1, 2023

View reviewed changes

LucioFranco merged commit 7ce9b97 into tokio-rs:master Sep 1, 2023

tarcieri deleted the name-trait branch September 1, 2023 14:42

LucioFranco mentioned this pull request Sep 25, 2023

Follow up, implement Name trait implementation via prost-build #926

Closed

tony-iqlusion mentioned this pull request Oct 3, 2023

cosmos-sdk-proto: replace TypeUrl with Name trait cosmos/cosmos-rust#432

Merged

tinrab mentioned this pull request Oct 11, 2023

feat: Generate implementation for Name trait #931

Merged

Missjones2 approved these changes Oct 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `Name` trait + `Any` encoding support #896

feat: `Name` trait + `Any` encoding support #896

tarcieri commented Aug 14, 2023

LucioFranco commented Aug 15, 2023

tarcieri commented Aug 15, 2023 •

edited

Loading

LucioFranco Aug 25, 2023

tarcieri Aug 25, 2023

LucioFranco Aug 28, 2023

LucioFranco commented Aug 25, 2023

journaux commented Aug 25, 2023

tarcieri commented Aug 25, 2023 •

edited

Loading

tarcieri commented Aug 26, 2023 •

edited

Loading

tarcieri commented Aug 27, 2023

tarcieri Aug 27, 2023

tarcieri Aug 27, 2023

LucioFranco Aug 28, 2023

tarcieri Aug 28, 2023

LucioFranco commented Aug 28, 2023

tarcieri commented Aug 28, 2023

LucioFranco commented Aug 29, 2023

LucioFranco commented Sep 1, 2023

sillykelvin commented Sep 19, 2023

LucioFranco commented Sep 25, 2023

tarcieri commented Sep 25, 2023

LucioFranco commented Sep 25, 2023

feat: Name trait + Any encoding support #896

feat: Name trait + Any encoding support #896

Conversation

tarcieri commented Aug 14, 2023

LucioFranco commented Aug 15, 2023

tarcieri commented Aug 15, 2023 • edited Loading

LucioFranco Aug 25, 2023

Choose a reason for hiding this comment

tarcieri Aug 25, 2023

Choose a reason for hiding this comment

LucioFranco Aug 28, 2023

Choose a reason for hiding this comment

LucioFranco commented Aug 25, 2023

journaux commented Aug 25, 2023

tarcieri commented Aug 25, 2023 • edited Loading

tarcieri commented Aug 26, 2023 • edited Loading

tarcieri commented Aug 27, 2023

tarcieri Aug 27, 2023

Choose a reason for hiding this comment

tarcieri Aug 27, 2023

Choose a reason for hiding this comment

LucioFranco Aug 28, 2023

Choose a reason for hiding this comment

tarcieri Aug 28, 2023

Choose a reason for hiding this comment

LucioFranco commented Aug 28, 2023

tarcieri commented Aug 28, 2023

LucioFranco commented Aug 29, 2023

LucioFranco commented Sep 1, 2023

sillykelvin commented Sep 19, 2023

LucioFranco commented Sep 25, 2023

tarcieri commented Sep 25, 2023

LucioFranco commented Sep 25, 2023

feat: `Name` trait + `Any` encoding support #896

feat: `Name` trait + `Any` encoding support #896

tarcieri commented Aug 15, 2023 •

edited

Loading

tarcieri commented Aug 25, 2023 •

edited

Loading

tarcieri commented Aug 26, 2023 •

edited

Loading