From 4db6b4d5bab07a9525b3dd89d69a0a38fb999797 Mon Sep 17 00:00:00 2001 From: Stuart Douglas Date: Wed, 2 Oct 2024 14:11:40 +1000 Subject: [PATCH] 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 --- internal/integration/actions.go | 26 ++++++++ .../deployment/DefaultOptionalBuildItem.java | 20 ++++++ .../block/ftl/deployment/ModuleBuilder.java | 28 ++++++-- .../block/ftl/deployment/ModuleProcessor.java | 4 +- .../ftl/runtime/config/FTLConfigSource.java | 4 -- .../javalang/deployment/FTLJavaProcessor.java | 11 ++++ .../kotlin/deployment/FTLKotlinProcessor.java | 11 ++++ .../deployment/KotlinCodeGenerator.java | 3 +- jvm-runtime/jvm_integration_test.go | 65 +++++++++++++------ .../block/ftl/test/TestInvokeGoFromJava.java | 21 +++--- .../block/ftl/test/TestInvokeGoFromKotlin.kt | 20 +++--- 11 files changed, 161 insertions(+), 52 deletions(-) create mode 100644 jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/DefaultOptionalBuildItem.java create mode 100644 jvm-runtime/ftl-runtime/java/deployment/src/main/java/xyz/block/ftl/javalang/deployment/FTLJavaProcessor.java create mode 100644 jvm-runtime/ftl-runtime/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/FTLKotlinProcessor.java diff --git a/internal/integration/actions.go b/internal/integration/actions.go index eacc70969e..34e6f501ce 100644 --- 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/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/DefaultOptionalBuildItem.java b/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/DefaultOptionalBuildItem.java new file mode 100644 index 0000000000..0d3a54ca1f --- /dev/null +++ b/jvm-runtime/ftl-runtime/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/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleBuilder.java b/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleBuilder.java index 607a7dcf3c..6a8f8abee9 100644 --- a/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleBuilder.java +++ b/jvm-runtime/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> comments; private final List validationFailures = new ArrayList<>(); + private final boolean defaultToOptional; public ModuleBuilder(IndexView index, String moduleName, Map knownTopics, Map verbClients, FTLRecorder recorder, - Map> comments, Map typeAliases) { + Map> comments, Map typeAliases, boolean defaultToOptional) { this.index = index; this.moduleName = moduleName; this.moduleBuilder = Module.newBuilder() @@ -103,6 +106,7 @@ public ModuleBuilder(IndexView index, String moduleName, Map(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/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleProcessor.java b/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleProcessor.java index bd2c31c4c9..3a3ddee870 100644 --- a/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/ModuleProcessor.java +++ b/jvm-runtime/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 typeAliasBuildItems, + DefaultOptionalBuildItem defaultOptionalBuildItem, List schemaContributorBuildItems) throws Exception { String moduleName = moduleNameBuildItem.getModuleName(); Map> 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/jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/runtime/config/FTLConfigSource.java b/jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/runtime/config/FTLConfigSource.java index a3d85f5064..ae7d941c9e 100644 --- a/jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/runtime/config/FTLConfigSource.java +++ b/jvm-runtime/ftl-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/jvm-runtime/ftl-runtime/java/deployment/src/main/java/xyz/block/ftl/javalang/deployment/FTLJavaProcessor.java b/jvm-runtime/ftl-runtime/java/deployment/src/main/java/xyz/block/ftl/javalang/deployment/FTLJavaProcessor.java new file mode 100644 index 0000000000..b60c463ad3 --- /dev/null +++ b/jvm-runtime/ftl-runtime/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/jvm-runtime/ftl-runtime/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/FTLKotlinProcessor.java b/jvm-runtime/ftl-runtime/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/FTLKotlinProcessor.java new file mode 100644 index 0000000000..07f1eb05b8 --- /dev/null +++ b/jvm-runtime/ftl-runtime/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/jvm-runtime/ftl-runtime/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/KotlinCodeGenerator.java b/jvm-runtime/ftl-runtime/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/KotlinCodeGenerator.java index 038d8f164e..ab78bef0ef 100644 --- a/jvm-runtime/ftl-runtime/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/KotlinCodeGenerator.java +++ b/jvm-runtime/ftl-runtime/kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/KotlinCodeGenerator.java @@ -210,8 +210,7 @@ private TypeName toKotlinTypeName(Type type, Map 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 index 4870858039..5090873b9f 100644 --- 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/jvm-runtime/testdata/java/javamodule/src/main/java/xyz/block/ftl/test/TestInvokeGoFromJava.java b/jvm-runtime/testdata/java/javamodule/src/main/java/xyz/block/ftl/test/TestInvokeGoFromJava.java index 294b37880d..4c785994e3 100644 --- a/jvm-runtime/testdata/java/javamodule/src/main/java/xyz/block/ftl/test/TestInvokeGoFromJava.java +++ b/jvm-runtime/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 optionalStringArrayVerb(List val, OptionalStringArrayVerbClient client) { + public @Nullable List optionalStringArrayVerb(@Nullable List val, OptionalStringArrayVerbClient client) { return client.call(val); } @Export @Verb - public Map optionalStringMapVerb(Map val, OptionalStringMapVerbClient client) { + public @Nullable Map optionalStringMapVerb(@Nullable Map 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); } diff --git a/jvm-runtime/testdata/kotlin/kotlinmodule/src/main/kotlin/xyz/block/ftl/test/TestInvokeGoFromKotlin.kt b/jvm-runtime/testdata/kotlin/kotlinmodule/src/main/kotlin/xyz/block/ftl/test/TestInvokeGoFromKotlin.kt index b54d638831..909d8aac6b 100644 --- a/jvm-runtime/testdata/kotlin/kotlinmodule/src/main/kotlin/xyz/block/ftl/test/TestInvokeGoFromKotlin.kt +++ b/jvm-runtime/testdata/kotlin/kotlinmodule/src/main/kotlin/xyz/block/ftl/test/TestInvokeGoFromKotlin.kt @@ -120,55 +120,55 @@ fun testObjectOptionalFieldsVerb( // now the same again but with option return / input types @Export @Verb -fun optionalIntVerb(payload: Long, client: OptionalIntVerbClient): Long { +fun optionalIntVerb(payload: Long?, client: OptionalIntVerbClient): Long? { return client.call(payload) } @Export @Verb -fun optionalFloatVerb(payload: Double, client: OptionalFloatVerbClient): Double { +fun optionalFloatVerb(payload: Double?, client: OptionalFloatVerbClient): Double? { return client.call(payload) } @Export @Verb -fun optionalStringVerb(payload: String, client: OptionalStringVerbClient): String { +fun optionalStringVerb(payload: String?, client: OptionalStringVerbClient): String? { return client.call(payload) } @Export @Verb -fun optionalBytesVerb(payload: ByteArray?, client: OptionalBytesVerbClient): ByteArray { +fun optionalBytesVerb(payload: ByteArray?, client: OptionalBytesVerbClient): ByteArray? { return client.call(payload!!) } @Export @Verb -fun optionalBoolVerb(payload: Boolean, client: OptionalBoolVerbClient): Boolean { +fun optionalBoolVerb(payload: Boolean?, client: OptionalBoolVerbClient): Boolean? { return client.call(payload) } @Export @Verb -fun optionalStringArrayVerb(payload: List, client: OptionalStringArrayVerbClient): List { +fun optionalStringArrayVerb(payload: List?, client: OptionalStringArrayVerbClient): List? { return client.call(payload) } @Export @Verb -fun optionalStringMapVerb(payload: Map, client: OptionalStringMapVerbClient): Map { +fun optionalStringMapVerb(payload: Map?, client: OptionalStringMapVerbClient): Map? { return client.call(payload) } @Export @Verb -fun optionalTimeVerb(instant: ZonedDateTime?, client: OptionalTimeVerbClient): ZonedDateTime { +fun optionalTimeVerb(instant: ZonedDateTime?, client: OptionalTimeVerbClient): ZonedDateTime? { return client.call(instant!!) } @Export @Verb -fun optionalTestObjectVerb(payload: TestObject?, client: OptionalTestObjectVerbClient): TestObject { +fun optionalTestObjectVerb(payload: TestObject?, client: OptionalTestObjectVerbClient): TestObject? { return client.call(payload!!) } @@ -177,7 +177,7 @@ fun optionalTestObjectVerb(payload: TestObject?, client: OptionalTestObjectVerbC fun optionalTestObjectOptionalFieldsVerb( payload: TestObjectOptionalFields?, client: OptionalTestObjectOptionalFieldsVerbClient -): TestObjectOptionalFields { +): TestObjectOptionalFields? { return client.call(payload!!) }