Support proto3 field presence (#281)

* Update protobuf pregenerated files

* Update grpcio-tools to latest version

* Implement proto3 field presence

* Fix to_dict with None optional fields.

* Add test with optional enum

* Properly support optional enums

* Add tests for 64-bit ints and floats

* Support field presence for int64 types

* Fix oneof serialization with proto3 field presence (#292)

= Description

The serialization of a oneof message that contains a message with fields
with explicit presence was buggy.

For example:

```
message A {
    oneof kind {
        B b = 1;
        C c = 2;
    }
}

message B {}
message C {
    optional bool z = 1;
}
```

Serializing `A(b=B())` would lead to this payload:

```
0A # tag1, length delimited
00 # length: 0
12 # tag2, length delimited
00 # length: 0
```

Which when deserialized, leads to the message `A(c=C())`.

= Explanation

The issue lies in the post_init method. All fields are introspected, and
if different from PLACEHOLDER, the message is marked as having been
"serialized_on_wire".
Then, when serializing `A(b=B())`, we go through each field of the
oneof:

- field 'b': this is the selected field from the group, so it is
  serialized
- field 'c': marked as 'serialized_on_wire', so it is added as well.

= Fix

The issue is that support for explicit presence changed the default
value from PLACEHOLDER to None. This breaks the post_init method in that
case, which is relatively easy to fix: if a field is optional, and set
to None, this is considered as the default value (which it is).

This fix however has a side-effect: the group_current for this field (the
oneof trick for explicit presence) is no longer set. This changes the
behavior when serializing the message in JSON: as the value is the
default one (None), and the group is not set (which would force the
serialization of the field), so None fields are no longer serialized in
JSON. This break one test, and will be fixed in the next commit.

* fix: do not serialize None fields in JSON format

This is linked to the fix from the previous commit: after it, scalar
None fields were not included in the JSON format, but some were still
included.

This is all cleaned up: None fields are not added in JSON by default,
as they indicate the default value of fields with explicit presence.
However, if `include_default_values is set, they are included.

* Fix: use builtin annotation prefix

* Remove comment

Co-authored-by: roblabla <[email protected]>
Co-authored-by: Vincent Thiberville <[email protected]>

Author: 3 people

.github/workflows/ci.yml

@@ -66,4 +66,4 @@ jobs:
 
       - name: Execute test suite
         shell: bash
-        run: poetry run pytest tests/
+        run: poetry run python -m pytest tests/

poetry.lock

@@ -23,7 +23,7 @@ python-dateutil = "^2.8"
 asv = "^0.4.2"
 black = "^21.11b0"
 bpython = "^0.19"
-grpcio-tools = "^1.30.0"
+grpcio-tools = "^1.40.0"
 jinja2 = "^2.11.2"
 mypy = "^0.770"
 poethepoet = ">=0.9.0"

pyproject.toml

@@ -145,6 +145,8 @@ class FieldMetadata:
     group: Optional[str] = None
     # Describes the wrapped type (e.g. when using google.protobuf.BoolValue)
     wraps: Optional[str] = None
+    # Is the field optional
+    optional: Optional[bool] = False
 
     @staticmethod
     def get(field: dataclasses.Field) -> "FieldMetadata":
@@ -159,12 +161,15 @@ def dataclass_field(
     map_types: Optional[Tuple[str, str]] = None,
     group: Optional[str] = None,
     wraps: Optional[str] = None,
+    optional: bool = False,
 ) -> dataclasses.Field:
     """Creates a dataclass field with attached protobuf metadata."""
     return dataclasses.field(
-        default=PLACEHOLDER,
+        default=None if optional else PLACEHOLDER,
         metadata={
-            "betterproto": FieldMetadata(number, proto_type, map_types, group, wraps)
+            "betterproto": FieldMetadata(
+                number, proto_type, map_types, group, wraps, optional
+            )
         },
     )
 
@@ -174,74 +179,107 @@ def dataclass_field(
 # out at runtime. The generated dataclass variables are still typed correctly.
 
 
-def enum_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_ENUM, group=group)
+def enum_field(number: int, group: Optional[str] = None, optional: bool = False) -> Any:
+    return dataclass_field(number, TYPE_ENUM, group=group, optional=optional)
 
 
-def bool_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_BOOL, group=group)
+def bool_field(number: int, group: Optional[str] = None, optional: bool = False) -> Any:
+    return dataclass_field(number, TYPE_BOOL, group=group, optional=optional)
 
 
-def int32_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_INT32, group=group)
+def int32_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_INT32, group=group, optional=optional)
 
 
-def int64_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_INT64, group=group)
+def int64_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_INT64, group=group, optional=optional)
 
 
-def uint32_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_UINT32, group=group)
+def uint32_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_UINT32, group=group, optional=optional)
 
 
-def uint64_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_UINT64, group=group)
+def uint64_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_UINT64, group=group, optional=optional)
 
 
-def sint32_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_SINT32, group=group)
+def sint32_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_SINT32, group=group, optional=optional)
 
 
-def sint64_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_SINT64, group=group)
+def sint64_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_SINT64, group=group, optional=optional)
 
 
-def float_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_FLOAT, group=group)
+def float_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_FLOAT, group=group, optional=optional)
 
 
-def double_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_DOUBLE, group=group)
+def double_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_DOUBLE, group=group, optional=optional)
 
 
-def fixed32_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_FIXED32, group=group)
+def fixed32_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_FIXED32, group=group, optional=optional)
 
 
-def fixed64_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_FIXED64, group=group)
+def fixed64_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_FIXED64, group=group, optional=optional)
 
 
-def sfixed32_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_SFIXED32, group=group)
+def sfixed32_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_SFIXED32, group=group, optional=optional)
 
 
-def sfixed64_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_SFIXED64, group=group)
+def sfixed64_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_SFIXED64, group=group, optional=optional)
 
 
-def string_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_STRING, group=group)
+def string_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_STRING, group=group, optional=optional)
 
 
-def bytes_field(number: int, group: Optional[str] = None) -> Any:
-    return dataclass_field(number, TYPE_BYTES, group=group)
+def bytes_field(
+    number: int, group: Optional[str] = None, optional: bool = False
+) -> Any:
+    return dataclass_field(number, TYPE_BYTES, group=group, optional=optional)
 
 
 def message_field(
-    number: int, group: Optional[str] = None, wraps: Optional[str] = None
+    number: int,
+    group: Optional[str] = None,
+    wraps: Optional[str] = None,
+    optional: bool = False,
 ) -> Any:
-    return dataclass_field(number, TYPE_MESSAGE, group=group, wraps=wraps)
+    return dataclass_field(
+        number, TYPE_MESSAGE, group=group, wraps=wraps, optional=optional
+    )
 
 
 def map_field(
@@ -586,7 +624,8 @@ def __post_init__(self) -> None:
             if meta.group:
                 group_current.setdefault(meta.group)
 
-            if self.__raw_get(field_name) != PLACEHOLDER:
+            value = self.__raw_get(field_name)
+            if value != PLACEHOLDER and not (meta.optional and value is None):
                 # Found a non-sentinel value
                 all_sentinel = False
 
@@ -701,12 +740,16 @@ def __bytes__(self) -> bytes:
 
             if value is None:
                 # Optional items should be skipped. This is used for the Google
-                # wrapper types.
+                # wrapper types and proto3 field presence/optional fields.
                 continue
 
             # Being selected in a a group means this field is the one that is
             # currently set in a `oneof` group, so it must be serialized even
             # if the value is the default zero value.
+            #
+            # Note that proto3 field presence/optional fields are put in a
+            # synthetic single-item oneof by protoc, which helps us ensure we
+            # send the value even if the value is the default zero value.
             selected_in_group = (
                 meta.group and self._group_current[meta.group] == field_name
             )
@@ -829,8 +872,9 @@ def _get_field_default_gen(cls, field: dataclasses.Field) -> Any:
                 # This is some kind of list (repeated) field.
                 return list
             elif t.__origin__ is Union and t.__args__[1] is type(None):
-                # This is an optional (wrapped) field. For setting the default we
-                # really don't care what kind of field it is.
+                # This is an optional field (either wrapped, or using proto3
+                # field presence). For setting the default we really don't care
+                # what kind of field it is.
                 return type(None)
             else:
                 return t
@@ -1041,6 +1085,9 @@ def to_dict(
                         ]
                     if value or include_default_values:
                         output[cased_name] = value
+                elif value is None:
+                    if include_default_values:
+                        output[cased_name] = value
                 elif (
                     value._serialized_on_wire
                     or include_default_values
@@ -1066,13 +1113,18 @@ def to_dict(
                 if meta.proto_type in INT_64_TYPES:
                     if field_is_repeated:
                         output[cased_name] = [str(n) for n in value]
+                    elif value is None:
+                        if include_default_values:
+                            output[cased_name] = value
                     else:
                         output[cased_name] = str(value)
                 elif meta.proto_type == TYPE_BYTES:
                     if field_is_repeated:
                         output[cased_name] = [
                             b64encode(b).decode("utf8") for b in value
                         ]
+                    elif value is None and include_default_values:
+                        output[cased_name] = value
                     else:
                         output[cased_name] = b64encode(value).decode("utf8")
                 elif meta.proto_type == TYPE_ENUM:
@@ -1085,6 +1137,12 @@ def to_dict(
                         else:
                             # transparently upgrade single value to repeated
                             output[cased_name] = [enum_class(value).name]
+                    elif value is None:
+                        if include_default_values:
+                            output[cased_name] = value
+                    elif meta.optional:
+                        enum_class = field_types[field_name].__args__[0]
+                        output[cased_name] = enum_class(value).name
                     else:
                         enum_class = field_types[field_name]  # noqa
                         output[cased_name] = enum_class(value).name
@@ -1141,6 +1199,9 @@ def from_dict(self: T, value: Dict[str, Any]) -> T:
                         setattr(self, field_name, v)
                     elif meta.wraps:
                         setattr(self, field_name, value[key])
+                    elif v is None:
+                        cls = self._betterproto.cls_by_field[field_name]
+                        setattr(self, field_name, cls().from_dict(value[key]))
                     else:
                         # NOTE: `from_dict` mutates the underlying message, so no
                         # assignment here is necessary.