Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write default values to oneof fields anyway #103

Closed
wants to merge 1 commit into from
Closed

Write default values to oneof fields anyway #103

wants to merge 1 commit into from

Conversation

farwayer
Copy link

Simple fix #102

@kjvalencik
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

farwayer commented May 21, 2019

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

@farwayer
Copy link
Author

farwayer commented May 9, 2021

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

@farwayer farwayer closed this by deleting the head repository Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible incorrect oneof serealization
2 participants