implement proto3 field presence (#88)

andrewparmet · web-flow · commit f00caa4cd358 · 2020-10-06T11:38:12.000-04:00
diff --git a/protokt-codegen/src/main/kotlin/com/toasttab/protokt/Main.kt b/protokt-codegen/src/main/kotlin/com/toasttab/protokt/Main.kt
@@ -23,6 +23,7 @@ import com.google.protobuf.DescriptorProtos.FileDescriptorProto
 import com.google.protobuf.ExtensionRegistry
 import com.google.protobuf.compiler.PluginProtos.CodeGeneratorRequest
 import com.google.protobuf.compiler.PluginProtos.CodeGeneratorResponse
+import com.google.protobuf.compiler.PluginProtos.CodeGeneratorResponse.Feature
 import com.toasttab.protokt.codegen.generate
 import com.toasttab.protokt.codegen.impl.STAnnotator
 import com.toasttab.protokt.codegen.impl.STEffects
@@ -51,6 +52,7 @@ internal fun main(bytes: ByteArray, out: OutputStream) {
 
     if (files.nonEmpty()) {
         CodeGeneratorResponse.newBuilder()
+            .setSupportedFeatures(Feature.FEATURE_PROTO3_OPTIONAL.number.toLong())
             .addAllFile(files)
             .build()
             .writeTo(out)
diff --git a/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/impl/MessageAnnotator.kt b/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/impl/MessageAnnotator.kt
@@ -24,7 +24,7 @@ import com.toasttab.protokt.codegen.impl.Implements.doesImplement
 import com.toasttab.protokt.codegen.impl.Implements.implements
 import com.toasttab.protokt.codegen.impl.MapEntryAnnotator.Companion.annotateMapEntry
 import com.toasttab.protokt.codegen.impl.MessageDocumentationAnnotator.annotateMessageDocumentation
-import com.toasttab.protokt.codegen.impl.OneofAnnotator.Companion.annotateOneOfs
+import com.toasttab.protokt.codegen.impl.OneofAnnotator.Companion.annotateOneofs
 import com.toasttab.protokt.codegen.impl.PropertyAnnotator.Companion.annotateProperties
 import com.toasttab.protokt.codegen.impl.STAnnotator.Context
 import com.toasttab.protokt.codegen.impl.STAnnotator.annotate
@@ -49,7 +49,7 @@ internal object MessageAnnotator {
             MessageTemplate.render(
                 message = messageInfo(msg, ctx),
                 properties = annotateProperties(msg, ctx),
-                oneofs = annotateOneOfs(msg, ctx),
+                oneofs = annotateOneofs(msg, ctx),
                 sizeof = annotateSizeof(msg, ctx),
                 serialize = annotateSerializer(msg, ctx),
                 deserialize = annotateDeserializer(msg, ctx),
diff --git a/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/impl/Nullability.kt b/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/impl/Nullability.kt
@@ -36,7 +36,7 @@ internal object Nullability {
     private val Field.isKotlinRepresentationNullable
         get() =
             when (this) {
-                is StandardField -> type == FieldType.MESSAGE && !repeated
+                is StandardField -> (type == FieldType.MESSAGE && !repeated) || optional
                 is Oneof -> true
             }
 
diff --git a/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/impl/OneofAnnotator.kt b/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/impl/OneofAnnotator.kt
@@ -34,7 +34,7 @@ private constructor(
     private val msg: Message,
     private val ctx: Context
 ) {
-    private fun annotateOneOfs() =
+    private fun annotateOneofs() =
         msg.fields.map {
             when (it) {
                 is Oneof ->
@@ -152,7 +152,7 @@ private constructor(
         msg.fields.filterIsInstance<Oneof>().map { it.name }
 
     companion object {
-        fun annotateOneOfs(msg: Message, ctx: Context) =
-            OneofAnnotator(msg, ctx).annotateOneOfs()
+        fun annotateOneofs(msg: Message, ctx: Context) =
+            OneofAnnotator(msg, ctx).annotateOneofs()
     }
 }
diff --git a/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/impl/PropertyAnnotator.kt b/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/impl/PropertyAnnotator.kt
@@ -59,7 +59,7 @@ private constructor(
                             fieldType = it.type.toString(),
                             repeated = it.repeated,
                             map = it.map,
-                            nullable = it.nullable,
+                            nullable = it.nullable || it.optional,
                             nonNullOption = it.hasNonNullOption,
                             overrides = it.overrides(ctx, msg),
                             wrapped = it.wrapped,
diff --git a/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/protoc/Protocol.kt b/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/protoc/Protocol.kt
@@ -48,7 +48,6 @@ fun toProtocol(ctx: ProtocolContext) =
         FileDesc(
             name = ctx.fdp.name,
             packageName = ctx.fdp.`package`,
-            version = ctx.fdp.syntax?.removePrefix("proto")?.toIntOrNull() ?: 2,
             options = ctx.fdp.fileOptions,
             context = ctx,
             sourceCodeInfo = ctx.fdp.sourceCodeInfo
@@ -270,8 +269,13 @@ private fun toOneof(
     field: FieldDescriptorProto,
     typeNames: Set<String>,
     fields: ImmutableList<Field>
-): Oneof {
+): Field {
     val newName = newFieldName(oneof.name, typeNames)
+
+    if (field.proto3Optional) {
+        return toStandard(idx, ctx, field, typeNames)
+    }
+
     val standardTuple = desc.fieldList.filter {
         it.hasOneofIndex() && it.oneofIndex == field.oneofIndex
     }.foldIndexed(
@@ -309,22 +313,25 @@ private fun toStandard(
     usedFieldNames: Set<String>,
     alwaysRequired: Boolean = false
 ): StandardField =
-    toFieldType(checkNotNull(fdp.type) { "Missing field type" }).let { type ->
+    toFieldType(fdp.type).let { type ->
         StandardField(
             number = fdp.number,
             name = newFieldName(fdp.name, usedFieldNames),
             type = type,
             repeated = fdp.label == LABEL_REPEATED,
-            optional = !alwaysRequired && fdp.label == LABEL_OPTIONAL,
+            optional =
+                !alwaysRequired &&
+                    (fdp.label == LABEL_OPTIONAL && ctx.proto2) ||
+                    fdp.proto3Optional,
             packed =
                 type.packable &&
                     // marginal support for proto2
-                    ((ctx.fdp.syntax == "proto2" && fdp.options.packed) ||
+                    ((ctx.proto2 && fdp.options.packed) ||
                         // packed if: proto3 and `packed` isn't set, or proto3
                         // and `packed` is true. If proto3, only explicitly
                         // setting `packed` to false disables packing, since
                         // the default value for an unset boolean is false.
-                        (ctx.fdp.syntax == "proto3" &&
+                        (ctx.proto3 &&
                             (!fdp.options.hasPacked() ||
                                 (fdp.options.hasPacked() && fdp.options.packed)))),
             mapEntry =
diff --git a/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/protoc/ProtocolContext.kt b/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/protoc/ProtocolContext.kt
@@ -35,6 +35,9 @@ class ProtocolContext(
 
     val fileName = fdp.name
     val version = getProtoktVersion(ProtocolContext::class)
+
+    val proto2 = !fdp.hasSyntax() || fdp.syntax == "proto2"
+    val proto3 = fdp.syntax == "proto3"
 }
 
 fun respectJavaPackage(params: Map<String, String>) =
diff --git a/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/protoc/Types.kt b/protokt-codegen/src/main/kotlin/com/toasttab/protokt/codegen/protoc/Types.kt
@@ -142,7 +142,6 @@ class OneofOptions(
 class FileDesc(
     val name: String,
     val packageName: String,
-    val version: Int,
     val options: FileOptions,
     val context: ProtocolContext,
     val sourceCodeInfo: DescriptorProtos.SourceCodeInfo
diff --git a/protokt-codegen/src/main/resources/dsl.stg b/protokt-codegen/src/main/resources/dsl.stg
@@ -71,7 +71,7 @@ dslValue(p) ::= <%
         emptyMap()
     <elseif (p.repeated)>
         emptyList()
-    <elseif (isMessage.(p.fieldType) || p.wrapped)>
+    <elseif (isMessage.(p.fieldType) || p.wrapped || p.nullable)>
         null
     <else>
         <p.defaultValue>
diff --git a/protokt-codegen/src/main/resources/renderers.stg b/protokt-codegen/src/main/resources/renderers.stg
@@ -111,7 +111,7 @@ deserializeType(p) ::= <%
 %>
 
 deserializeValue(p) ::= <%
-    <if (p.repeated || isMessage.(p.fieldType) || p.wrapped)>
+    <if (p.repeated || isMessage.(p.fieldType) || p.wrapped || p.nullable)>
         null
     <else>
         <p.defaultValue>
@@ -159,19 +159,29 @@ defaultValue(field, type, name) ::= <%
         emptyList()
     <elseif (isGeneratedType.(type))>
         <name><kotlinDefaultValues.(type)>
+    <elseif (field.nullable)>
+        null
     <else>
         <kotlinDefaultValues.(type)>
     <endif>
 %>
 
 nonDefaultValue(field, name) ::= <%
-    <if (field.repeated)>(<field.fieldName>.isNotEmpty())<\ >
-    <elseif (isBytes.(field.type) || isString.(field.type))>(<name>.isNotEmpty())<\ >
-    <elseif (isMessage.(field.type))>(<field.fieldName> != null)<\ >
-    <elseif (isEnum.(field.type))>(<name>.value != 0)<\ >
+    <if ((isMessage.(field.type) && !field.repeated) || field.optional)>
+        (<field.fieldName> != null)<\ >
+    <elseif (field.repeated)>
+        (<field.fieldName>.isNotEmpty())<\ >
+    <elseif (isBytes.(field.type) || isString.(field.type))>
+        (<proto2Check()><name>.isNotEmpty())<\ >
+    <elseif (isMessage.(field.type))>
+        (<field.fieldName> != null)<\ >
+    <elseif (isEnum.(field.type))>
+        (<name>.value != 0)<\ >
     <elseif (field.type.scalar)>
-        <if (isBool.(field.type))>(<name>)<\ >
-        <else>(<name> != <kotlinDefaultValues.(field.type)>)<\ >
+        <if (isBool.(field.type))>
+            (<name>)<\ >
+        <else>
+            (<name> != <kotlinDefaultValues.(field.type)>)<\ >
         <endif>
     <endif>
 %>
diff --git a/testing/options/src/test/kotlin/com/toasttab/protokt/testing/options/NonNullableTest.kt b/testing/options/src/test/kotlin/com/toasttab/protokt/testing/options/NonNullableTest.kt
@@ -25,14 +25,14 @@ class NonNullableTest {
     fun `test declared nullability`() {
         assertThat(
             NonNullModel::class.declaredMemberProperties
-            .first { it.name == "nonNullStringValue" }
+            .single { it.name == "nonNullStringValue" }
             .returnType
             .isMarkedNullable
         ).isFalse()
 
         assertThat(
             NonNullModel::class.declaredMemberProperties
-            .first { it.name == "nonNullOneof" }
+            .single { it.name == "nonNullOneof" }
             .returnType
             .isMarkedNullable
         ).isFalse()
diff --git a/testing/runtime-tests/src/main/proto/com/toasttab/protokt/testing/rt/test_proto3_optional.proto b/testing/runtime-tests/src/main/proto/com/toasttab/protokt/testing/rt/test_proto3_optional.proto
@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2020 Toast Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+syntax = "proto3";
+
+package com.toasttab.protokt.testing.rt;
+
+message TestProto3Optional {
+  optional int32 optional_int32 = 1;
+  optional string optional_string = 2;
+
+  // doesn't do anything, but must be supported
+  optional Foo optional_foo = 3;
+}
+
+message Foo {}
diff --git a/testing/runtime-tests/src/test/kotlin/com/toasttab/protokt/testing/rt/Proto3OptionalTest.kt b/testing/runtime-tests/src/test/kotlin/com/toasttab/protokt/testing/rt/Proto3OptionalTest.kt
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2020 Toast Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.toasttab.protokt.testing.rt
+
+import com.google.common.truth.Truth.assertThat
+import kotlin.reflect.full.declaredMemberProperties
+import org.junit.jupiter.api.Test
+
+class Proto3OptionalTest {
+    @Test
+    fun `optional scalar fields should be nullable`() {
+        assertThat(
+            TestProto3Optional::class.declaredMemberProperties
+                .single { it.name == "optionalInt32" }
+                .returnType
+                .isMarkedNullable
+        ).isTrue()
+
+        assertThat(
+            TestProto3Optional::class.declaredMemberProperties
+                .single { it.name == "optionalString" }
+                .returnType
+                .isMarkedNullable
+        ).isTrue()
+
+        assertThat(
+            TestProto3Optional::class.declaredMemberProperties
+                .single { it.name == "optionalFoo" }
+                .returnType
+                .isMarkedNullable
+        ).isTrue()
+    }
+
+    @Test
+    fun `optional fields serialize otherwise default values`() {
+        val serialized =
+            TestProto3Optional {
+                optionalInt32 = 0
+                optionalString = ""
+            }.serialize()
+
+        val deserialized = TestProto3Optional.deserialize(serialized)
+
+        assertThat(deserialized.optionalInt32).isEqualTo(0)
+        assertThat(deserialized.optionalString).isEqualTo("")
+    }
+
+    @Test
+    fun `optional fields don't serialize default values for nulls`() {
+        val serialized = TestProto3Optional { }.serialize()
+
+        val deserialized = TestProto3Optional.deserialize(serialized)
+
+        assertThat(deserialized.optionalInt32).isNull()
+        assertThat(deserialized.optionalString).isNull()
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ internal object Nullability {`
`36`	`36`	`private val Field.isKotlinRepresentationNullable`
`37`	`37`	`get() =`
`38`	`38`	`when (this) {`
`39`		`- is StandardField -> type == FieldType.MESSAGE && !repeated`
	`39`	`+ is StandardField -> (type == FieldType.MESSAGE && !repeated) \|\| optional`
`40`	`40`	`is Oneof -> true`
`41`	`41`	`}`
`42`	`42`
Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,9 @@ class ProtocolContext(`
`35`	`35`
`36`	`36`	`val fileName = fdp.name`
`37`	`37`	`val version = getProtoktVersion(ProtocolContext::class)`
	`38`	`+`
	`39`	`+ val proto2 = !fdp.hasSyntax() \|\| fdp.syntax == "proto2"`
	`40`	`+ val proto3 = fdp.syntax == "proto3"`
`38`	`41`	`}`
`39`	`42`
`40`	`43`	`fun respectJavaPackage(params: Map<String, String>) =`