From 5faeb6445bc388058fdc57bdb0baea5105f2dc58 Mon Sep 17 00:00:00 2001 From: albertchae <217050+albertchae@users.noreply.github.com> Date: Thu, 12 Sep 2024 17:50:48 -0700 Subject: [PATCH] Fix hashing step ids in loops (#72) Per https://github.com/inngest/inngest/blob/main/docs/SDK_SPEC.md#512-ids-and-hashing, add `:n` starting with `:1` for repeated instances of a step id Refactored to be a combination of golang and inngest-js implementation to handle edge case of user defined stepId colliding - https://github.com/inngest/inngestgo/blob/0a00daba0b2db68ff0f080f787cf63f0a63b44d8/internal/sdkrequest/manager.go#L123-L131 - https://github.com/inngest/inngest-js/blob/79069e1a3d700624ce49b323922c113fc952bcc6/packages/inngest/src/components/execution/v1.ts#L819-L831 * Use combination of hash and loop to find next unused stepId The hash means we don't have to loop from 0 every time and for most cases will just correctly return us the next unused stepId, but looping afterwards guarantees we don't collide with a user defined stepId. So this will be O(1) in most cases and potentially O(n) for pathological functions that have many steps manually named with the `:n` suffix * The inngest-js SDK currently optionally warns of parallel indexing, but this isn't in scope for beta so I left it out. --- .../testfunctions/LoopFunction.java | 45 +++++++++++++++++++ .../springbootdemo/DemoTestConfiguration.java | 1 + .../LoopFunctionIntegrationTest.java | 30 +++++++++++++ inngest/src/main/kotlin/com/inngest/State.kt | 27 ++++++++++- 4 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 inngest-spring-boot-demo/src/main/java/com/inngest/springbootdemo/testfunctions/LoopFunction.java create mode 100644 inngest-spring-boot-demo/src/test/java/com/inngest/springbootdemo/LoopFunctionIntegrationTest.java diff --git a/inngest-spring-boot-demo/src/main/java/com/inngest/springbootdemo/testfunctions/LoopFunction.java b/inngest-spring-boot-demo/src/main/java/com/inngest/springbootdemo/testfunctions/LoopFunction.java new file mode 100644 index 00000000..526e067c --- /dev/null +++ b/inngest-spring-boot-demo/src/main/java/com/inngest/springbootdemo/testfunctions/LoopFunction.java @@ -0,0 +1,45 @@ +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 LoopFunction extends InngestFunction { + + @NotNull + @Override + public InngestFunctionConfigBuilder config(InngestFunctionConfigBuilder builder) { + return builder + .id("loop-fn") + .name("Loop Function") + .triggerEvent("test/loop"); + } + + + @Override + public Integer execute(FunctionContext ctx, Step step) { + int runningCount = 10; + + // explicitly naming a step that the SDK will try to use in the loop shouldn't break the loop + int effectivelyFinalVariableForLambda1 = runningCount; + runningCount = step.run("add-num:3", () -> effectivelyFinalVariableForLambda1 + 50, Integer.class); + + for (int i = 0; i < 5; i++) { + int effectivelyFinalVariableForLambda2 = runningCount; + // The actual stepIds used will be add-num, add-num:1, add-num:2, add-num:4, add-num:5 + runningCount = step.run("add-num", () -> effectivelyFinalVariableForLambda2 + 10, Integer.class); + } + + // explicitly reusing step names that the SDK used during the loop should both execute + // These will be modified to add-num:4:1 and add-num:4:2 respectively + int effectivelyFinalVariableForLambda3 = runningCount; + runningCount = step.run("add-num:4", () -> effectivelyFinalVariableForLambda3 + 30, Integer.class); + int effectivelyFinalVariableForLambda4 = runningCount; + runningCount = step.run("add-num:4", () -> effectivelyFinalVariableForLambda4 + 30, Integer.class); + + return runningCount; + } +} + diff --git a/inngest-spring-boot-demo/src/test/java/com/inngest/springbootdemo/DemoTestConfiguration.java b/inngest-spring-boot-demo/src/test/java/com/inngest/springbootdemo/DemoTestConfiguration.java index 82c4dd59..707a05c8 100644 --- a/inngest-spring-boot-demo/src/test/java/com/inngest/springbootdemo/DemoTestConfiguration.java +++ b/inngest-spring-boot-demo/src/test/java/com/inngest/springbootdemo/DemoTestConfiguration.java @@ -31,6 +31,7 @@ protected HashMap functions() { addInngestFunction(functions, new Scale2DObjectFunction()); addInngestFunction(functions, new MultiplyMatrixFunction()); addInngestFunction(functions, new WithOnFailureFunction()); + addInngestFunction(functions, new LoopFunction()); return functions; } diff --git a/inngest-spring-boot-demo/src/test/java/com/inngest/springbootdemo/LoopFunctionIntegrationTest.java b/inngest-spring-boot-demo/src/test/java/com/inngest/springbootdemo/LoopFunctionIntegrationTest.java new file mode 100644 index 00000000..40967527 --- /dev/null +++ b/inngest-spring-boot-demo/src/test/java/com/inngest/springbootdemo/LoopFunctionIntegrationTest.java @@ -0,0 +1,30 @@ +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; + +@IntegrationTest +@Execution(ExecutionMode.CONCURRENT) +class LoopFunctionIntegrationTest { + @Autowired + private DevServerComponent devServer; + + @Autowired + private Inngest client; + + @Test + void testStepsInLoopExecuteCorrectly() throws Exception { + String loopEvent = InngestFunctionTestHelpers.sendEvent(client, "test/loop").getIds()[0]; + Thread.sleep(2000); + + RunEntry loopRun = devServer.runsByEvent(loopEvent).first(); + assertEquals("Completed", loopRun.getStatus()); + + assertEquals(170, loopRun.getOutput()); + } +} diff --git a/inngest/src/main/kotlin/com/inngest/State.kt b/inngest/src/main/kotlin/com/inngest/State.kt index 50e98942..fd1d35b3 100644 --- a/inngest/src/main/kotlin/com/inngest/State.kt +++ b/inngest/src/main/kotlin/com/inngest/State.kt @@ -9,8 +9,14 @@ class StateNotFound : Throwable("State not found for id") class State( private val payloadJson: String, ) { + private val stepIdsToNextStepNumber = mutableMapOf() + private val stepIds = mutableSetOf() + fun getHashFromId(id: String): String { - val bytes = id.toByteArray(Charsets.UTF_8) + val idToHash: String = findNextAvailableStepId(id) + stepIds.add(idToHash) + + val bytes = idToHash.toByteArray(Charsets.UTF_8) val digest = MessageDigest.getInstance("SHA-1") val hashedBytes = digest.digest(bytes) val sb = StringBuilder() @@ -20,6 +26,25 @@ class State( return sb.toString() } + private fun findNextAvailableStepId(id: String): String { + if (id !in stepIds) { + return id + } + + // start with the seen count so far for current stepId + // but loop over all seen stepIds to make sure a user didn't explicitly define + // a step using the same step number + var stepNumber = stepIdsToNextStepNumber.getOrDefault(id, 1) + while ("$id:$stepNumber" in stepIds) { + stepNumber = stepNumber + 1 + } + // now we know stepNumber is unused and can be used for the current stepId + // save stepNumber + 1 to the hash for next time + stepIdsToNextStepNumber[id] = stepNumber + 1 + + return "$id:$stepNumber" + } + inline fun getState( hashedId: String, fieldName: String = "data",