fix: java type handling #2354

stuartwdouglas · 2024-08-14T04:45:31Z

Improves the tests and fixes problems with the handling of some types, there is still work to go (as seen by the commented out tests), but this significantly improves the situation.

jonathanj-square · 2024-08-14T05:16:50Z

...-runtime/ftl-runtime/deployment/src/main/java/xyz/block/ftl/deployment/FTLCodeGenerator.java

        } else if (type.hasString()) {
            return ClassName.get(String.class);
        } else if (type.hasOptional()) {
-            return toJavaTypeName(type.getOptional().getType(), typeAliasMap);
+            // Always box for optional, as normal primities can't be null
+            return toJavaTypeName(type.getOptional().getType(), typeAliasMap, true);


if I'm not mistaken it appears that we are unwrapping the optional type and boxing it. What do you think about making use of java.util.Optional[T] instead in this instance (in the interest of idiomatic optionality semantic preservation)?

A boxed type maps exactly to the underlying FTL type (true, false, undefined). Optional adds an extra state (true, false, empty, the Optional field is null).

Optional has never really hugely caught on, and it has a lot of drawbacks (Intellij actually warns against using it for parameter or field types). It was mostly used to indicate that a method may return a null value. In practice it solves very little, and you end up with an extra possible state that the Optional itself is null. It also does not play particularly well with both Kotlin and JSON mapping libraries, both of which are very relevant here.

To make optional types work the way you would expect in Kotlin you need to use the @NotNull annotation, rather than Optional, which is why I am going for this approach.

Will this get represented in Kotlin as a native optional?

It should be yes, although medium term I might want to do an actual native kotlin code generator.

There is still more work around optional, I have filed #2356, but in general kotlin uses the @NotNull annotation to tell the difference between Foo and Foo?.

jonathanj-square · 2024-08-14T05:28:04Z

java-runtime/ftl-runtime/deployment/src/main/java/xyz/block/ftl/deployment/FtlProcessor.java

@@ -536,9 +543,33 @@ private static Class<?> loadClass(org.jboss.jandex.Type param) throws ClassNotFo
                default:
                    throw new RuntimeException("Unknown primitive type " + param.asPrimitiveType().primitive());
            }
-        } else {
-            throw new RuntimeException("Unknown type " + param.kind());
+        } else if (param.kind() == org.jboss.jandex.Type.Kind.ARRAY) {


nit: FTLCodeGenerator.java and this file both feature type projection logic. It might be useful to consolidate this.

One maps from a Jandex bytecode representation of a type to a FTL schema, the other maps the FTL schema representation of a type to a Java language string representing the type. They are not dealing with the same types of objects, and also have subtle differences in the type that is supposed to be generated. e.g. a FTL boolean type should generally just be mapped to a boolean, except if you are producing a type parameter because you can't use primitives in type params.

At some point soon I will also add Kotlin to that integration test, so you can see how the calls / schemas map in all 3 languages.

The motivation for that comment has to do with the similarity of the type projection task being accomplished. There is probably some desire for this to work consistently across the these type systems and it will be easier to spot unintended drift if they are co-located. I marked this as a nit though because I suspect there are other implementation concerns that might be negatively impacted by decoupling this type projection logic

jonathanj-square · 2024-08-14T05:45:47Z

java-runtime/ftl-runtime/runtime/src/main/java/xyz/block/ftl/runtime/FTLController.java

        }

        @Override
        public void onCompleted() {
-            verbService.getModuleContext(ModuleContextRequest.newBuilder().setModule(moduleName).build(), moduleObserver);
+            onError(new RuntimeException("connection closed"));


nit: an explanation for why connection closure is the only reason for completion might be comment worthy

I guess it is really stream closed rather than connection closed.

stuartwdouglas · 2024-08-14T06:24:31Z

integration/actions.go

@@ -90,6 +90,17 @@ func Chain(actions ...Action) Action {
 	}
 }

