-
Notifications
You must be signed in to change notification settings - Fork 3
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 step state incorrect type
when returning null from steps
#96
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package com.inngest.springbootdemo.testfunctions; | ||
|
||
import com.inngest.FunctionContext; | ||
import com.inngest.InngestFunction; | ||
import com.inngest.InngestFunctionConfigBuilder; | ||
import com.inngest.Step; | ||
import org.jetbrains.annotations.NotNull; | ||
|
||
public class ReturnNullStepFunction extends InngestFunction { | ||
@NotNull | ||
@Override | ||
public InngestFunctionConfigBuilder config(InngestFunctionConfigBuilder builder) { | ||
return builder | ||
.id("null-step-fn") | ||
.name("Function that has a step that returns null") | ||
.triggerEvent("test/return.null.step"); | ||
} | ||
|
||
@Override | ||
public String execute(FunctionContext ctx, Step step) { | ||
return step.run("null", () -> null, String.class); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
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 static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
||
|
||
@IntegrationTest | ||
@Execution(ExecutionMode.CONCURRENT) | ||
class ReturnNullStepIntegrationTest { | ||
@Autowired | ||
private DevServerComponent devServer; | ||
|
||
@Autowired | ||
private Inngest client; | ||
|
||
@Test | ||
void testReturnNullFromStep() throws Exception { | ||
String eventId = InngestFunctionTestHelpers.sendEvent(client, "test/return.null.step").getIds()[0]; | ||
|
||
Thread.sleep(5000); | ||
|
||
RunEntry<Object> run = devServer.runsByEvent(eventId).first(); | ||
|
||
assertEquals(eventId, run.getEvent_id()); | ||
assertEquals("Completed", run.getStatus()); | ||
assertNull(run.getOutput()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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.NullNode | ||
import com.fasterxml.jackson.databind.node.ObjectNode | ||
import java.security.MessageDigest | ||
|
||
|
@@ -60,16 +61,13 @@ class State( | |
val node: JsonNode = mapper.readTree(payloadJson) | ||
val stepResult = node.path("steps").get(hashedId) ?: throw StateNotFound() | ||
|
||
if (stepResult.has(fieldName)) { | ||
return deserializeStepData(stepResult.get(fieldName), type) | ||
} else if (stepResult.has("error")) { | ||
val error = mapper.treeToValue(stepResult.get("error"), StepError::class.java) | ||
throw error | ||
return when { | ||
stepResult.has(fieldName) -> deserializeStepData(stepResult.get(fieldName), type) | ||
stepResult.has("error") -> throw mapper.treeToValue(stepResult.get("error"), StepError::class.java) | ||
// NOTE - Sleep steps will be stored as null | ||
stepResult is NullNode -> null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
else -> throw Exception("Unexpected step data structure") | ||
} | ||
// NOTE - Sleep steps will be stored as null | ||
// TODO - Investigate if sendEvents stores null as well. | ||
// TODO - Check the state is actually null | ||
return null | ||
} | ||
|
||
private fun <T> deserializeStepData( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,10 +102,7 @@ class Step( | |
val hashedId = state.getHashFromId(id) | ||
|
||
try { | ||
val stepResult = state.getState(hashedId, type) | ||
if (stepResult != null) { | ||
return stepResult | ||
} | ||
Comment on lines
-106
to
-108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like we inadvertently introduced this bug with #19 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i believe it was always present because even the older version check returns val stepResult = state.getState<T>(hashedId)
if (stepResult is T) { i made sure it's the case with this generic example: inline fun <reified T> isOfType(value: Any?): Boolean {
return value is T
}
fun main() {
println(isOfType<String>(null))
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. awesome, thanks for verifying |
||
return state.getState(hashedId, type) as T | ||
} catch (e: StateNotFound) { | ||
// If there is no existing result, run the lambda | ||
executeStep(id, hashedId, fn) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just a showcase poc of the bug, I'll be dropping the commit before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it failing before and working after 👍