ipfs / protons

Protocol Buffers for Node.js and the browser without eval
Other
32 stars 23 forks source link

Why don't write when enum field value is 0 #77

Closed skysbird closed 1 year ago

skysbird commented 1 year ago

https://github.com/ipfs/protons/blob/51746ec22fcc4337e3975a2a5eff871b336ab5e9/packages/protons/src/index.ts#L405

message HopMessage {
   enum Type {
     RESERVE = 0;
     CONNECT = 1;
     STATUS = 2;
 }
  required Type type = 1;
  optional Peer peer = 2;
  optional Reservation reservation = 3;
  optional Limit limit = 4;
  optional Status status = 5;
   }

When I protons this proto, it will gnenerate the code like this:

export const codec = (): Codec<HopMessage> => {
if (_codec == null) {
  _codec = message<HopMessage>((obj, w, opts = {}) => {
    if (opts.lengthDelimited !== false) {
      w.fork()
    }

    if (opts.writeDefaults === true || (obj.type != null && ___TypeValues[obj.type] !== 0) {
      w.uint32(8)
      HopMessage.Type.codec().encode(obj.type, w)
    }

This will cause the wrong binary data(MALFORM) when I encode the HopMessage with type value is RESERVE when the relay node is kubo

achingbrain commented 1 year ago
  required Type type = 1;

required has been removed from proto3.

To simulate it on the wire, mark the field optional, then a value will always be written into the protobuf, even if it's the default value, and proto2 parsers will be able to decode the message.

ckousik commented 1 year ago
  required Type type = 1;

required has been removed from proto3.

To simulate it on the wire, mark the field optional, then a value will always be written into the protobuf, even if it's the default value, and proto2 parsers will be able to decode the message.

Wouldn't this mean the value would not be written if not explicitly set? This would cause a well formatted proto3 message to fail when being decoded in proto2. Consider:

syntax = "proto3"
message Hop {
  enum Type {
    RESERVE = 0;
    ...
  }
  optional Type type;
}

As far as typechecks are considered HopMessage {} is a valid message and would be serialised as an empty slice, but this would fail when being parsed by proto2 since required Type type is not set.

achingbrain commented 1 year ago

Agreed, but there's no other way to ensure the value is written onto the wire in proto3.

Consider this:

syntax = "proto3"
message Hop {
  enum Type {
    RESERVE = 0;
    ...
  }
  Type type;
}

With this definition { type: 0 } would also be serialized to an empty slice and a proto2 parser would fail to read it.

It's better to upgrade everyone to proto3 and not use required - even Google express some pretty strong caveats over using that keyword.

Where that isn't possible the only thing you can do is mark it optional and enforce the field presence at the application level.

skysbird commented 1 year ago

related with these pr: https://github.com/libp2p/specs/pull/506 https://github.com/ipfs/protons/pull/80