protobuf-es icon indicating copy to clipboard operation
protobuf-es copied to clipboard

Add message and field name to binary serialization errors

Open bhollis opened this issue 1 year ago • 3 comments

This adds the message and field name to errors thrown during binary serialization of scalar fields. It should convert a message like:

Error: invalid uint32: -1

To:

Error: cannot encode field com.whatever.MyMessage.my_field to binary: invalid uint32: -1

I followed the wording of the error thrown here for the phrasing: https://github.com/bufbuild/protobuf-es/blob/main/packages/protobuf/src/to-binary.ts#L74

Fixes #983

bhollis avatar Oct 03 '24 18:10 bhollis

Thanks for the PR, Ben. We'll need some insight whether / how this impacts performance and bundle size. I think it would be ideal to use the same mechanism for error messages that toJson uses (but writeField is also used for extensions). We need good test coverage to make sure error messages have the expected content and will not regress.

timostamm avatar Oct 07 '24 09:10 timostamm

Running npm run perf in packages/protobuf-test:

Edit: Hmm, looks like there aren't any performance tests for toBinary. Let me add them.

Before:

toBinary   perf-payload.bin x 5,528 ops/sec ±4.33% (94 runs sampled)
toBinary   tiny example.User x 1,098,031 ops/sec ±0.35% (95 runs sampled)
toBinary   normal example.User x 177,436 ops/sec ±1.62% (95 runs sampled)
toBinary   scalar values x 275,100 ops/sec ±0.76% (97 runs sampled)
toBinary   repeated scalar values x 94,595 ops/sec ±0.64% (98 runs sampled)
toBinary   map with scalar keys and values x 65,719 ops/sec ±0.58% (95 runs sampled)
toBinary   repeated field with 1000 messages x 2,984 ops/sec ±0.56% (98 runs sampled)
toBinary   map field with 1000 messages x 729 ops/sec ±0.71% (97 runs sampled)

After:

toBinary   perf-payload.bin x 5,452 ops/sec ±1.57% (97 runs sampled)
toBinary   tiny example.User x 1,076,997 ops/sec ±0.32% (96 runs sampled)
toBinary   normal example.User x 182,993 ops/sec ±0.56% (98 runs sampled)
toBinary   scalar values x 277,949 ops/sec ±0.55% (95 runs sampled)
toBinary   repeated scalar values x 90,580 ops/sec ±3.47% (92 runs sampled)
toBinary   map with scalar keys and values x 63,867 ops/sec ±0.62% (95 runs sampled)
toBinary   repeated field with 1000 messages x 2,956 ops/sec ±0.51% (97 runs sampled)
toBinary   map field with 1000 messages x 696 ops/sec ±0.79% (94 runs sampled)

bhollis avatar Oct 07 '24 16:10 bhollis

Thanks for the updates, Ben. I'll take a look as soon as possible, but have to take care of some Connect work first.

timostamm avatar Oct 09 '24 13:10 timostamm

A gentle nudge on this - it would still be of great benefit to have this information.

bhollis avatar Dec 18 '24 06:12 bhollis

Sure, here are those changes.

bhollis avatar Jan 07 '25 22:01 bhollis