+// SubTests runs a list of individual actions as separate tests
+func SubTests(tests ...SubTest) Action {
+	return func(t testing.TB, ic TestContext) {


@alecthomas are you ok with this approach for running multiple assertions against a single instance?

Chain() already does functionally the same thing, though this uses named subtests so is slightly different. Seems reasonable.

alecthomas

Overall LGTM but let's use the established approach for multi-language tests that we already have. Will send a PR to support it.

alecthomas · 2024-08-14T07:47:53Z

integration/actions.go

@@ -90,6 +90,17 @@ func Chain(actions ...Action) Action {
 	}
 }

+// SubTests runs a list of individual actions as separate tests
+func SubTests(tests ...SubTest) Action {
+	return func(t testing.TB, ic TestContext) {


Chain() already does functionally the same thing, though this uses named subtests so is slightly different. Seems reasonable.

alecthomas · 2024-08-14T07:56:23Z

java-runtime/java_integration_test.go

+	return PairedTest(prefex+"-"+verb, VerbTest[T](verb, value))
+}
+
+type TestObject struct {


We already have an approach in place for doing multi-language tests, I'd really prefer we don't recreate that in a bespoke manner for the Java runtime.

To make that work with the existing system we'll need some mechanism to tell the integration testing system which languages to run the same test against. I'll send a PR to support this.

Won't chain fail at the first failure? As I had a large number of tests it was really useful to just see the overall state in the IDE.

I didn't realize there was already this sort of multi language testing, I should have checked first.

Also TestObject is just an arbitrary object with lots of different field types to test how they serialize, it is not really part of the testing system.

Yeah I understand that TestObject is common to both languages, this kind of "conformance testing" to ensure that the behaviour across multiple languages is identical is exactly what the integration framework was designed for. We actually used to have 1:1 integration test coverage across kotlin and go, but sadly we had to delete them when we paused support :(

alecthomas · 2024-08-14T08:59:12Z

I've extended the integration harness to support multiple languages here. Something like the following filesystem layout:

testdata/go/hello
testdata/java/hello

Then run the single test across both languages with:

in.Run(t,
  in.WithLanguages("go", "java"),
  in.CopyModule("hello"),
 ...
)

Improves the tests and fixes problems with the handling of some types, there is still a significant amount of work to go.

worstell · 2024-08-14T21:29:50Z

awesome, lgtm! should we add a ticket for handling enums/designing how sum types will be represented in Java as well?

alecthomas · 2024-08-14T22:59:05Z

integration/harness.go

-func WithJava() Option {
+// BuildJava is a Run* option that ensures the Java runtime is built.
+// If the test languages contain java this is not necessary, as it is implied
+func BuildJava() Option {


Let's keep all options prefixed with "With" so they're easy to discover.

Ok, I just found WithJava kinda confusing as it did not actually add the java language. I will change it to WithJavaBuild

stuartwdouglas requested a review from alecthomas as a code owner August 14, 2024 04:45

stuartwdouglas requested review from a team and worstell and removed request for a team August 14, 2024 04:45

ftl-robot mentioned this pull request Aug 14, 2024

Dashboard #728

Open

jonathanj-square reviewed Aug 14, 2024

View reviewed changes

stuartwdouglas force-pushed the stuartwdouglas/java-tests branch from 90004dc to b947adb Compare August 14, 2024 06:00

stuartwdouglas commented Aug 14, 2024

View reviewed changes

stuartwdouglas force-pushed the stuartwdouglas/java-tests branch 2 times, most recently from e4ee054 to 43bf219 Compare August 14, 2024 07:15

alecthomas reviewed Aug 14, 2024

View reviewed changes

stuartwdouglas force-pushed the stuartwdouglas/java-tests branch from 43bf219 to 9fc0b37 Compare August 14, 2024 08:43

stuartwdouglas added the run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue label Aug 14, 2024

fix: java type handling

adfa922

Improves the tests and fixes problems with the handling of some types, there is still a significant amount of work to go.

stuartwdouglas force-pushed the stuartwdouglas/java-tests branch from 9fc0b37 to adfa922 Compare August 14, 2024 19:53

stuartwdouglas added 2 commits August 15, 2024 06:48

fix: fix Java leases and add test

2ce1deb

chore: clean up the WithJava API

cd1ee3f

stuartwdouglas force-pushed the stuartwdouglas/java-tests branch from c99f200 to cd1ee3f Compare August 14, 2024 21:23

worstell approved these changes Aug 14, 2024

View reviewed changes

chore: fix gitignore

c4cf07e

stuartwdouglas force-pushed the stuartwdouglas/java-tests branch from 97102ef to c4cf07e Compare August 14, 2024 21:48

alecthomas approved these changes Aug 14, 2024

View reviewed changes

chore: use version 1.0-SNAPSHOT

5221b9c

stuartwdouglas force-pushed the stuartwdouglas/java-tests branch from fcf83c2 to 5221b9c Compare August 14, 2024 23:02

stuartwdouglas enabled auto-merge August 14, 2024 23:52

stuartwdouglas added this pull request to the merge queue Aug 14, 2024

Merged via the queue into main with commit df046cd Aug 15, 2024
69 checks passed

stuartwdouglas deleted the stuartwdouglas/java-tests branch August 15, 2024 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: java type handling #2354

fix: java type handling #2354

stuartwdouglas commented Aug 14, 2024

jonathanj-square Aug 14, 2024 •

edited

Loading

stuartwdouglas Aug 14, 2024

alecthomas Aug 14, 2024

stuartwdouglas Aug 14, 2024

jonathanj-square Aug 14, 2024 •

edited

Loading

stuartwdouglas Aug 14, 2024

stuartwdouglas Aug 14, 2024

jonathanj-square Aug 14, 2024 •

edited

Loading

jonathanj-square Aug 14, 2024 •

edited

Loading

stuartwdouglas Aug 14, 2024

stuartwdouglas Aug 14, 2024

alecthomas Aug 14, 2024

alecthomas left a comment

alecthomas Aug 14, 2024

alecthomas Aug 14, 2024

stuartwdouglas Aug 14, 2024

stuartwdouglas Aug 14, 2024

alecthomas Aug 14, 2024

alecthomas commented Aug 14, 2024 •

edited

Loading

worstell commented Aug 14, 2024

alecthomas Aug 14, 2024

stuartwdouglas Aug 14, 2024

fix: java type handling #2354

fix: java type handling #2354

Conversation

stuartwdouglas commented Aug 14, 2024

jonathanj-square Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanj-square Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanj-square Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

jonathanj-square Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecthomas commented Aug 14, 2024 • edited Loading

worstell commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanj-square Aug 14, 2024 •

edited

Loading

jonathanj-square Aug 14, 2024 •

edited

Loading

jonathanj-square Aug 14, 2024 •

edited

Loading

jonathanj-square Aug 14, 2024 •

edited

Loading

alecthomas commented Aug 14, 2024 •

edited

Loading