From 937f7af8e35b4a5e9f3515a600f99cfbf647c856 Mon Sep 17 00:00:00 2001 From: albertchae <217050+albertchae@users.noreply.github.com> Date: Mon, 9 Sep 2024 19:45:59 -0700 Subject: [PATCH] Validate concurrency key is required when scope is account or env (#81) - If concurrency scope is account or env, validate that key is provided - Also added check that at most 2 concurrency options are provided - Removed exception catch in InngestFunction.kt since it is swallowing other InngestInvalidConfigurationExceptions - Add key for RestoreFromGlacier example --- .../inngest/testserver/RestoreFromGlacier.kt | 2 +- .../kotlin/com/inngest/InngestFunction.kt | 8 +-- .../inngest/InngestFunctionConfigBuilder.kt | 12 +++- .../InngestFunctionConfigBuilderTest.kt | 64 +++++++++++++++++++ 4 files changed, 76 insertions(+), 10 deletions(-) diff --git a/inngest-test-server/src/main/kotlin/com/inngest/testserver/RestoreFromGlacier.kt b/inngest-test-server/src/main/kotlin/com/inngest/testserver/RestoreFromGlacier.kt index 7a6b2e1e..6c49d0fc 100644 --- a/inngest-test-server/src/main/kotlin/com/inngest/testserver/RestoreFromGlacier.kt +++ b/inngest-test-server/src/main/kotlin/com/inngest/testserver/RestoreFromGlacier.kt @@ -9,7 +9,7 @@ class RestoreFromGlacier : InngestFunction() { .id("RestoreFromGlacier") .name("Restore from Glacier") .trigger(InngestFunctionTriggers.Event("delivery/restore.requested")) - .concurrency(10, null, ConcurrencyScope.ENVIRONMENT) + .concurrency(10, "event.data.user_id", ConcurrencyScope.ENVIRONMENT) override fun execute( ctx: FunctionContext, diff --git a/inngest/src/main/kotlin/com/inngest/InngestFunction.kt b/inngest/src/main/kotlin/com/inngest/InngestFunction.kt index bcbc24c6..a64dc6ab 100644 --- a/inngest/src/main/kotlin/com/inngest/InngestFunction.kt +++ b/inngest/src/main/kotlin/com/inngest/InngestFunction.kt @@ -20,13 +20,7 @@ abstract class InngestFunction { } fun id(): String { - try { - return buildConfig().id!! - } catch (e: Exception) { - throw InngestInvalidConfigurationException( - "Function id must be configured via builder: ${this.javaClass.name}", - ) - } + return buildConfig().id!! } internal fun toInngestFunction(): InternalInngestFunction { diff --git a/inngest/src/main/kotlin/com/inngest/InngestFunctionConfigBuilder.kt b/inngest/src/main/kotlin/com/inngest/InngestFunctionConfigBuilder.kt index 633e0815..30e5bf3b 100644 --- a/inngest/src/main/kotlin/com/inngest/InngestFunctionConfigBuilder.kt +++ b/inngest/src/main/kotlin/com/inngest/InngestFunctionConfigBuilder.kt @@ -113,12 +113,20 @@ class InngestFunctionConfigBuilder { key: String? = null, scope: ConcurrencyScope? = null, ): InngestFunctionConfigBuilder { + when (scope) { + ConcurrencyScope.ENVIRONMENT -> if (key == null) throw InngestInvalidConfigurationException("Concurrency key required with environment scope") + ConcurrencyScope.ACCOUNT -> if (key == null) throw InngestInvalidConfigurationException("Concurrency key required with account scope") + ConcurrencyScope.FUNCTION -> {} + null -> {} + } + val c = Concurrency(limit, key, scope) - // TODO - Limit concurrency length to 2 if (this.concurrency == null) { this.concurrency = mutableListOf(c) + } else if (this.concurrency!!.size == 2) { + throw InngestInvalidConfigurationException("Maximum of 2 concurrency options allowed") } else { - this.concurrency?.add(c) + this.concurrency!!.add(c) } return this } diff --git a/inngest/src/test/kotlin/com/inngest/InngestFunctionConfigBuilderTest.kt b/inngest/src/test/kotlin/com/inngest/InngestFunctionConfigBuilderTest.kt index 229734e3..05b4f37d 100644 --- a/inngest/src/test/kotlin/com/inngest/InngestFunctionConfigBuilderTest.kt +++ b/inngest/src/test/kotlin/com/inngest/InngestFunctionConfigBuilderTest.kt @@ -21,4 +21,68 @@ class InngestFunctionConfigBuilderTest { .build("app-id", "https://mysite.com/api/inngest") } } + + @Test + fun testConcurrencyLimitOnly() { + val config = + InngestFunctionConfigBuilder() + .id("test-id") + .concurrency(5) + .build("app-id", "https://mysite.com/api/inngest") + assertEquals?>(listOf(Concurrency(5)), config.concurrency) + } + + @Test + fun testConcurrencyLimitAndKeyWithoutScope() { + val config = + InngestFunctionConfigBuilder() + .id("test-id") + .concurrency(5, "event.data.user_id") + .build("app-id", "https://mysite.com/api/inngest") + assertEquals?>(listOf(Concurrency(5, "event.data.user_id")), config.concurrency) + } + + @Test + fun testConcurrencyScope() { + val config = + InngestFunctionConfigBuilder() + .id("test-id") + .concurrency(5, null, ConcurrencyScope.FUNCTION) + .build("app-id", "https://mysite.com/api/inngest") + assertEquals?>(listOf(Concurrency(5, null, ConcurrencyScope.FUNCTION)), config.concurrency) + + assertFailsWith { + InngestFunctionConfigBuilder() + .id("test-id") + .concurrency(5, null, ConcurrencyScope.ENVIRONMENT) + .build("app-id", "https://mysite.com/api/inngest") + } + + assertFailsWith { + InngestFunctionConfigBuilder() + .id("test-id") + .concurrency(5, null, ConcurrencyScope.ACCOUNT) + .build("app-id", "https://mysite.com/api/inngest") + } + } + + @Test + fun testConcurrencyMaxOptionsLength() { + val config = + InngestFunctionConfigBuilder() + .id("test-id") + .concurrency(5, null, ConcurrencyScope.FUNCTION) + .concurrency(7, "event.data.user_id", ConcurrencyScope.ENVIRONMENT) + .build("app-id", "https://mysite.com/api/inngest") + assertEquals?>(listOf(Concurrency(5, null, ConcurrencyScope.FUNCTION), Concurrency(7, "event.data.user_id", ConcurrencyScope.ENVIRONMENT)), config.concurrency) + + assertFailsWith { + InngestFunctionConfigBuilder() + .id("test-id") + .concurrency(5, null, ConcurrencyScope.FUNCTION) + .concurrency(7, "event.data.user_id", ConcurrencyScope.ENVIRONMENT) + .concurrency(9, "event.data.account_id", ConcurrencyScope.ACCOUNT) + .build("app-id", "https://mysite.com/api/inngest") + } + } }