meta generated for messages with oneOf properties is incorret for other properties

[jkristia]

I see an issue in the generated meta when a class contains a oneof type.
this class

message UsingOneOf {
  type1.Type1Basic some_type = 1;
  oneof one_of_type {
    type1.Type1Basic one_of_type1 = 3;
    type2.Type2Basic one_of_type2 = 4;
  }
}

generates oneOfIndex: 0 for all 3 properties, not just the 2 properties in the oneof field.
This is with version

    "ts-proto": "^1.170.0",
    "ts-proto-descriptors": "^1.15.0",

The generated meta. Notice field some_type also has oneOfIndex: 0

   {
      "name": "UsingOneOf",
      "field": [{
        "name": "some_type",
        "number": 1,
        "label": 1,
        "type": 11,
        "typeName": ".type1.Type1Basic",
        "extendee": "",
        "defaultValue": "",
        "oneofIndex": 0,   <----
        "jsonName": "someType",
        "options": undefined,
        "proto3Optional": false,
      }, {
        "name": "one_of_type1",
        "number": 3,
        "label": 1,
        "type": 11,
        "typeName": ".type1.Type1Basic",
        "extendee": "",
        "defaultValue": "",
        "oneofIndex": 0,
        "jsonName": "oneOfType1",
        "options": undefined,
        "proto3Optional": false,
      }, {
        "name": "one_of_type2",
        "number": 4,
        "label": 1,
        "type": 11,
        "typeName": ".type2.Type2Basic",
        "extendee": "",
        "defaultValue": "",
        "oneofIndex": 0,
        "jsonName": "oneOfType2",
        "options": undefined,
        "proto3Optional": false,
      }],
      "extension": [],
      "nestedType": [],
      "enumType": [],
      "extensionRange": [],
      "oneofDecl": [{ "name": "one_of_type", "options": undefined }],
      "options": undefined,
      "reservedRange": [],
      "reservedName": [],
    }

[stephenh]

@lukealvoeiro I haven't looked into this very much, but oneofIndex is a proto2 field; could this be from a ts-proto-descriptor thing / your recent PR?

You'd mentioned bumping ts-proto-descriptors, but I don't think that ended up happening, but I kinda thought it shouldn't really matter since it'd still be using the generated-by-older-ts-proto versions of the decode code... 🤔

[cmd-johnson]

I just stumbled over this same thing: I can't reconstruct oneofs correctly from the file descriptors generated by ts-proto.

I think this might be coming from running the fileDesc through FileDescriptorProto.fromPartial(...), which assigns default values to all missing fields, setting the (correctly) empty value of oneofIndex to 0 instead.

I'm not quite sure what a good fix would look like. I've tried removing the disableProto2Optionals=true flag in protos/build.sh, which generates the types correctly with most values now being optional, but that leads to a whole lot of other problems downstream during schema generation.

---

Is there a good way to attach a debugger to the plugin to be able to step through the code while the .ts files are generated? That would really help me to get into this further.

[stephenh]

Is there a good way to attach a debugger to the plugin to be able to
step through the code while the .ts files are generated?

I know I've done that before, but it looks like maybe in a previous build setup (when we had the *.bin files), and/or the settings are on my other machine...

My current guess of what would work is to take the protoc invocation from integration/update-code.sh and the NODE_OPTIONS before it and just put debugger stuff in there:

  NODE_OPTIONS="--import tsx" protoc --experimental_allow_proto3_optional \
    "--plugin=$PLUGIN_PATH" \
    --ts_proto_opt="annotateFilesWithVersion=false,${PARAMETERS}" \
    --ts_proto_out=./ \
    $PROTO_FILES

Like --inspect-brk=9229...

Note that integration/update-code.sh uses its own integration/protoc-gen-ts_proto script that will use the src/**.ts files instead of the built *.js files like the top-level ./protoc-gen-ts_proto does; which is useful for a quick workflow.

Actually wouldn't be a bad idea to just have a DEBUG=true/false variable at the top of update-cost.sh, so that contributors could easily flip that to true and then just run (from within the integration directory) ./update-code.sh whatever-test-theyre-working-on.

Thanks for looking into this @jonaskello !

[cmd-johnson]

With that debugging actually worked on the first try! 👍

---

Here's what I've tried so far:

Removing the disableProto2Optionals=true flag in protos/build.sh, updating all usages of the now optional fields with explicit default values and removing the call to FileDescriptorProto.fromPartial(fileDesc) appears to be working fine for generating the actual code. I think this is needed because the oneofIndex must be optional in the generated schema types or the type of the protoMetadata will not match the expected type when we start removing the onofIndex or setting it to undefined for values that aren't part of a oneof declaration. (It also sounds like a breaking change for people using the protoMetadata value.)

Where this approach fails is when generating the protoMetadata.fileDescriptor field. This appears to be mainly because the introspection schema instances are all using Object.create(...) calls, effectively adding all the empty default fields (i.e. arrays and maps) to the returned object's prototypes instead of directly on them.

I can see that's by design (changelog of v1.61.0) and is perfectly fine if we're using the properties directly, but using these objects inside code`fileDescriptor: ${descriptor}` only iterates over the object's own properties, effectively hiding a lot of relevant information in the generated code output.

What might work is to always set the option on the object itself instead of updating it's prototype's value
E.g. instead of

message.arrayProperty.push(ValueProto.decode(reader, reader.uint32()));

do

message.arrayProperty = message.arrayProperty;
message.arrayProperty.push(ValueProto.decode(reader, reader.uint32()));

...which looks ridiculous, but actually does what we want - attach the property to the actual object instead of its prototype if and only if the value differs from the default value.
We could also check for message.hasOwnProperty("arrayProperty") first, but I'm guessing that simply always doing the assignment will be more performant 🤷.
Alternatives would be message.arrayProperty = message.arrayProperty.concat(...) or message.arrayProperty = [...message.arrayProperty, ...], but those will be a lot slower than just push()ing values onto an existing array.

This appears to fix the oneofIndex problem, but now the array fields that are marked as required in the ProtoMetadata type (or rather the underlying FileDescriptorProto type) don't appear in the protoMetadata export anymore, because again, they're only defined on an object's prototype instead of the objects themselves and no values were added to them.

Anyway, that's as far as I got today. If you have any more insights that'd be great!