Skip to content

Commit

Permalink
Fix corner cases around code generation and "reserved" keywords (#280)
Browse files Browse the repository at this point in the history 
* Validate component nor handlers start with `restate` or `openapi`
* Allow to use send as handler name, by adding a prefix in front of it when generating the client.
  • Loading branch information
@slinkydeveloper
slinkydeveloper authored Apr 11, 2024
1 parent b4e737e commit 0c5917b
Show file tree
Hide file tree
Showing 11 changed files with 150 additions and 77 deletions.
17 changes: 16 additions & 1 deletion sdk-api-gen-common/src/main/java/dev/restate/sdk/gen/model/Component.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,22 @@ public List<Handler> getHandlers() {
return handlers;
}

public Component build() {
public Component validateAndBuild() {
String componentNameLowercase = componentName.toLowerCase();
if (componentNameLowercase.startsWith("restate")
|| componentNameLowercase.startsWith("openapi")) {
throw new IllegalArgumentException(
"A component name cannot start with `restate` or `openapi`");
}

if (componentType.equals(ComponentType.WORKFLOW)) {
if (handlers.stream().filter(m -> m.getHandlerType().equals(HandlerType.WORKFLOW)).count()
!= 1) {
throw new IllegalArgumentException(
"Workflow services must have exactly one method annotated as @Workflow");
}
}

return new Component(
Objects.requireNonNull(targetPkg),
Objects.requireNonNull(targetFqcn),
Expand Down
9 changes: 8 additions & 1 deletion sdk-api-gen-common/src/main/java/dev/restate/sdk/gen/model/Handler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,14 @@ public PayloadType getOutputType() {
return outputType;
}

public Handler build() {
public Handler validateAndBuild() {
String handlerNameLowercase = name.toString().toLowerCase();
if (handlerNameLowercase.startsWith("restate")
|| handlerNameLowercase.startsWith("openapi")) {
throw new IllegalArgumentException(
"A component name cannot start with `restate` or `openapi`");
}

return new Handler(
Objects.requireNonNull(name), Objects.requireNonNull(handlerType), inputType, outputType);
}
Expand Down
21 changes: 16 additions & 5 deletions sdk-api-gen-common/src/main/java/dev/restate/sdk/gen/template/HandlebarsTemplateEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,22 @@
import java.io.Writer;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

public class HandlebarsTemplateEngine {

private final String baseTemplateName;
private final Map<ComponentType, Template> templates;
private final Set<String> handlerNamesToPrefix;

public HandlebarsTemplateEngine(
String baseTemplateName,
TemplateLoader templateLoader,
Map<ComponentType, String> templates) {
Map<ComponentType, String> templates,
Set<String> handlerNamesToPrefix) {
this.baseTemplateName = baseTemplateName;
this.handlerNamesToPrefix = handlerNamesToPrefix;

Handlebars handlebars = new Handlebars(templateLoader);
handlebars.registerHelpers(StringHelpers.class);
Expand Down Expand Up @@ -65,7 +69,9 @@ public void generate(ThrowingFunction<String, Writer> createFile, Component comp
this.templates
.get(component.getComponentType())
.apply(
Context.newBuilder(new ComponentTemplateModel(component, this.baseTemplateName))
Context.newBuilder(
new ComponentTemplateModel(
component, this.baseTemplateName, this.handlerNamesToPrefix))
.resolver(FieldValueResolver.INSTANCE)
.build(),
out);
Expand All @@ -86,7 +92,8 @@ static class ComponentTemplateModel {
public final boolean isService;
public final List<HandlerTemplateModel> handlers;

private ComponentTemplateModel(Component inner, String baseTemplateName) {
private ComponentTemplateModel(
Component inner, String baseTemplateName, Set<String> handlerNamesToPrefix) {
this.originalClassPkg = inner.getTargetPkg().toString();
this.originalClassFqcn = inner.getTargetFqcn().toString();
this.generatedClassSimpleNamePrefix = inner.getSimpleComponentName();
Expand All @@ -99,12 +106,15 @@ private ComponentTemplateModel(Component inner, String baseTemplateName) {
this.isService = inner.getComponentType() == ComponentType.SERVICE;

this.handlers =
inner.getMethods().stream().map(HandlerTemplateModel::new).collect(Collectors.toList());
inner.getMethods().stream()
.map(h -> new HandlerTemplateModel(h, handlerNamesToPrefix))
.collect(Collectors.toList());
}
}

static class HandlerTemplateModel {
public final String name;
public final String methodName;
public final String handlerType;
public final boolean isWorkflow;
public final boolean isShared;
Expand All @@ -123,8 +133,9 @@ static class HandlerTemplateModel {
public final String boxedOutputFqcn;
public final String outputSerdeFieldName;

private HandlerTemplateModel(Handler inner) {
private HandlerTemplateModel(Handler inner, Set<String> handlerNamesToPrefix) {
this.name = inner.getName().toString();
this.methodName = (handlerNamesToPrefix.contains(this.name) ? "_" : "") + this.name;
this.handlerType = inner.getHandlerType().toString();
this.isWorkflow = inner.getHandlerType() == HandlerType.WORKFLOW;
this.isShared = inner.getHandlerType() == HandlerType.SHARED;
Expand Down
11 changes: 8 additions & 3 deletions sdk-api-gen/src/main/java/dev/restate/sdk/gen/ComponentProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public class ComponentProcessor extends AbstractProcessor {
private HandlebarsTemplateEngine bindableComponentCodegen;
private HandlebarsTemplateEngine clientCodegen;

private static final Set<String> RESERVED_METHOD_NAMES = Set.of("send");

@Override
public synchronized void init(ProcessingEnvironment processingEnv) {
super.init(processingEnv);
Expand All @@ -55,7 +57,8 @@ public synchronized void init(ProcessingEnvironment processingEnv) {
ComponentType.SERVICE,
"templates/BindableComponentFactory.hbs",
ComponentType.VIRTUAL_OBJECT,
"templates/BindableComponentFactory.hbs"));
"templates/BindableComponentFactory.hbs"),
RESERVED_METHOD_NAMES);
this.bindableComponentCodegen =
new HandlebarsTemplateEngine(
"BindableComponent",
Expand All @@ -66,7 +69,8 @@ public synchronized void init(ProcessingEnvironment processingEnv) {
ComponentType.SERVICE,
"templates/BindableComponent.hbs",
ComponentType.VIRTUAL_OBJECT,
"templates/BindableComponent.hbs"));
"templates/BindableComponent.hbs"),
RESERVED_METHOD_NAMES);
this.clientCodegen =
new HandlebarsTemplateEngine(
"Client",
Expand All @@ -77,7 +81,8 @@ public synchronized void init(ProcessingEnvironment processingEnv) {
ComponentType.SERVICE,
"templates/Client.hbs",
ComponentType.VIRTUAL_OBJECT,
"templates/Client.hbs"));
"templates/Client.hbs"),
RESERVED_METHOD_NAMES);
}

@Override
Expand Down
67 changes: 33 additions & 34 deletions sdk-api-gen/src/main/java/dev/restate/sdk/gen/ElementConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,20 +107,27 @@ public Component fromTypeElement(TypeElement element) {
|| e.getAnnotation(Shared.class) != null)
.map(e -> fromExecutableElement(type, ((ExecutableElement) e)))
.collect(Collectors.toList());
validateHandlers(type, handlers, element);

if (handlers.isEmpty()) {
messager.printMessage(
Diagnostic.Kind.WARNING, "The component " + componentName + " has no handlers", element);
}

return new Component.Builder()
.withTargetPkg(targetPkg)
.withTargetFqcn(targetFqcn)
.withComponentName(componentName)
.withComponentType(type)
.withHandlers(handlers)
.build();
try {
return new Component.Builder()
.withTargetPkg(targetPkg)
.withTargetFqcn(targetFqcn)
.withComponentName(componentName)
.withComponentType(type)
.withHandlers(handlers)
.validateAndBuild();
} catch (Exception e) {
messager.printMessage(
Diagnostic.Kind.ERROR,
"Can't build the component " + componentName + ": " + e.getMessage(),
element);
return null;
}
}

private void validateType(TypeElement element) {
Expand All @@ -140,20 +147,6 @@ private void validateType(TypeElement element) {
}
}

private void validateHandlers(
ComponentType componentType, List<Handler> handlers, TypeElement element) {
// Additional validation for Workflow types
if (componentType.equals(ComponentType.WORKFLOW)) {
if (handlers.stream().filter(m -> m.getHandlerType().equals(HandlerType.WORKFLOW)).count()
!= 1) {
messager.printMessage(
Diagnostic.Kind.ERROR,
"Workflow services must have exactly one method annotated as @Workflow",
element);
}
}
}

private Handler fromExecutableElement(ComponentType componentType, ExecutableElement element) {
if (!element.getTypeParameters().isEmpty()) {
messager.printMessage(
Expand Down Expand Up @@ -199,18 +192,24 @@ private Handler fromExecutableElement(ComponentType componentType, ExecutableEle

validateMethodSignature(componentType, handlerType, element);

return new Handler.Builder()
.withName(element.getSimpleName())
.withHandlerType(handlerType)
.withInputType(
element.getParameters().size() > 1
? payloadFromType(element.getParameters().get(1).asType())
: EMPTY_PAYLOAD)
.withOutputType(
!element.getReturnType().getKind().equals(TypeKind.VOID)
? payloadFromType(element.getReturnType())
: EMPTY_PAYLOAD)
.build();
try {
return new Handler.Builder()
.withName(element.getSimpleName())
.withHandlerType(handlerType)
.withInputType(
element.getParameters().size() > 1
? payloadFromType(element.getParameters().get(1).asType())
: EMPTY_PAYLOAD)
.withOutputType(
!element.getReturnType().getKind().equals(TypeKind.VOID)
? payloadFromType(element.getReturnType())
: EMPTY_PAYLOAD)
.validateAndBuild();
} catch (Exception e) {
messager.printMessage(
Diagnostic.Kind.ERROR, "Error when building handler: " + e.getMessage(), element);
return null;
}
}

private HandlerType defaultHandlerType(ComponentType componentType, ExecutableElement element) {
Expand Down
28 changes: 14 additions & 14 deletions sdk-api-gen/src/main/resources/templates/Client.hbs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class {{generatedClassSimpleName}} {
}

{{#handlers}}
public Awaitable<{{{boxedOutputFqcn}}}> {{name}}({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
public Awaitable<{{{boxedOutputFqcn}}}> {{methodName}}({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
return this.ctx.call(
{{#if isObject}}Target.virtualObject(COMPONENT_NAME, this.key, "{{name}}"){{else}}Target.service(COMPONENT_NAME, "{{name}}"){{/if}},
{{inputSerdeFieldName}},
Expand All @@ -65,7 +65,7 @@ public class {{generatedClassSimpleName}} {
}

{{#handlers}}
public void {{name}}({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
public void {{methodName}}({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
ContextClient.this.ctx.send(
{{#if isObject}}Target.virtualObject(COMPONENT_NAME, ContextClient.this.key, "{{name}}"){{else}}Target.service(COMPONENT_NAME, "{{name}}"){{/if}},
{{inputSerdeFieldName}},
Expand All @@ -86,13 +86,13 @@ public class {{generatedClassSimpleName}} {
}

{{#handlers}}
public {{#if outputEmpty}}void{{else}}{{{outputFqcn}}}{{/if}} {{name}}({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
{{^outputEmpty}}return {{/outputEmpty}}this.{{name}}(
public {{#if outputEmpty}}void{{else}}{{{outputFqcn}}}{{/if}} {{methodName}}({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
{{^outputEmpty}}return {{/outputEmpty}}this.{{methodName}}(
{{^inputEmpty}}req, {{/inputEmpty}}
dev.restate.sdk.client.RequestOptions.DEFAULT);
}

public {{#if outputEmpty}}void{{else}}{{{outputFqcn}}}{{/if}} {{name}}({{^inputEmpty}}{{{inputFqcn}}} req, {{/inputEmpty}}dev.restate.sdk.client.RequestOptions requestOptions) {
public {{#if outputEmpty}}void{{else}}{{{outputFqcn}}}{{/if}} {{methodName}}({{^inputEmpty}}{{{inputFqcn}}} req, {{/inputEmpty}}dev.restate.sdk.client.RequestOptions requestOptions) {
{{^outputEmpty}}return {{/outputEmpty}}this.ingressClient.call(
{{#if isObject}}Target.virtualObject(COMPONENT_NAME, this.key, "{{name}}"){{else}}Target.service(COMPONENT_NAME, "{{name}}"){{/if}},
{{inputSerdeFieldName}},
Expand All @@ -101,13 +101,13 @@ public class {{generatedClassSimpleName}} {
requestOptions);
}

public {{#if outputEmpty}}java.util.concurrent.CompletableFuture<Void>{{else}}java.util.concurrent.CompletableFuture<{{{boxedOutputFqcn}}}>{{/if}} {{name}}Async({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
return this.{{name}}Async(
public {{#if outputEmpty}}java.util.concurrent.CompletableFuture<Void>{{else}}java.util.concurrent.CompletableFuture<{{{boxedOutputFqcn}}}>{{/if}} {{methodName}}Async({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
return this.{{methodName}}Async(
{{^inputEmpty}}req, {{/inputEmpty}}
dev.restate.sdk.client.RequestOptions.DEFAULT);
}

public {{#if outputEmpty}}java.util.concurrent.CompletableFuture<Void>{{else}}java.util.concurrent.CompletableFuture<{{{boxedOutputFqcn}}}>{{/if}} {{name}}Async({{^inputEmpty}}{{{inputFqcn}}} req, {{/inputEmpty}}dev.restate.sdk.client.RequestOptions requestOptions) {
public {{#if outputEmpty}}java.util.concurrent.CompletableFuture<Void>{{else}}java.util.concurrent.CompletableFuture<{{{boxedOutputFqcn}}}>{{/if}} {{methodName}}Async({{^inputEmpty}}{{{inputFqcn}}} req, {{/inputEmpty}}dev.restate.sdk.client.RequestOptions requestOptions) {
return this.ingressClient.callAsync(
{{#if isObject}}Target.virtualObject(COMPONENT_NAME, this.key, "{{name}}"){{else}}Target.service(COMPONENT_NAME, "{{name}}"){{/if}},
{{inputSerdeFieldName}},
Expand All @@ -133,13 +133,13 @@ public class {{generatedClassSimpleName}} {
}

{{#handlers}}
public String {{name}}({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
return this.{{name}}(
public String {{methodName}}({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
return this.{{methodName}}(
{{^inputEmpty}}req, {{/inputEmpty}}
dev.restate.sdk.client.RequestOptions.DEFAULT);
}

public String {{name}}({{^inputEmpty}}{{{inputFqcn}}} req, {{/inputEmpty}}dev.restate.sdk.client.RequestOptions requestOptions) {
public String {{methodName}}({{^inputEmpty}}{{{inputFqcn}}} req, {{/inputEmpty}}dev.restate.sdk.client.RequestOptions requestOptions) {
return IngressClient.this.ingressClient.send(
{{#if isObject}}Target.virtualObject(COMPONENT_NAME, IngressClient.this.key, "{{name}}"){{else}}Target.service(COMPONENT_NAME, "{{name}}"){{/if}},
{{inputSerdeFieldName}},
Expand All @@ -148,13 +148,13 @@ public class {{generatedClassSimpleName}} {
requestOptions);
}

public java.util.concurrent.CompletableFuture<String> {{name}}Async({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
return this.{{name}}Async(
public java.util.concurrent.CompletableFuture<String> {{methodName}}Async({{^inputEmpty}}{{{inputFqcn}}} req{{/inputEmpty}}) {
return this.{{methodName}}Async(
{{^inputEmpty}}req, {{/inputEmpty}}
dev.restate.sdk.client.RequestOptions.DEFAULT);
}

public java.util.concurrent.CompletableFuture<String> {{name}}Async({{^inputEmpty}}{{{inputFqcn}}} req, {{/inputEmpty}}dev.restate.sdk.client.RequestOptions requestOptions) {
public java.util.concurrent.CompletableFuture<String> {{methodName}}Async({{^inputEmpty}}{{{inputFqcn}}} req, {{/inputEmpty}}dev.restate.sdk.client.RequestOptions requestOptions) {
return IngressClient.this.ingressClient.sendAsync(
{{#if isObject}}Target.virtualObject(COMPONENT_NAME, IngressClient.this.key, "{{name}}"){{else}}Target.service(COMPONENT_NAME, "{{name}}"){{/if}},
{{inputSerdeFieldName}},
Expand Down
9 changes: 9 additions & 0 deletions sdk-api-gen/src/test/java/dev/restate/sdk/CodegenTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,15 @@ public void primitiveInput(Context context, int input) {
}
}

@VirtualObject
static class CornerCases {
@Exclusive
public String send(ObjectContext context, String request) {
// Just needs to compile
return CodegenTestCornerCasesClient.fromContext(context, request)._send("my_send").await();
}
}

@Override
public Stream<TestDefinitions.TestDefinition> definitions() {
return Stream.of(
Expand Down
Loading

0 comments on commit 0c5917b

Please sign in to comment.