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 serialization of subclasses #92

Merged
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package com.inngest.springbootdemo.testfunctions;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.inngest.FunctionContext;
import com.inngest.InngestFunction;
import com.inngest.InngestFunctionConfigBuilder;
import com.inngest.Step;
import org.jetbrains.annotations.NotNull;

class Dog {
@JsonProperty("legs")
public int legs;

public Dog(@JsonProperty("legs") int legs) {
this.legs = legs;
}
}

class Corgi extends Dog {
@JsonProperty("stumpy")
public boolean stumpy;

public Corgi(@JsonProperty("legs") int legs, @JsonProperty("stumpy") boolean stumpy) {
super(legs);

this.stumpy = stumpy;
}
}

public class DeserializeSubclassFunction extends InngestFunction {
@NotNull
@Override
public InngestFunctionConfigBuilder config(InngestFunctionConfigBuilder builder) {
return builder
.id("DeserializeSubclassFunction")
.name("Deserialize subclass function")
.triggerEvent("test/deserialize.subclass")
.retries(0);
}

@Override
public String execute(FunctionContext ctx, Step step) {
Dog corgi = step.run("get-corgi", () -> new Corgi(4, true), Dog.class);

assert(((Corgi) corgi).stumpy == true);

return "Successfully cast Corgi";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.inngest.springbootdemo.testfunctions;

import com.inngest.*;
import org.jetbrains.annotations.NotNull;

public class TryCatchGenericExceptionFunction extends InngestFunction {
@NotNull
@Override
public InngestFunctionConfigBuilder config(InngestFunctionConfigBuilder builder) {
return builder
.id("try-catch-deserialize-exception-function")
.name("Try Catch Deserialize Exception Function")
.triggerEvent("test/try.catch.deserialize.exception")
.retries(0);
}

@Override
public String execute(FunctionContext ctx, Step step) {
try {
step.run("fail-step", () -> {
throw new CustomException("Something fatally went wrong");
}, String.class);
} catch (Exception originalException) {
Exception e = step.run("handle-error", () -> originalException, Exception.class);
return e.getMessage();
}

return "An error should have been thrown and this message should not be returned";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ protected HashMap<String, InngestFunction> functions() {
addInngestFunction(functions, new RetriableErrorFunction());
addInngestFunction(functions, new ZeroRetriesFunction());
addInngestFunction(functions, new InvokeFailureFunction());
addInngestFunction(functions, new DeserializeSubclassFunction());
addInngestFunction(functions, new TryCatchGenericExceptionFunction());
addInngestFunction(functions, new TryCatchRunFunction());
addInngestFunction(functions, new ThrottledFunction());
addInngestFunction(functions, new RateLimitedFunction());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package com.inngest.springbootdemo;

import com.inngest.Inngest;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.parallel.Execution;
import org.junit.jupiter.api.parallel.ExecutionMode;
import org.springframework.beans.factory.annotation.Autowired;

import java.util.LinkedHashMap;

import static org.junit.jupiter.api.Assertions.*;

@IntegrationTest
@Execution(ExecutionMode.CONCURRENT)
class DeserializationIntegrationTest {
@Autowired
private DevServerComponent devServer;

static int sleepTime = 5000;

@Autowired
private Inngest client;

@Test
void testShouldDeserializeSubclassCorrectly() throws Exception {
String eventId = InngestFunctionTestHelpers.sendEvent(client, "test/deserialize.subclass").getIds()[0];

Thread.sleep(sleepTime);

RunEntry<Object> run = devServer.runsByEvent(eventId).first();
Object output = run.getOutput();
if (output instanceof LinkedHashMap) {
fail("Run threw an exception serialized into a LinkedHashMap:" + output);
}
String outputString = (String) output;

assertEquals("Completed", run.getStatus() );
assertNotNull(run.getEnded_at());

assertEquals("Successfully cast Corgi", outputString);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
import org.junit.jupiter.api.parallel.ExecutionMode;
import org.springframework.beans.factory.annotation.Autowired;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import java.util.LinkedHashMap;

import static org.junit.jupiter.api.Assertions.*;

@IntegrationTest
@Execution(ExecutionMode.CONCURRENT)
Expand Down Expand Up @@ -50,4 +51,22 @@ void testShouldCatchStepErrorWhenRunThrows() throws Exception {
assertEquals("Something fatally went wrong", output);
}

@Test
void testShouldCatchAndDeserializeExceptionWhenRunThrows() throws Exception {
String eventId = InngestFunctionTestHelpers.sendEvent(client, "test/try.catch.deserialize.exception").getIds()[0];

Thread.sleep(sleepTime);

RunEntry<Object> run = devServer.runsByEvent(eventId).first();
Object output = run.getOutput();
if (output instanceof LinkedHashMap) {
fail("Run threw an exception serialized into a LinkedHashMap:" + output);
}
String outputString = (String) output;

assertEquals("Completed", run.getStatus());
assertNotNull(run.getEnded_at());

assertEquals("Something fatally went wrong", outputString);
}
}
25 changes: 24 additions & 1 deletion inngest/src/main/kotlin/com/inngest/Function.kt
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package com.inngest

import com.beust.klaxon.Json
import com.fasterxml.jackson.databind.JsonNode
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.databind.node.ObjectNode
import java.util.function.BiFunction

// TODO - Add an abstraction layer between the Function call response and the comm handler response
Expand Down Expand Up @@ -229,8 +232,9 @@ internal open class InternalInngestFunction(
// NOTE - Currently this error could be caught in the user's own function
// that wraps a
// step.run() - how can we prevent that or warn?

return StepResult(
data = e.data,
data = serializeStepData(e.data),
id = e.hashedId,
name = e.id,
op = OpCode.StepRun,
Expand All @@ -255,4 +259,23 @@ internal open class InternalInngestFunction(
// TODO use URL objects for serveUrl instead of strings so we can fetch things like scheme
return configBuilder.build(client.appId, serveUrl)
}

private fun serializeStepData(stepData: Any?): JsonNode? {
if (stepData == null) {
return stepData
}

val mapper = ObjectMapper()
val jsonString = mapper.writeValueAsString(stepData)
val readOnlyJson = mapper.readTree(jsonString)
Comment on lines +269 to +270
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

converting an object to a JSON string only to read that back in to get a JSON node is silly, but replacing this with

val writeableJson = mapper.convertValue(stepData, ObjectNode::class.java)

resulted in a bunch of test failures, so we decided to punt on figuring this out to a follow up

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good - I think I may have had code like this in an early PoC. There wasn't a great pattern for this in Java and the JSON libraries from what I could tell that didn't result in duplicate parsing/serialization work. I'm find with leaving this for a future improvement.


if (!readOnlyJson.isObject) {
// primitives can be serialized directly
return readOnlyJson
}

val writeableJson = mapper.readTree(jsonString) as ObjectNode
writeableJson.put("class", stepData.javaClass.name)
return writeableJson
}
}
19 changes: 17 additions & 2 deletions inngest/src/main/kotlin/com/inngest/State.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.inngest

import com.fasterxml.jackson.databind.JsonNode
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.databind.node.ObjectNode
import java.security.MessageDigest

class StateNotFound : Throwable("State not found for id")
Expand Down Expand Up @@ -60,8 +61,7 @@ class State(
val stepResult = node.path("steps").get(hashedId) ?: throw StateNotFound()

if (stepResult.has(fieldName)) {
val dataNode = stepResult.get(fieldName)
return mapper.treeToValue(dataNode, type)
return deserializeStepData(stepResult.get(fieldName), type)
} else if (stepResult.has("error")) {
val error = mapper.treeToValue(stepResult.get("error"), StepError::class.java)
throw error
Expand All @@ -71,4 +71,19 @@ class State(
// TODO - Check the state is actually null
return null
}

private fun <T> deserializeStepData(
serializedStepData: JsonNode?,
type: Class<T>,
): T? {
val mapper = ObjectMapper()
if (serializedStepData == null || !serializedStepData.isObject || !serializedStepData.has("class")) {
// null and primitives can be deserialized directly
return mapper.treeToValue(serializedStepData, type)
}

val writeableJson = serializedStepData as ObjectNode
val className = writeableJson.remove("class").asText()
return mapper.treeToValue(writeableJson, Class.forName(className)) as T
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to figure out how to handle the case where this will fail because the class no longer exists (or renamed) or has been changed enough since serialization that this won't deserialize

}
}