mapbox / pbf

A low-level, lightweight protocol buffers implementation in JavaScript.
BSD 3-Clause "New" or "Revised" License
801 stars 107 forks source link

Write default values to oneof fields anyway #103

Closed farwayer closed 8 months ago

farwayer commented 5 years ago

Simple fix #102

kjvalencik commented 5 years ago

What do the non-JS libraries encode? It seems strange to encode a one-of with the default since you can still decode the default. I've never tested this.

farwayer commented 5 years ago

We should encode default value to know what one-of field was set. Official python and js protobuf library encode default value of one-of too:

message Id {
  oneof type {
    uint64 numId = 1;
    double doubleId = 2;
    string strId = 3;
  }
}

js

const id = new google_pb.Id();
id.setNumid(0);
const s = id.serializeBinary();
console.log(Buffer.from(s))

<Buffer 08 00>

python

import p_pb2

id = p_pb2.Id()
id.numId = 0
data = id.SerializeToString()
print(data)

b'\x08\x00'

Of course it will be nice to add check user set only one one-of field but logic will be more complex. For now, let's assume the user is doing the right thing.

kjvalencik commented 5 years ago

Ah, okay. I think I misunderstood. I thought you were saying that when the oneof isn't set it should still encode the oneof. That's not what the C++ implementation does. If you don't set any fields, it should be 0 bytes.

It sounds like this is a different issue from that, if I understand correctly.

farwayer commented 5 years ago

Yes, the issue is not about whole oneof field but is about oneof subfield with default value. We should encode it anyway.

farwayer commented 3 years ago

Is it any change this PR will be merged? Behavior is still incorrect.