WIP: Add json (serde) support #1081

cynecx · 2024-06-13T00:46:38Z

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:

Decide whether the whole "custom serde" design is the one we want to persue
Properly document code and public api
Properly put this all behind a feature flag
Probably split things up because this PR also includes a new Any implementation
...

So I am just putting this up here to get a general vibe check...

caspermeijn

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?

tests/src/lib.rs

caspermeijn · 2024-06-13T13:45:38Z

prost-types/src/any_v2.rs

+    pub trait Sealed {}
+}
+
+pub trait AnyValue: CoreAny + Message + private::Sealed {


Can you explain what any v2 is and why it is better?

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>), } }

prost-build/src/code_generator.rs

cynecx · 2024-06-14T05:24:23Z

However this PR is way too big to review.
Can you split this work into smaller PRs?

Yeah, it's actually on the TODO list. I just still haven't gotten to actually complete this as a whole yet.

ehiggs · 2024-06-21T11:38:34Z

prost-derive/src/field/mod.rs

@@ -172,6 +173,32 @@ impl Field {
            _ => None,
        }
    }
+
+    pub fn json(&self) -> Option<Option<&Json>> {


Wouldn't it make more sense to flatten this into Option<&Json>

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 🤷‍♂️.

ehiggs · 2024-06-21T13:41:11Z

prost-types/src/duration.rs

@@ -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 {


This seems like an unrelated fix that can ship on its own.

prost-build/src/code_generator.rs

cblichmann · 2024-10-19T14:51:46Z

Any news on this? This would be incredibly useful.

… workarounds

cynecx · 2024-11-22T22:48:00Z

I've done a quick rebase and made few adjustments to pass the conformance tests again. It also seems like I need to do some tweaks to fix the CI for no-std.

Any news on this? This would be incredibly useful.

I am still way too busy with other things. What this PR needs is a split and a bit of slight documentation work. I will probably get to that at the end of December perhaps. Thanks for your patience!

ehiggs · 2024-11-25T20:20:11Z

prost-derive/Cargo.toml

@@ -14,7 +14,8 @@ proc-macro = true

 [dependencies]
 anyhow = "1.0.1"
-itertools = ">=0.10.1, <=0.13"
+heck = "0.4.1"


0.5 was released 9 months ago. Any reason to use a version from 2 years ago?

Not sure anymore but I guess this PR or at least my first local impl predated this commit 2b6140c.

But yeh, we can certainly bump this.

ehiggs · 2024-11-25T20:41:06Z

prost-types/src/duration.rs

@@ -69,6 +69,10 @@ impl Duration {
        result.normalize();
        result
    }
+
+    pub fn is_valid(&self) -> bool {
+        self.seconds >= -315_576_000_000 && self.seconds <= 315_576_000_000


Sorry if I'm a tourist asking silly questions here, but what does it mean to be a duration consisting of a negative number of seconds?

aiui, Chrono calls it a TimeDelta for this reason

There was a conformance test that checked this behavior... It's basically an api contract / invariant, that is mentioned here: https://github.com/protocolbuffers/protobuf/blob/2ad21af8406316d2b170230f93db7d10c36108dd/src/google/protobuf/duration.proto#L103.

ehiggs · 2024-11-25T21:06:25Z

prost/src/serde/de.rs

+    }
+}
+
+// FIXME: Make `T` contravariant, not covariant, by changing the `T` in `PhantomData` to


looks like this was done since you have PhantomData<fn() -> T>

ehiggs · 2024-11-25T21:23:33Z

@caspermeijn has asked for this to be split into smaller PRs:
#1081 (review)

Reviewing this, I would also expect to see example messages which get extended and validating that before and after changes to the proto file, it works.

e.g.

message TestMessage {
    string name = 1;
}

Then a json payload that satisfies that is then stuck into:

message TestMessage {
    string name = 1;
    int32 age = 2;
}

Or is there an existing test framework and this is using it as another pass with serde enabled and I just don't see how it fits together.

I'm not a committer so take my suggestion with a grain of salt. You've already done an impressive bit of work here!

cynecx · 2024-11-25T22:28:04Z

@caspermeijn has asked for this to be split into smaller PRs:

Yeah, I've mentioned this in my earlier update comment as well. It's definitely on my to-do list! :)

Or is there an existing test framework and this is using it as another pass with serde enabled and I just don't see how it fits together.

I haven't written any specific tests for this yet. That said, I'm not overly concerned because the conformance test suite already exercises this functionality by feeding data (with unknown/missing fields) for decoding/deserialization during our roundtrip conversions. However, having some dedicated tests in our own codebase wouldn't hurt either.

