TypeError when using optional on custom field options

[ehyland]

Problem description

When marking a custom field option as optional, loadSync throws a the following error

TypeError: Cannot read properties of undefined (reading 'indexOf')

Note this error only occurs when using optional

Reproduction steps

1. Clone https://github.com/ehyland/tmp-proto-loader-issue
2. Install dependencies npm i
3. Run test script node test-with-optional.js

Environment

• OS & Arch: MacOS 12.2 - amd64
• Node: v16.13.0
• Node installation method: nvm
• Package name and version: @grpc/[email protected]

Additional context

Error with stack

./node_modules/protobufjs/ext/descriptor/index.js:488
        if ((descriptor.oneofIndex = this.parent.oneofsArray.indexOf(this.partOf)) < 0)
                                                             ^

TypeError: Cannot read properties of undefined (reading 'indexOf')
    at Field.toDescriptor (./node_modules/protobufjs/ext/descriptor/index.js:488:62)
    at Root_toDescriptorRecursive (./node_modules/protobufjs/ext/descriptor/index.js:142:40)
    at Root_toDescriptorRecursive (./node_modules/protobufjs/ext/descriptor/index.js:146:13)
    at Root.toDescriptor (./node_modules/protobufjs/ext/descriptor/index.js:121:5)
    at createPackageDefinition (./node_modules/@grpc/proto-loader/build/src/index.js:148:33)
    at Object.loadSync (./node_modules/@grpc/proto-loader/build/src/index.js:195:12)
    at Object.<anonymous> (./test.js:6:13)

Proto file that will fail to load

See https://github.com/ehyland/tmp-proto-loader-issue to reproduce

syntax = "proto3";
package example;
import "google/protobuf/descriptor.proto";

extend google.protobuf.FieldOptions {
  optional bool sensitive = 99999;
}

message MyMessage {
  string id = 1;
  string secret_sauce = 3 [(example.sensitive) = true];
}

[samc]

A quick change from ↴

syntax = "proto3";
...

to ↴

syntax = "proto2";
...

... should do the trick. The reason that you're getting an error is because the optional key modifier doesn't exist in proto3 - all values are optional by default. You can either remove the optional modifiers, and stick with proto3, or just use the proto2 syntax.

[murgatroid99]

That is incorrect. A new optional modifier was added in proto3 with slightly different semantics than the similar modifier in proto2. Protobuf.js has support for that modifier, but this seems to be a consequence of the combination of that modifier, custom field options, and descriptor message generation.

[samc]

Interesting, just taking a look at this write-up on the changes to field presence behaviors in the v3.15 release - that table outlines the optional modifier support for the scalar primitives. So, the combination of the optional modifier with the bool scalar is more than likely a syntactic error.

Looking at the official docs:

For string, bytes, and message fields, optional is compatible with repeated. Given serialized data of a repeated field as input, clients that expect this field to be optional will take the last input value if it's a primitive type field or merge all input elements if it's a message type field. Note that this is not generally safe for numeric types, including bools and enums. Repeated fields of numeric types can be serialized in the packed format, which will not be parsed correctly when an optional field is expected.

[murgatroid99]

I don't think you're interpreting that correctly either. The table you linked has two entries for "Singular numeric (integer or floating point)", one with optional and one without, which shows that optional numeric fields are supported.

The other quotation there is about the wire compatibility between message declarations that declare a field as optional and declarations for the same message type that declare a field as repeated. It doesn't say anything about the usability of the optional modifier itself.

[samc]

Looking at the call stack, starting at parseExtension:

https://github.com/protobufjs/protobuf.js/blob/f5b893c03e9694bbe7da7c4001cc74b06039eb9c/src/parse.js#L749
↓
https://github.com/protobufjs/protobuf.js/blob/f5b893c03e9694bbe7da7c4001cc74b06039eb9c/src/parse.js#L396
↓
https://github.com/protobufjs/protobuf.js/blob/f5b893c03e9694bbe7da7c4001cc74b06039eb9c/src/namespace.js#L218

So it looks like namespace extensions aren't compatible with optional modifiers whatsoever in proto3. Did a quick search and it looks like protobufjs/protobuf.js#1602 adds a type guard, but it hasn't landed in a 6.11.x release as of yet.

[samc]

I don't think you're interpreting that correctly either. The table you linked has two entries for "Singular numeric (integer or floating point)", one with optional and one without, which shows that optional numeric fields are supported.

Correct me if I'm wrong but we're talking about a bool field type in optional bool sensitive = 99999;, not a singular numeric field type. I haven't looked at the source for optional default value coalescence for bool scalars but I would imagine it's a noop given the removal of explicit default values in proto3, and the fact that in a binary value, explicit presence is also a noop (substitution).

The only scenario I could see a where an optional bool value would make sense is in the case of default: true.

Don't have a ton of experience with either codebase, so take all of that w/ a grain of salt.

[murgatroid99]

Yes, that is the field I am talking about. bool is a numeric type. It doesn't match any other entry in that table better, and it is encoded as a varint, the same as most integer types. It just happens to only have 2 valid values.

The purpose of optional in proto3 is to add field presence information that otherwise doesn't exist. Essentially, it makes the value nullable. There is no contradiction when using it with bool, because it effectively adds a third possible value.

That is a good catch with the linked PR. I see that the PR to publish it, protobufjs/protobuf.js#1603, is still pending. I think it is safe to say that fixing this bug is blocked on that release.

[arussellsaw]

👋 hello! Should this be resolved now? i'm hitting this issue (or at least the same stacktrace) in the Retool on-prem container, and unsure if i need to be asking them to upgrade to a more recent version, or if the bug still exists!

[murgatroid99]

There may be multiple causes of very similar stacktraces. Can you file a separate issue with the details of the problem you are experiencing?

[ezequielmasciarelli]

Hello, I am also having exactly the same issue as @arussellsaw regarding retool. We are also hitting this issue on retool

[xuanlongvts]

the same

[wSedlacek]

When using extend google.protobuf.FieldOptions I also receive Cannot read properties of undefined.
The this.parent is a Namespace.

It seems like it would be very easy to just catch the undefined and skip it even if the extended fields are left out of the loaded protos.

An easy work around (at least for me) was to remove optional from my extension.

[murgatroid99]

Can you share a stacktrace of the error you are getting?

[wSedlacek]

TypeError: Cannot read properties of undefined (reading 'indexOf')
    at Field.toDescriptor (/Users/wsedlacek/Code/work/shipyard/dist/apps/event-sync/main.js:75586:62)
    at Root_toDescriptorRecursive (/Users/wsedlacek/Code/work/shipyard/dist/apps/event-sync/main.js:75289:66)
    at Root_toDescriptorRecursive (/Users/wsedlacek/Code/work/shipyard/dist/apps/event-sync/main.js:75291:55)
    at Root_toDescriptorRecursive (/Users/wsedlacek/Code/work/shipyard/dist/apps/event-sync/main.js:75291:55)
    at Root_toDescriptorRecursive (/Users/wsedlacek/Code/work/shipyard/dist/apps/event-sync/main.js:75291:55)
    at Root.toDescriptor (/Users/wsedlacek/Code/work/shipyard/dist/apps/event-sync/main.js:75275:5)
    at createPackageDefinition (/Users/wsedlacek/Code/work/shipyard/dist/apps/event-sync/main.js:13457:33)
    at loadSync (/Users/wsedlacek/Code/work/shipyard/dist/apps/event-sync/main.js:13501:12)

extend google.protobuf.FieldOptions {
  optional bool some_value = 50000;
}

This works though.

extend google.protobuf.FieldOptions {
 bool some_value = 50000;
}