Validate concurrency key is required when scope is account or env #81
Conversation
| 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") |
There was a problem hiding this comment.
the ticket originally said function or env scope but dev server shows this
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah I was considering a similar idea but it seemed more cumbersome to do in kotlin, so I went with the validation method
| } catch (e: Exception) { | ||
| throw InngestInvalidConfigurationException( | ||
| "Function id must be configured via builder: ${this.javaClass.name}", | ||
| ) | ||
| } |
There was a problem hiding this comment.
this is unnecessary because it's being thrown here
and catch (e: Exception) was swallowing other InngestInvalidConfigurationExceptions and rethrowing them for the wrong reason
- 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
albertchae
force-pushed
the
albert/inn-3354-concurrency-scope-validation
branch
from
a458849 to
b950969
Compare
albertchae
requested review from
djfarrelly,
darwin67,
jpwilliams and
tonyhb
as code owners
| ConcurrencyScope.FUNCTION -> {} | ||
| null -> {} |
There was a problem hiding this comment.
nit: can be a single else clause unless you wanted all expected values explicit.
There was a problem hiding this comment.
yeah I was thinking of making it exhaustive but I could be convinced either way
| if (this.concurrency == null) { | ||
| this.concurrency = mutableListOf(c) | ||
| } else if (this.concurrency!!.size == 2) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
|
@KiKoS0 going to merge as is but if you have additional opinions on the |
Summary
other InngestInvalidConfigurationExceptions
Checklist
Related