protobufjs / protobuf.js

Protocol Buffers for JavaScript & TypeScript.
Other
9.97k stars 1.42k forks source link

Field.optional should not be !Field.required for proto3 #1850

Open egamble opened 1 year ago

egamble commented 1 year ago

protobuf.js version: 7.1.2

We are parsing a .proto file in proto3 format with protobufjs.loadSync. Using the resulting parsed objects, we need to distinguish between a field that is marked optional vs. a field that is not marked optional and also not marked required. Unfortunately, the protobufjs parser always sets Field.optional to the value of !Field.required, so we can't detect the optional keyword in the parsed Field object.

We use the Golang protobuf library with the same .proto file (in proto3 format) which generates different Golang code for fields marked optional, vs. fields not marked as either optional or required. We need to use the protobufjs parser in a similar way.

For example, we would like to detect the difference between the parsed field1 and field2 objects in this .proto file:

syntax = "proto3";

message Message1 {
  string field1 = 1;
  optional string field2 = 2;
}
jonaskello commented 3 months ago

I encountered the same problem, the parser ignores the optional keyword. Is there a work-around?

egamble commented 3 months ago

I encountered the same problem, the parser ignores the optional keyword. Is there a work-around?

Haven't found one.

martin-traverse commented 3 months ago

Hi - I did some work related to this in PR #2011 to fix nullability in the type info for TS and JS Doc. The short answer is that for proto3 you probably want this test:

var is_optional = field.options != null && field.options["proto3_optional"] === true;

For a more complete answer, rather than thinking of optional / required, I found it more helpful to think of "implicit" and "explicit" presence as described in the new Protobuf editions spec. The optional keyword in proto3 indicates explicit presence, but implicit presence fields are still optional and can be omitted on the wire, so setting field.optional = false would not be accurate and would probably cause a lot of problems. Only proto2 has fields that are actually required.

In cli/targets/static.js I added a couple of methods, isExplicitPresence() and isImplicitPresence(), which work for both proto2 and proto3 syntax. You could make a similar method isLegacyRequired(). Ideally these methods should be updated to work with the new editions syntax qualifiers as well.

For reference: https://protobuf.dev/editions/