Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proto3 doesn't pack repeated fields by default #976

Open
daniel-sherwood opened this issue Jan 31, 2024 · 5 comments
Open

proto3 doesn't pack repeated fields by default #976

daniel-sherwood opened this issue Jan 31, 2024 · 5 comments

Comments

@daniel-sherwood
Copy link

The code here

.map_or(self.syntax == Syntax::Proto3, |options| options.packed())
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
Copy link
Author

daniel-sherwood commented Feb 1, 2024

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

@caspermeijn
Copy link
Collaborator

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.

@bhollis
Copy link

bhollis commented May 1, 2024

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

if can_pack(&field)
&& !field
.options
.as_ref()
.map_or(self.syntax == Syntax::Proto3, |options| options.packed())
{
self.buf.push_str(", packed=\"false\"");
}

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.

@caspermeijn
Copy link
Collaborator

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

@bhollis
Copy link

bhollis commented May 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants