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.

Sign up for GitHub

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

Validate concurrency key is required when scope is account or env #81

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Validate concurrency key is required when scope is account or env #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion inngest-test-server/src/main/kotlin/com/inngest/testserver/RestoreFromGlacier.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
8 changes: 1 addition & 7 deletions inngest/src/main/kotlin/com/inngest/InngestFunction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}",
)
}
Comment on lines -25 to -29
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is unnecessary because it's being thrown here

inngest-kt/inngest/src/main/kotlin/com/inngest/InngestFunctionConfigBuilder.kt

Lines 210 to 212 in 70ac4e3

if (id == null) {
throw InngestInvalidConfigurationException("Function id must be configured via builder")
}

and catch (e: Exception) was swallowing other InngestInvalidConfigurationExceptions and rethrowing them for the wrong reason

return buildConfig().id!!
}

internal fun toInngestFunction(): InternalInngestFunction {
Expand Down
12 changes: 10 additions & 2 deletions inngest/src/main/kotlin/com/inngest/InngestFunctionConfigBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Comment on lines +117 to +118
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the ticket originally said function or env scope but dev server shows this

Screenshot 2024-09-09 at 8 22 02 AM
Screenshot 2024-09-09 at 8 20 49 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see yeah that makes sense, the js sdk isn't preventing this either but it could probably prevent it better with a union type instead of exceptions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I was considering a similar idea but it seemed more cumbersome to do in kotlin, so I went with the validation method

ConcurrencyScope.FUNCTION -> {}
null -> {}
Comment on lines +119 to +120
Copy link
Collaborator

@KiKoS0 KiKoS0 Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: can be a single else clause unless you wanted all expected values explicit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I was thinking of making it exhaustive but I could be convinced either way

}

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that i think of it Kotlin not inferring the non-null type is because different threads could mutate it to null between the null check and accessing it right? Similar to your other stepIdsToSeenCount[id] being still nullable as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

huh interesting I was wondering why the compiler was realizing it wasn't null but that makes sense. Do you know a way around this? I tried wrapping this with a synchronized block and it doesn't seem to help 🤔

throw InngestInvalidConfigurationException("Maximum of 2 concurrency options allowed")
} else {
this.concurrency?.add(c)
this.concurrency!!.add(c)
}
return this
}
Expand Down
64 changes: 64 additions & 0 deletions inngest/src/test/kotlin/com/inngest/InngestFunctionConfigBuilderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<List<Concurrency>?>(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<List<Concurrency>?>(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<List<Concurrency>?>(listOf(Concurrency(5, null, ConcurrencyScope.FUNCTION)), config.concurrency)

assertFailsWith<InngestInvalidConfigurationException> {
InngestFunctionConfigBuilder()
.id("test-id")
.concurrency(5, null, ConcurrencyScope.ENVIRONMENT)
.build("app-id", "https://mysite.com/api/inngest")
}

assertFailsWith<InngestInvalidConfigurationException> {
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<List<Concurrency>?>(listOf(Concurrency(5, null, ConcurrencyScope.FUNCTION), Concurrency(7, "event.data.user_id", ConcurrencyScope.ENVIRONMENT)), config.concurrency)

assertFailsWith<InngestInvalidConfigurationException> {
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")
}
}
}
Loading