Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: optional handling in the JVM #2951

Merged
merged 1 commit into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions internal/integration/actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package xyz.block.ftl.deployment;

public enum Nullability {
MISSING,
NULLABLE,
NOT_NULL
}
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
80 changes: 59 additions & 21 deletions jvm-runtime/jvm_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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")
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -43,6 +44,9 @@

public class TestInvokeGoFromJava {

/**
* JAVA COMMENT
*/
@Export
@Verb
public void emptyVerb(EmptyVerbClient emptyVerbClient) {
Expand All @@ -61,9 +65,6 @@ public String sourceVerb(SourceVerbClient sourceVerbClient) {
return sourceVerbClient.call();
}

/**
* JAVA COMMENT
*/
@Export
@Verb
public void errorEmptyVerb(ErrorEmptyVerbClient client) {
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the typical place for an annotation on an array?!

Copy link
Collaborator Author

@stuartwdouglas stuartwdouglas Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nullable Object[] is an array with nullable elements, Object @Nullable [] is an array that can be null.

It's very weird syntax and not very common, you generally don't want nullable arrays anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suuuuper weird!

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);
}

Expand Down
Loading
Loading