fix: optional handling in the JVM

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 · 4db6b4d · 4db6b4d
1 parent 4c33afd
commit 4db6b4d
Show file tree

Hide file tree

Showing 11 changed files with 161 additions and 52 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/ModuleBuilder.java b/...e/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleBuilder.java
@@ -26,6 +26,7 @@
 import org.jboss.jandex.PrimitiveType;
 import org.jboss.jandex.VoidType;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
 import com.fasterxml.jackson.annotation.JsonAlias;
 import com.fasterxml.jackson.databind.JsonNode;
@@ -72,6 +73,7 @@ public class ModuleBuilder {
     public static final DotName INSTANT = DotName.createSimple(Instant.class);
     public static final DotName ZONED_DATE_TIME = DotName.createSimple(ZonedDateTime.class);
     public static final DotName NOT_NULL = DotName.createSimple(NotNull.class);
+    public static final DotName NULLABLE = DotName.createSimple(Nullable.class);
     public static final DotName JSON_NODE = DotName.createSimple(JsonNode.class.getName());
     public static final DotName OFFSET_DATE_TIME = DotName.createSimple(OffsetDateTime.class.getName());
     public static final DotName GENERATED_REF = DotName.createSimple(GeneratedRef.class);
@@ -89,10 +91,11 @@ public class ModuleBuilder {
     private final FTLRecorder recorder;
     private final Map<String, Iterable<String>> comments;
     private final List<ValidationFailure> validationFailures = new ArrayList<>();
+    private final boolean defaultToOptional;
 
     public ModuleBuilder(IndexView index, String moduleName, Map<DotName, TopicsBuildItem.DiscoveredTopic> knownTopics,
             Map<DotName, VerbClientBuildItem.DiscoveredClients> verbClients, FTLRecorder recorder,
-            Map<String, Iterable<String>> comments, Map<TypeKey, ExistingRef> typeAliases) {
+            Map<String, Iterable<String>> comments, Map<TypeKey, ExistingRef> typeAliases, boolean defaultToOptional) {
         this.index = index;
         this.moduleName = moduleName;
         this.moduleBuilder = Module.newBuilder()
@@ -103,6 +106,7 @@ public ModuleBuilder(IndexView index, String moduleName, Map<DotName, TopicsBuil
         this.recorder = recorder;
         this.comments = comments;
         this.dataElements = new HashMap<>(typeAliases);
+        this.defaultToOptional = defaultToOptional;
     }
 
     public static @NotNull String methodToName(MethodInfo method) {
@@ -245,15 +249,15 @@ public void registerVerbMethod(MethodInfo method, String className,
                 verbBuilder.addMetadata(Metadata.newBuilder().setCalls(callsMetadata));
             }
 
-            //TODO: we need better handling around Optional
             recorder.registerVerb(moduleName, verbName, method.name(), parameterTypes,
                     Class.forName(className, false, Thread.currentThread().getContextClassLoader()), paramMappers,
                     method.returnType() == VoidType.VOID);
+
             verbBuilder
                     .setName(verbName)
                     .setExport(exported)
-                    .setRequest(buildType(bodyParamType, exported))
-                    .setResponse(buildType(method.returnType(), exported));
+                    .setRequest(buildNullableType(bodyParamType, exported))
+                    .setResponse(buildNullableType(method.returnType(), exported));
             Optional.ofNullable(comments.get(CommentKey.ofVerb(verbName)))
                     .ifPresent(verbBuilder::addAllComments);
 
@@ -269,6 +273,22 @@ public void registerVerbMethod(MethodInfo method, String className,
         }
     }
 
+    public Type buildNullableType(org.jboss.jandex.Type type, boolean export) {
+        var res = buildType(type, export);
+        if (res.hasOptional()) {
+            return res;
+        }
+        if (type.hasAnnotation(NOT_NULL)) {
+            return res;
+        } else if (type.hasAnnotation(NULLABLE) || defaultToOptional) {
+            return Type.newBuilder().setOptional(xyz.block.ftl.v1.schema.Optional.newBuilder()
+                    .setType(res))
+                    .build();
+
+        }
+        return res;
+    }
+
     public Type buildType(org.jboss.jandex.Type type, boolean export) {
         switch (type.kind()) {
             case PRIMITIVE -> {

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/...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,28 @@ 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)...)
 
 	in.Run(t,
 		in.WithJavaBuild(),
@@ -212,6 +211,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 +268,14 @@ 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")
+	})
+}
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);
     }