prost icon indicating copy to clipboard operation
prost copied to clipboard

`proto3` doesn't pack repeated fields by default

Open daniel-sherwood opened this issue 5 months ago • 5 comments

The code here https://github.com/tokio-rs/prost/blob/63c00248abef84337c1010b6a10f3258b2db8b54/prost-build/src/code_generator.rs#L435 attempts to set the default behaviour for proto3 to be packed, however because options.packed() returns false unless [packed = true] is set on the field, this fails.

daniel-sherwood avatar Jan 31 '24 21:01 daniel-sherwood

Just realised this is only the case where the field has other attributes and hence options is not None

daniel-sherwood avatar Feb 01 '24 07:02 daniel-sherwood

I don't understand what your request is. Can you describe your observed behavior and your expected behavior? An example file or project will help with reproducing your problem.

caspermeijn avatar Feb 13 '24 07:02 caspermeijn

We're hitting the same bug. Consider a field like this:

  repeated uint64 user_ids = 2 [
    (buf.validate.field).repeated.min_items = 1,
    (buf.validate.field).repeated.max_items = 25
  ];

This generates the following from prost:

#[prost(uint64, repeated, packed="false", tag="2")]
pub user_ids: ::prost::alloc::vec::Vec<u64>,

However if I remove the options, like this:

  repeated uint64 user_ids = 2;

Then the generated code looks like this:

#[prost(uint64, repeated, tag="2")]
pub user_ids: ::prost::alloc::vec::Vec<u64>,

As @daniel-sherwood correctly identified, this is a bug at https://github.com/tokio-rs/prost/blob/63c00248abef84337c1010b6a10f3258b2db8b54/prost-build/src/code_generator.rs#L431-L438

In my first example, there are options on the field so map_or does not use its default and the code falls back to options.packed(). options.packed() appears to be either true or false, with a default of false. But it should default to true if the file is in proto3 syntax.

bhollis avatar May 01 '24 23:05 bhollis

Thanks, now I understand your problem better. Are you willing to open a PR to fix this?

caspermeijn avatar May 13 '24 14:05 caspermeijn

Maybe! It'd be my first time trying Rust, so it'd probably take a while.

bhollis avatar May 13 '24 17:05 bhollis