WIP: Add json (serde) support #1081
Conversation
There was a problem hiding this comment.
This is impressive work and this feature is on the project roadmap (#624). However this PR is way too big to review. I did scroll through the code and ask some questions that came to mind.
Can you split this work into smaller PRs?
| pub trait Sealed {} | ||
| } | ||
|
|
||
| pub trait AnyValue: CoreAny + Message + private::Sealed { |
There was a problem hiding this comment.
Can you explain what any v2 is and why it is better?
There was a problem hiding this comment.
This basically tries to address the "improved Any" bulletpoint in #624. The new Any implementation provides support for JSON and "cross" serialization (JSON <-> Binary).
The summary for that change is basically going from:
struct OldAny {
type_path: String,
data: Vec<u8>,
}to
struct NewAny {
type_path: String,
kind: enum {
Protobuf(Vec<u8>),
Json(JsonValue),
Dyn(Box<dyn AnyValue>),
}
}
Yeah, it's actually on the TODO list. I just still haven't gotten to actually complete this as a whole yet. |
| @@ -172,6 +173,32 @@ impl Field { | |||
| _ => None, | |||
| } | |||
| } | |||
|
|
|||
| pub fn json(&self) -> Option<Option<&Json>> { | |||
There was a problem hiding this comment.
Wouldn't it make more sense to flatten this into Option<&Json>
There was a problem hiding this comment.
Conceptionally yes, but this is an actual tri-state kind of thing, because (iirc) we are skipping code generation for Self::OneOf fields. But we could also make this more flat and just add another check for an oneof field at the callsite 🤷♂️.
| @@ -107,7 +111,7 @@ impl fmt::Display for Duration { | |||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
| let mut d = *self; | |||
| d.normalize(); | |||
| if self.seconds < 0 && self.nanos < 0 { | |||
| if self.seconds < 0 && self.nanos <= 0 || self.seconds <= 0 && self.nanos < 0 { | |||
There was a problem hiding this comment.
This seems like an unrelated fix that can ship on its own.
I started this work because I'm also working on adding Connect RPC support to tonic. So having proper protobuf json support is the first step into achieving that goal.
This (unfortunately very big) PR adds json/serde support to prost. This is still a WIP effort but it is almost feature complete and passes all conformance tests except tests related to fieldmasks.
A non-exhaustive list of TODOs:
AnyimplementationSo I am just putting this up here to get a general vibe check...