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.

Sign up for GitHub

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: Drop verb client interfaces #3110

Merged
merged 5 commits into from
Oct 16, 2024
Merged

fix: Drop verb client interfaces #3110

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1af6356
Box Java verb param and response types
tomdaffurn Oct 14, 2024
6346afc
Don't use super interfaces for generted verb clients
tomdaffurn Oct 15, 2024
b751a5b
Delete the interfaces, rename VerbClientDefinition
tomdaffurn Oct 15, 2024
497e94a
fix comments
tomdaffurn Oct 16, 2024
d69e183
fix comments
tomdaffurn Oct 16, 2024
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
252 changes: 104 additions & 148 deletions ...e/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/VerbProcessor.java
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions ...untime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/VerbType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package xyz.block.ftl.deployment;

public enum VerbType {
VERB,
SINK,
SOURCE,
EMPTY
}
19 changes: 11 additions & 8 deletions jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/VerbClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
package xyz.block.ftl;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

/**
* A client for a specific verb.
*
* The sink source and empty interfaces allow for different call signatures.
*
* @param <P> The verb parameter type
* @param <R> The verb return type
* Annotation that is used to define a verb client
*/
public interface VerbClient<P, R> {
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public @interface VerbClient {

R call(P param);
String module() default "";

String name();
}
18 changes: 0 additions & 18 deletions jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/VerbClientDefinition.java
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/VerbClientEmpty.java
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/VerbClientSink.java
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions jvm-runtime/ftl-runtime/common/runtime/src/main/java/xyz/block/ftl/VerbClientSource.java
View file
Open in desktop

This file was deleted.

27 changes: 7 additions & 20 deletions ...me/java/deployment/src/main/java/xyz/block/ftl/javalang/deployment/JavaCodeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@
import xyz.block.ftl.TypeAlias;
import xyz.block.ftl.TypeAliasMapper;
import xyz.block.ftl.VerbClient;
import xyz.block.ftl.VerbClientDefinition;
import xyz.block.ftl.VerbClientEmpty;
import xyz.block.ftl.VerbClientSink;
import xyz.block.ftl.VerbClientSource;
import xyz.block.ftl.deployment.JVMCodeGenerator;
import xyz.block.ftl.v1.schema.Data;
import xyz.block.ftl.v1.schema.Enum;
Expand Down Expand Up @@ -279,41 +275,32 @@ protected void generateVerb(Module module, Verb verb, String packageName, Map<De
Map<DeclRef, String> nativeTypeAliasMap, Path outputDir)
throws IOException {
TypeSpec.Builder typeBuilder = TypeSpec.interfaceBuilder(className(verb.getName()) + CLIENT)
.addAnnotation(AnnotationSpec.builder(VerbClientDefinition.class)
.addAnnotation(AnnotationSpec.builder(VerbClient.class)
.addMember("name", "\"" + verb.getName() + "\"")
.addMember("module", "\"" + module.getName() + "\"")
.build())
.addModifiers(Modifier.PUBLIC);
.addModifiers(Modifier.PUBLIC)
.addJavadoc("A client for the $L.$L verb", module.getName(), verb.getName());
if (verb.getRequest().hasUnit() && verb.getResponse().hasUnit()) {
typeBuilder.addSuperinterface(ClassName.get(VerbClientEmpty.class));
typeBuilder.addMethod(MethodSpec.methodBuilder("call")
.addModifiers(Modifier.ABSTRACT, Modifier.PUBLIC).build());
} else if (verb.getRequest().hasUnit()) {
typeBuilder.addSuperinterface(ParameterizedTypeName.get(ClassName.get(VerbClientSource.class),
toJavaTypeName(verb.getResponse(), typeAliasMap, nativeTypeAliasMap, true)));
typeBuilder.addMethod(MethodSpec.methodBuilder("call")
.returns(toAnnotatedJavaTypeName(verb.getResponse(), typeAliasMap, nativeTypeAliasMap))
.addModifiers(Modifier.ABSTRACT, Modifier.PUBLIC).build());
} else if (verb.getResponse().hasUnit()) {
typeBuilder.addSuperinterface(ParameterizedTypeName.get(ClassName.get(VerbClientSink.class),
toJavaTypeName(verb.getRequest(), typeAliasMap, nativeTypeAliasMap, true)));
typeBuilder.addMethod(MethodSpec.methodBuilder("call").returns(TypeName.VOID)
.addParameter(toAnnotatedJavaTypeName(verb.getRequest(), typeAliasMap, nativeTypeAliasMap), "value")
.addModifiers(Modifier.ABSTRACT, Modifier.PUBLIC).build());
} else {
typeBuilder.addSuperinterface(ParameterizedTypeName.get(ClassName.get(VerbClient.class),
toJavaTypeName(verb.getRequest(), typeAliasMap, nativeTypeAliasMap, true),
toJavaTypeName(verb.getResponse(), typeAliasMap, nativeTypeAliasMap, true)));
typeBuilder.addMethod(MethodSpec.methodBuilder("call")
.returns(toAnnotatedJavaTypeName(verb.getResponse(), typeAliasMap, nativeTypeAliasMap))
.addParameter(toAnnotatedJavaTypeName(verb.getRequest(), typeAliasMap, nativeTypeAliasMap), "value")
.addModifiers(Modifier.ABSTRACT, Modifier.PUBLIC).build());
}

TypeSpec helloWorld = typeBuilder
.build();

JavaFile javaFile = JavaFile.builder(packageName, helloWorld)
.build();

TypeSpec client = typeBuilder.build();
JavaFile javaFile = JavaFile.builder(packageName, client).build();
javaFile.writeTo(outputDir);
}

Expand Down
27 changes: 8 additions & 19 deletions .../kotlin/deployment/src/main/java/xyz/block/ftl/kotlin/deployment/KotlinCodeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@
import xyz.block.ftl.TypeAlias;
import xyz.block.ftl.TypeAliasMapper;
import xyz.block.ftl.VerbClient;
import xyz.block.ftl.VerbClientDefinition;
import xyz.block.ftl.VerbClientEmpty;
import xyz.block.ftl.VerbClientSink;
import xyz.block.ftl.VerbClientSource;
import xyz.block.ftl.deployment.JVMCodeGenerator;
import xyz.block.ftl.v1.schema.Data;
import xyz.block.ftl.v1.schema.Enum;
Expand Down Expand Up @@ -154,39 +150,32 @@ protected void generateVerb(Module module, Verb verb, String packageName, Map<De
throws IOException {
String thisType = className(verb.getName()) + CLIENT;
TypeSpec.Builder typeBuilder = TypeSpec.interfaceBuilder(thisType)
.addAnnotation(AnnotationSpec.builder(VerbClientDefinition.class)
.addAnnotation(AnnotationSpec.builder(VerbClient.class)
.addMember("name=\"" + verb.getName() + "\"")
.addMember("module=\"" + module.getName() + "\"")
.build())
.addModifiers(KModifier.PUBLIC);
.addModifiers(KModifier.PUBLIC)
.addKdoc("A client for the %L.%L verb", module.getName(), verb.getName());
if (verb.getRequest().hasUnit() && verb.getResponse().hasUnit()) {
typeBuilder.addSuperinterface(className(VerbClientEmpty.class), CodeBlock.of(""));
typeBuilder.addFunction(FunSpec.builder("call")
.addModifiers(KModifier.ABSTRACT, KModifier.PUBLIC).build());
} else if (verb.getRequest().hasUnit()) {
typeBuilder.addSuperinterface(ParameterizedTypeName.get(className(VerbClientSource.class),
toKotlinTypeName(verb.getResponse(), typeAliasMap, nativeTypeAliasMap)), CodeBlock.of(""));
typeBuilder.addFunction(FunSpec.builder("call")
.returns(toKotlinTypeName(verb.getResponse(), typeAliasMap, nativeTypeAliasMap))
.addModifiers(KModifier.PUBLIC, KModifier.OVERRIDE, KModifier.ABSTRACT).build());
.addModifiers(KModifier.PUBLIC, KModifier.ABSTRACT).build());
} else if (verb.getResponse().hasUnit()) {
typeBuilder.addSuperinterface(ParameterizedTypeName.get(className(VerbClientSink.class),
toKotlinTypeName(verb.getRequest(), typeAliasMap, nativeTypeAliasMap)), CodeBlock.of(""));
typeBuilder.addFunction(FunSpec.builder("call")
.addModifiers(KModifier.OVERRIDE, KModifier.ABSTRACT)
.addModifiers(KModifier.PUBLIC, KModifier.ABSTRACT)
.addParameter("value", toKotlinTypeName(verb.getRequest(), typeAliasMap, nativeTypeAliasMap)).build());
} else {
typeBuilder.addSuperinterface(ParameterizedTypeName.get(className(VerbClient.class),
toKotlinTypeName(verb.getRequest(), typeAliasMap, nativeTypeAliasMap),
toKotlinTypeName(verb.getResponse(), typeAliasMap, nativeTypeAliasMap)), CodeBlock.of(""));
typeBuilder.addFunction(FunSpec.builder("call")
.returns(toKotlinTypeName(verb.getResponse(), typeAliasMap, nativeTypeAliasMap))
.addParameter("value", toKotlinTypeName(verb.getRequest(), typeAliasMap, nativeTypeAliasMap))
.addModifiers(KModifier.PUBLIC, KModifier.OVERRIDE, KModifier.ABSTRACT).build());
.addModifiers(KModifier.PUBLIC, KModifier.ABSTRACT).build());
}

FileSpec javaFile = FileSpec.builder(packageName, thisType)
.addType(typeBuilder.build())
.build();

javaFile.writeTo(outputDir);
}

Expand Down
10 changes: 10 additions & 0 deletions jvm-runtime/testdata/go/gomodule/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,3 +280,13 @@ func TypeEnumVerb(ctx context.Context, val AnimalWrapper) (AnimalWrapper, error)
//func MixedEnumVerb(ctx context.Context, val Mixed) (Mixed, error) {
// return val, nil
//}

//ftl:verb export
func PrimitiveResponseVerb(ctx context.Context, val string) (int, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is already IntVerb above that has an int response type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So looking into this I don't know if we need this change:

Currently we generate:

@VerbClientDefinition(
    name = "intVerb",
    module = "gomodule"
)
public interface IntVerbClient extends VerbClient<Long, Long> {
  long call(long value);
}

Which is valid, even though the primitive method does not override the interface type, because we actually generate both methods:

https://github.com/TBD54566975/ftl/blob/b0465e3c2d14cb72400b8724058db1445c663619/jvm-runtime/ftl-runtime/common/deployment/src/main/java/xyz/block/ftl/deployment/VerbProcessor.java#L93

Basically in the int -> int case we generate three different methods:

Object -> Object
Long -> Long
long -> long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compile error doesn't show up for IntVerb because the generated signature of call(long) is different to call(Long) in VerbClient. Need to have an Object param with primitive response value to make the signature the same but different response type.

I guess we can restrict boxing to cases where the param is an Object and the response isn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see what you mean.

I wonder how much implementing VerbClient actually matters. For the generated classes I don't think it actually adds anything, and not being able to use primitives is just kinda annoying. I am not 100% happy with the way verb clients currently work anyway, as invocation within the same module is clunky. Let me think about this a bit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly something like: https://github.com/TBD54566975/ftl/compare/stuartwdouglas/jvm-api-refactor?expand=1

return 1, nil
}

//ftl:verb export
func PrimitiveParamVerb(ctx context.Context, val int) (string, error) {
return "", nil
}