tatethurston / TwirpScript

A protobuf RPC framework for JavaScript and TypeScript
MIT License
143 stars 13 forks source link

Null message in response are filled with empty values #162

Closed alexisvisco closed 2 years ago

alexisvisco commented 2 years ago

Hi !

When I receive a protobuf message with a field that is a message and is null, the decode will read the message and set default value for it. However it is null.

Could you please fix this bug ? :)

tatethurston commented 2 years ago

Hey @alexisvisco, a few questions:

  1. Is this during a JSON or Protobuf decoding?
  2. Is this an optional field?

I think this is the correct behavior per the the protobuf docs:

If a value is missing in the JSON-encoded data or if its value is null, it will be interpreted as the appropriate default value when parsed into a protocol buffer.

But happy to talk through this more.

alexisvisco commented 2 years ago
  1. It is with the protobuf decoding
  2. It is not an optional field but it is a message (not a native type)

I don't think message are serialized with default value because in Go message field are generated with pointers and when you provide nothing for a field that is a message it put a null pointer.

Is this more clear to you ?

alexisvisco commented 2 years ago
message FindByIdResponse {
  Dashboard dashboard = 1;
}

If in my server implementation I return an empty FindByIdResponse, the go client will set dashboard as a null pointer. It is the correct behavior imo.

tatethurston commented 2 years ago

What's the reason to avoid marking dashboard as optional? https://developers.google.com/protocol-buffers/docs/proto#optional

message FindByIdResponse {
  optional Dashboard dashboard = 1;
}

This is more flexible because it enables consumers to decide the nullability semantics of a given field.

alexisvisco commented 2 years ago

https://stackoverflow.com/a/42634681/8810797

In protobuf 3 all fields are optionals :)

tatethurston commented 2 years ago

The author of this post is overloading the term optional, which is why the author uses quotes when referencing optional: "optional". optional in protobuf is a keyword with a narrow and specific definition.

In proto3, all fields are "optional" (in that it is not an error if the sender fails to set them).

The author of the post is correct that it's not an error if the sender fails to send them -- in fact protobuf libraries generally don't send fields that are the default value under protobuf regardless of what the sender sets. That is an optimization that can be applied at the serialization layer to decrease the size of the message over the wire. But this is not describing the semantics of the optional keyword.

optional is used to determine the semantics on the deserialization side*. Specifically, do we leave the value as an "empty" / "bottom" type (undefined / null / nil, etc) or do we "hydrate" with the default value.

protobuf3 originally did not support optional, but it was added in a minor version a few years ago (3.15), which was released after that stackoverflow post was published if I recall correctly.

* It also impacts the over the wire semantics, to disambiguate no value from a default value.

alexisvisco commented 2 years ago

This is maybe a misunderstanding because optional was recently added in proto 3.

However setting optional prefix to a field which is a message does not solve the problem. In my server implementation I return the field with a null value and the client get a message with default value, how am I supposed to know if the field is set or not ?

tatethurston commented 2 years ago

Thanks @alexisvisco. TwirpScript's message deserialization had a bug when processing optional, message fields where the default message value was always being supplied. This has been fixed. Now if the server supplies an optional field with a null value the client will receive undefined when reading the field.