repeated custom options cannot be parsed

[hugebdu]

protobuf.js version: 6.8.8

repeated custom options are not supported due to being mapped to object type in ReflectionObject#options

Maybe should be an array?

[rclark]

Running up against this problem when trying to parse .proto files in https://github.com/envoyproxy/data-plane-api.

Here is an example of repeated options that can't be parsed:

  repeated core.HeaderValueOption request_headers_to_add = 6
      [(validate.rules).repeated .max_items = 1000];

In another file in that repository, these custom field options also cannot be parsed (though this may be a totally different issue):

  google.protobuf.Duration idle_timeout = 24
      [(validate.rules).duration.gt = {}, (gogoproto.stdduration) = true];

[petebarnett]

@rclark Just found your post while working with the same set of proto files from Envoy.
It affects all custom options used inline with a message or enum.
I'm now trying to work around it by pre-processing the files to remove these, since they result in no code generation anyway.
Ideally, I'd love to implement Lyft protoc-gen-validate for Protobuf.js, but I don't know enough yet about extension points for parsing or codegen. Not sure if it's even possible.

[eyalpost]

@nicolasnoble @alexander-fenster Is there any intention to fix this?
I'm willing to give it a try and make a PR but want to first check with you about which solution fits best here since this can break existing clients.
Should we add an option that indicates whether to parse repeated options?

[alexander-fenster]

@eyalpost We haven't looked at this one yet, so the below text is just some general thoughts.

When we looked at similar issues before, the questions that we asked often were "how does protoc behave in this case", and "what's written in the protobuf Language Guide". For the latter question, I was unable to find an exact answer now, so, I think, the protoc behavior would be our best baseline to look at.

Anyway, the current suggestion here in this issue (replace an option value with a list of values) can break the current users so cannot be released without a semver major release. I wonder if there is a compatible and non-ugly way of making it work without breaking any of the existing code :)

[hugebdu]

@alexander-fenster how about leaving ReflectionObject#options the way it is today and introducing a new property ReflectionObject#rawOptions of type Array?
Similar to node's http.IncomingMessage#rawHeaders?

[alexander-fenster]

@hugebdu That might be a good idea since adding the new property won't break the existing users and those who need repeated custom options can just start using it. Feel free to send a PR and we'll be happy to review it!

[eyalpost]

Another question before starting to implement.
So assuming we have the following proto:

extend google.protobuf.FieldOptions {
    repeated int32 flags = 50000;
}

message TestFieldOptionsInt {
    string field1 = 2 [(flags) = 555, (flags) = 666];
    string field2 = 3 [(flags) = 777];
}

I would expect field1.rawOptions["(flags)"] to return [555,666]
But what would you expect field2.rawOptions["(flags)"] to return?
Just 777 ? Or [777]?

[alexander-fenster]

I personally would expect it to return [777], so that when a new option gets added to the proto (which by itself is a non-breaking change), this change will not break the existing code. I'd love to see other folks' thoughts on that (@murgatroid99, @nicolasnoble WDYT?)

[eyalpost]

I tend to agree, but the main problem is that currently the parser does not resolve the option definition so it has no knowledge whether the option is repeated or not.
I suggest we start with rawOptions as a mechanism to simply collect the options from the proto definition "as-is" (Implemented in PR #1254 ) and will always appear as an array of options - This means for the above case, the rawOptions for field1 will be [{flags: 555}, {flags: 666}] and for field2 it will be [{flags: 777}] which means the client is responsible for finding all the values for a flag that they are interested in.

[alexander-fenster]

@eyalpost I think this is fine, and this is exactly the raw options as the name suggests, so the user can choose how they process the options. We'll take a look at the PR early next week! Thank you for handling this.

[eyalpost]

There's a limitation with the current implementation with message based options. Would love to hear your feedback on this.
Consider the following scenario:

message Rule {
  string val = 1;
  string val2 = 2;
}
extend google.protobuf.MessageOptions { 
    repeated Rule rule = 50000; 
}

message TestOptions {
    option (rule) = {val : "X"};
    option (rule) = {val2: "Y", val: "Z"};
}

The current implementation will produce these rawOptions:

[
 {"(rule).val": "X"},
 {"(rule).val2": "Y"},
 {"(rule).val": "Z"}
]

Which means there's no way to reconstruct the original definition.
I already have a POC which produces the following instead:

[
  {"(rule)": {"val": "X"}},
  {"(rule)": {"val2": "Y", "val": "Z"}}
]

but I need to give it some more work before it's ready for review

[eyalpost]

@alexander-fenster Opened a new PR: #1256 which implements proper parsing of options

[eyalpost]

@alexander-fenster note that this new PR still keeps the current options field intact to make sure it doesn’t break existing clients