fix: optional handling in the JVM (#2951)

For Kotlin this should exactly match the language semantics. For Java types are not considered Optional unless they are wrapped in a java.util.Optional, or annotated with @nullable. The exception to this are the boxed primitive types, which are always considered nullable. fixes: #2356
block · Oct 2, 2024 · ddc902c · ddc902c
1 parent 3656949
commit ddc902c
Show file tree

Hide file tree

Showing 14 changed files with 245 additions and 95 deletions.
diff --git a/internal/integration/actions.go b/internal/integration/actions.go
@@ -417,6 +417,32 @@ func VerifySchema(check func(ctx context.Context, t testing.TB, sch *schemapb.Sc
 	}
 }
 
+// VerifySchemaVerb lets you test the current schema for a specific verb
+func VerifySchemaVerb(module string, verb string, check func(ctx context.Context, t testing.TB, sch *schemapb.Verb)) Action {
+	return func(t testing.TB, ic TestContext) {
+		sch, err := ic.Controller.GetSchema(ic, connect.NewRequest(&ftlv1.GetSchemaRequest{}))
+		if err != nil {
+			t.Errorf("failed to get schema: %v", err)
+			return
+		}
+		if err != nil {
+			t.Errorf("failed to deserialize schema: %v", err)
+			return
+		}
+		for _, m := range sch.Msg.GetSchema().Modules {
+			if m.Name == module {
+				for _, v := range m.Decls {
+					if v.GetVerb() != nil && v.GetVerb().Name == verb {
+						check(ic.Context, t, v.GetVerb())
+						return
+					}
+				}
+			}
+
+		}
+	}
+}
+
 // Fail expects the next action to Fail.
 func Fail(next Action, msg string, args ...any) Action {
 	return func(t testing.TB, ic TestContext) {

diff --git a/...me/common/deployment/src/main/java/xyz/block/ftl/deployment/DefaultOptionalBuildItem.java b/...me/common/deployment/src/main/java/xyz/block/ftl/deployment/DefaultOptionalBuildItem.java
@@ -0,0 +1,20 @@
+package xyz.block.ftl.deployment;
+
+import io.quarkus.builder.item.SimpleBuildItem;
+
+/**
+ * Build item that indicates if a type with no nullability information should default to optional.
+ *
+ * This is different between Kotlin and Java
+ */
+public final class DefaultOptionalBuildItem extends SimpleBuildItem {
+    final boolean defaultToOptional;
+
+    public DefaultOptionalBuildItem(boolean defaultToOptional) {
+        this.defaultToOptional = defaultToOptional;
+    }
+
+    public boolean isDefaultToOptional() {
+        return defaultToOptional;
+    }
+}
diff --git a/...e/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/HTTPProcessor.java b/...e/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/HTTPProcessor.java
@@ -157,8 +157,9 @@ public void accept(ModuleBuilder moduleBuilder) {
                             .setIngress(ingressBuilder
                                     .build())
                             .build();
-                    Type requestTypeParam = moduleBuilder.buildType(bodyParamType, true);
-                    Type responseTypeParam = moduleBuilder.buildType(endpoint.getMethodInfo().returnType(), true);
+                    Type requestTypeParam = moduleBuilder.buildType(bodyParamType, true, Nullability.NOT_NULL);
+                    Type responseTypeParam = moduleBuilder.buildType(endpoint.getMethodInfo().returnType(), true,
+                            Nullability.NOT_NULL);
                     Type stringType = Type.newBuilder().setString(xyz.block.ftl.v1.schema.String.newBuilder().build()).build();
                     Type pathParamType = Type.newBuilder()
                             .setMap(xyz.block.ftl.v1.schema.Map.newBuilder().setKey(stringType)

diff --git a/...e/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleBuilder.java b/...e/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleBuilder.java
diff --git a/...ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleProcessor.java b/...ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleProcessor.java
@@ -112,6 +112,7 @@ public void generateSchema(CombinedIndexBuildItem index,
             TopicsBuildItem topicsBuildItem,
             VerbClientBuildItem verbClientBuildItem,
             List<TypeAliasBuildItem> typeAliasBuildItems,
+            DefaultOptionalBuildItem defaultOptionalBuildItem,
             List<SchemaContributorBuildItem> schemaContributorBuildItems) throws Exception {
         String moduleName = moduleNameBuildItem.getModuleName();
         Map<String, Iterable<String>> comments = readComments();
@@ -133,7 +134,8 @@ public void generateSchema(CombinedIndexBuildItem index,
         }
 
         ModuleBuilder moduleBuilder = new ModuleBuilder(index.getComputingIndex(), moduleName, topicsBuildItem.getTopics(),
-                verbClientBuildItem.getVerbClients(), recorder, comments, existingRefs);
+                verbClientBuildItem.getVerbClients(), recorder, comments, existingRefs,
+                defaultOptionalBuildItem.isDefaultToOptional());
 
         for (var i : schemaContributorBuildItems) {
             i.getSchemaContributor().accept(moduleBuilder);

diff --git a/...ime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/Nullability.java b/...ime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/Nullability.java
@@ -0,0 +1,7 @@
+package xyz.block.ftl.deployment;
+
+public enum Nullability {
+    MISSING,
+    NULLABLE,
+    NOT_NULL
+}
diff --git a/...ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/TopicsProcessor.java b/...ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/TopicsProcessor.java
@@ -90,7 +90,8 @@ public void accept(ModuleBuilder moduleBuilder) {
                     moduleBuilder.addDecls(Decl.newBuilder().setTopic(xyz.block.ftl.v1.schema.Topic.newBuilder()
                             .setExport(topic.exported())
                             .setName(topic.topicName())
-                            .setEvent(moduleBuilder.buildType(topic.eventType(), topic.exported())).build()).build());
+                            .setEvent(moduleBuilder.buildType(topic.eventType(), topic.exported(), Nullability.NOT_NULL))
+                            .build()).build());
                 }
             }
         });

diff --git a/...tl-runtime/common/runtime/src/main/java/xyz/block/ftl/runtime/config/FTLConfigSource.java b/...tl-runtime/common/runtime/src/main/java/xyz/block/ftl/runtime/config/FTLConfigSource.java
@@ -96,7 +96,6 @@ public String getValue(String s) {
             }
         }
         if (s.startsWith("quarkus.datasource")) {
-            System.out.println("prop: " + s);
             switch (s) {
                 case DEFAULT_USER -> {
                     return Optional.ofNullable(controller.getDatasource("default")).map(FTLController.Datasource::username)
@@ -115,19 +114,16 @@ public String getValue(String s) {
             }
             Matcher m = USER_PATTERN.matcher(s);
             if (m.matches()) {
-                System.out.println("match: " + s);
                 return Optional.ofNullable(controller.getDatasource(m.group(1))).map(FTLController.Datasource::username)
                         .orElse(null);
             }
             m = PASSWORD_PATTERN.matcher(s);
             if (m.matches()) {
-                System.out.println("match: " + s);
                 return Optional.ofNullable(controller.getDatasource(m.group(1))).map(FTLController.Datasource::password)
                         .orElse(null);
             }
             m = URL_PATTERN.matcher(s);
             if (m.matches()) {
-                System.out.println("match: " + s);
                 return Optional.ofNullable(controller.getDatasource(m.group(1))).map(FTLController.Datasource::connectionString)
                         .orElse(null);
             }

diff --git a/...ime/java/deployment/src/main/java/xyz/block/ftl/javalang/deployment/FTLJavaProcessor.java b/...ime/java/deployment/src/main/java/xyz/block/ftl/javalang/deployment/FTLJavaProcessor.java
@@ -0,0 +1,11 @@
+package xyz.block.ftl.javalang.deployment;
+
+import io.quarkus.deployment.annotations.BuildStep;
+import xyz.block.ftl.deployment.DefaultOptionalBuildItem;
+
+public class FTLJavaProcessor {
+    @BuildStep
+    public DefaultOptionalBuildItem defaultOptional() {
+        return new DefaultOptionalBuildItem(false);
+    }
+}
diff --git a/...e/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/FTLKotlinProcessor.java b/...e/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/FTLKotlinProcessor.java
@@ -0,0 +1,11 @@
+package xyz.block.ftl.kotlin.deployment;
+
+import io.quarkus.deployment.annotations.BuildStep;
+import xyz.block.ftl.deployment.DefaultOptionalBuildItem;
+
+public class FTLKotlinProcessor {
+    @BuildStep
+    public DefaultOptionalBuildItem defaultOptional() {
+        return new DefaultOptionalBuildItem(true);
+    }
+}
diff --git a/.../kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/KotlinCodeGenerator.java b/.../kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/KotlinCodeGenerator.java
@@ -210,8 +210,7 @@ private TypeName toKotlinTypeName(Type type, Map<DeclRef, Type> typeAliasMap, Ma
         } else if (type.hasString()) {
             return new ClassName("kotlin", "String");
         } else if (type.hasOptional()) {
-            // Always box for optional, as normal primities can't be null
-            return toKotlinTypeName(type.getOptional().getType(), typeAliasMap, nativeTypeAliasMap);
+            return toKotlinTypeName(type.getOptional().getType(), typeAliasMap, nativeTypeAliasMap).copy(true, List.of());
         } else if (type.hasRef()) {
             if (type.getRef().getModule().isEmpty()) {
                 return TypeVariableName.get(type.getRef().getName());

diff --git a/jvm-runtime/jvm_integration_test.go b/jvm-runtime/jvm_integration_test.go
@@ -148,29 +148,36 @@ func TestJVMCoreFunctionality(t *testing.T) {
 	// tests = append(tests, PairedPrefixVerbTest("nilvalue", "optionalTestObjectOptionalFieldsVerb", ftl.None[any]())...)
 
 	// Schema comments
-	tests = append(tests, in.SubTest{Name: "schemaComments", Action: in.VerifySchema(func(ctx context.Context, t testing.TB, sch *schemapb.Schema) {
-		javaOk := false
-		kotlinOk := false
-		for _, module := range sch.Modules {
-			if module.Name == "gomodule" {
-				continue
-			}
-			for _, decl := range module.Decls {
-				if decl.GetVerb() != nil {
-					for _, comment := range decl.GetVerb().GetComments() {
-						if strings.Contains(comment, "JAVA COMMENT") {
-							javaOk = true
-						}
-						if strings.Contains(comment, "KOTLIN COMMENT") {
-							kotlinOk = true
-						}
-					}
+	tests = append(tests, JVMTest("schemaComments", func(name string, module string) in.Action {
+		return in.VerifySchemaVerb(module, "emptyVerb", func(ctx context.Context, t testing.TB, verb *schemapb.Verb) {
+			ok := false
+			for _, comment := range verb.GetComments() {
+				if strings.Contains(comment, "JAVA COMMENT") {
+					ok = true
+				}
+				if strings.Contains(comment, "KOTLIN COMMENT") {
+					ok = true
 				}
 			}
-		}
-		assert.True(t, javaOk, "java comment not found")
-		assert.True(t, kotlinOk, "kotlin comment not found")
-	})})
+			assert.True(t, ok, "comment not found")
+		})
+	})...)
+	tests = append(tests, JVMTest("optionalIntVerb", verifyOptionalVerb)...)
+	tests = append(tests, JVMTest("optionalFloatVerb", verifyOptionalVerb)...)
+	tests = append(tests, JVMTest("optionalStringVerb", verifyOptionalVerb)...)
+	tests = append(tests, JVMTest("optionalBytesVerb", verifyOptionalVerb)...)
+	tests = append(tests, JVMTest("optionalStringArrayVerb", verifyOptionalVerb)...)
+	tests = append(tests, JVMTest("optionalStringMapVerb", verifyOptionalVerb)...)
+	tests = append(tests, JVMTest("optionalTimeVerb", verifyOptionalVerb)...)
+	tests = append(tests, JVMTest("optionalTestObjectVerb", verifyOptionalVerb)...)
+	tests = append(tests, JVMTest("intVerb", verifyNonOptionalVerb)...)
+	tests = append(tests, JVMTest("floatVerb", verifyNonOptionalVerb)...)
+	tests = append(tests, JVMTest("stringVerb", verifyNonOptionalVerb)...)
+	tests = append(tests, JVMTest("bytesVerb", verifyNonOptionalVerb)...)
+	tests = append(tests, JVMTest("stringArrayVerb", verifyNonOptionalVerb)...)
+	tests = append(tests, JVMTest("stringMapVerb", verifyNonOptionalVerb)...)
+	tests = append(tests, JVMTest("timeVerb", verifyNonOptionalVerb)...)
+	tests = append(tests, JVMTest("testObjectVerb", verifyNonOptionalVerb)...)
 
 	in.Run(t,
 		in.WithJavaBuild(),
@@ -212,6 +219,19 @@ func PairedTest(name string, testFunc func(module string) in.Action) []in.SubTes
 	}
 }
 
+func JVMTest(name string, testFunc func(name string, module string) in.Action) []in.SubTest {
+	return []in.SubTest{
+		{
+			Name:   name + "-java",
+			Action: testFunc(name, "javamodule"),
+		},
+		{
+			Name:   name + "-kotlin",
+			Action: testFunc(name, "kotlinmodule"),
+		},
+	}
+}
+
 func VerbTest[T any](verb string, value T) func(module string) in.Action {
 	return func(module string) in.Action {
 		return in.Call(module, verb, value, func(t testing.TB, response T) {
@@ -256,3 +276,21 @@ type ParameterizedType[T any] struct {
 	Option ftl.Option[T] `json:"option"`
 	Map    map[string]T  `json:"map"`
 }
+
+func subTest(name string, test in.Action) in.Action {
+	return in.SubTests(in.SubTest{Name: name, Action: test})
+}
+
+func verifyOptionalVerb(name string, module string) in.Action {
+	return in.VerifySchemaVerb(module, name, func(ctx context.Context, t testing.TB, verb *schemapb.Verb) {
+		assert.True(t, verb.Response.GetOptional() != nil, "response not optional")
+		assert.True(t, verb.Request.GetOptional() != nil, "request not optional")
+	})
+}
+
+func verifyNonOptionalVerb(name string, module string) in.Action {
+	return in.VerifySchemaVerb(module, name, func(ctx context.Context, t testing.TB, verb *schemapb.Verb) {
+		assert.True(t, verb.Response.GetOptional() == nil, "response was optional")
+		assert.True(t, verb.Request.GetOptional() == nil, "request was optional")
+	})
+}
diff --git a/...ntime/testdata/java/javamodule/src/main/java/xyz/block/ftl/test/TestInvokeGoFromJava.java b/...ntime/testdata/java/javamodule/src/main/java/xyz/block/ftl/test/TestInvokeGoFromJava.java
@@ -5,6 +5,7 @@
 import java.util.Map;
 
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
 import ftl.gomodule.BoolVerbClient;
 import ftl.gomodule.BytesVerbClient;
@@ -43,6 +44,9 @@
 
 public class TestInvokeGoFromJava {
 
+    /**
+     * JAVA COMMENT
+     */
     @Export
     @Verb
     public void emptyVerb(EmptyVerbClient emptyVerbClient) {
@@ -61,9 +65,6 @@ public String sourceVerb(SourceVerbClient sourceVerbClient) {
         return sourceVerbClient.call();
     }
 
-    /**
-     * JAVA COMMENT
-     */
     @Export
     @Verb
     public void errorEmptyVerb(ErrorEmptyVerbClient client) {
@@ -166,43 +167,43 @@ public Double optionalFloatVerb(Double val, OptionalFloatVerbClient client) {
 
     @Export
     @Verb
-    public String optionalStringVerb(String val, OptionalStringVerbClient client) {
+    public @Nullable String optionalStringVerb(@Nullable String val, OptionalStringVerbClient client) {
         return client.call(val);
     }
 
     @Export
     @Verb
-    public byte[] optionalBytesVerb(byte[] val, OptionalBytesVerbClient client) {
+    public byte @Nullable [] optionalBytesVerb(byte @Nullable [] val, OptionalBytesVerbClient client) {
         return client.call(val);
     }
 
     @Export
     @Verb
-    public boolean optionalBoolVerb(boolean val, OptionalBoolVerbClient client) {
+    public Boolean optionalBoolVerb(Boolean val, OptionalBoolVerbClient client) {
         return client.call(val);
     }
 
     @Export
     @Verb
-    public List<String> optionalStringArrayVerb(List<String> val, OptionalStringArrayVerbClient client) {
+    public @Nullable List<String> optionalStringArrayVerb(@Nullable List<String> val, OptionalStringArrayVerbClient client) {
         return client.call(val);
     }
 
     @Export
     @Verb
-    public Map<String, String> optionalStringMapVerb(Map<String, String> val, OptionalStringMapVerbClient client) {
+    public @Nullable Map<String, String> optionalStringMapVerb(@Nullable Map<String, String> val, OptionalStringMapVerbClient client) {
         return client.call(val);
     }
 
     @Export
     @Verb
-    public ZonedDateTime optionalTimeVerb(ZonedDateTime instant, OptionalTimeVerbClient client) {
+    public @Nullable ZonedDateTime optionalTimeVerb(@Nullable ZonedDateTime instant, OptionalTimeVerbClient client) {
         return client.call(instant);
     }
 
     @Export
     @Verb
-    public TestObject optionalTestObjectVerb(TestObject val, OptionalTestObjectVerbClient client) {
+    public @Nullable TestObject optionalTestObjectVerb(@Nullable TestObject val, OptionalTestObjectVerbClient client) {
         return client.call(val);
     }