For now, I'll likely prioritize other tasks in the near future. However, I'd love to collaborate on this, if you have any ideas for tests, feel free to share :D

… repr

cynecx · 2024-12-08T00:22:01Z

The most recent commits should address almost all conformance failures. There is just one remaining, but that is a rather uninteresting one (Recommended.Proto2.JsonInput.FieldNameExtension.Validator). I've also added some more cleanups and lint fixes.

I think I am approaching a state where I can actually focus on the other TODOs mentioned in my other comments above.

caspermeijn reviewed Jun 13, 2024

View reviewed changes

ehiggs reviewed Jun 21, 2024

View reviewed changes

cynecx commented Jun 23, 2024

View reviewed changes

prost-build/src/code_generator.rs Show resolved Hide resolved

cynecx mentioned this pull request Jul 4, 2024

Better serde support for Enumerations #1087

Open

cynecx force-pushed the serde branch from c3cc1e0 to a8f365f Compare July 21, 2024 18:24

cynecx force-pushed the serde branch from a8f365f to 5e094f3 Compare August 22, 2024 20:56

cynecx added 17 commits November 22, 2024 22:04

feat: add serde support

4674b34

Move built-in serialization impls to main prost to avoid orphan check…

b726394

… workarounds

Don't encode empty message in new any repr

73c4b38

Cover old len/buffer checks in new roundtrip checker

70160a9

Cleanup roundtrip testing

962ea03

clippy

5413e05

Cleanup cargo features

5d9e663

Fix import typo

0a3c569

Bump to min required itertools

2e977f0

Address warning with MSRV toolchain

5400096

Format Cargo.tomls

976e3b2

Don't include serde in default features + enable serde impls in tests

23a4b3a

Format Cargo.tomls

ffd5be3

Fix tests failures

c3f9672

Keep previous Any impl exposed

8778f3e

codegen: fix additional indent

a52d4fc

more conformance fixes to oneof handling and nan/inf serialization

a544538

cynecx force-pushed the serde branch from 9962114 to a544538 Compare November 22, 2024 22:37

ehiggs reviewed Nov 25, 2024

View reviewed changes

cynecx added 13 commits November 27, 2024 14:05

support protobuf's json_name override option in prost-build

f297fb8

Add documentation for camelcase json name conversion

25fcc10

fix comment in conformance failing tests

e19ec2f

properly parse and convert json fieldmasks + parse NullValue's string…

5cf3c06

… repr

support for ignoring unknown enum values

1ee2567

address some clippy lints

35786dd

small fixes related to cfg and imports

f062b3c

enable serde for tests

e5b5ea8

address doc failures

b3f6b79

make serde module more no_std aware

0c87530

address more clippy lints

d34a05f

make prost-types no-std aware

1b072d9

make tests more no_std aware

167f6c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add json (serde) support #1081

WIP: Add json (serde) support #1081

cynecx commented Jun 13, 2024 •

edited

Loading

caspermeijn left a comment

caspermeijn Jun 13, 2024

cynecx Jun 14, 2024 •

edited

Loading

cynecx commented Jun 14, 2024

ehiggs Jun 21, 2024

cynecx Jun 21, 2024 •

edited

Loading

ehiggs Jun 21, 2024

cblichmann commented Oct 19, 2024

cynecx commented Nov 22, 2024

ehiggs Nov 25, 2024 •

edited

Loading

cynecx Nov 25, 2024

ehiggs Nov 25, 2024

ehiggs Nov 25, 2024

cynecx Nov 25, 2024

ehiggs Nov 25, 2024

ehiggs commented Nov 25, 2024

cynecx commented Nov 25, 2024

cynecx commented Dec 8, 2024

WIP: Add json (serde) support #1081

Are you sure you want to change the base?

WIP: Add json (serde) support #1081

Conversation

cynecx commented Jun 13, 2024 • edited Loading

caspermeijn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cynecx Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

cynecx commented Jun 14, 2024

Choose a reason for hiding this comment

cynecx Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblichmann commented Oct 19, 2024

cynecx commented Nov 22, 2024

ehiggs Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehiggs commented Nov 25, 2024

cynecx commented Nov 25, 2024

cynecx commented Dec 8, 2024

cynecx commented Jun 13, 2024 •

edited

Loading

cynecx Jun 14, 2024 •

edited

Loading

cynecx Jun 21, 2024 •

edited

Loading

ehiggs Nov 25, 2024 •

edited

Loading