CELDEV-1343 EndpointConcurrencyLimiter - #34
Conversation
| .ifPresent(Semaphore::release); | ||
| } | ||
|
|
||
| private Optional<String> getConcurrencyLimit(Object handler) { |
There was a problem hiding this comment.
getConcurrencyLimit() sounds as though it returns the concurrency limit, but it actually returns the configuration key. Please rename it to getConcurrencyLimitName() and rename the local name variables to configName so their role is explicit.
| @Target({ TYPE, METHOD }) | ||
| public @interface EndpointConcurrencyLimit { | ||
|
|
||
| String value(); |
There was a problem hiding this comment.
The limiter identity travels through the implementation as a raw String, so a typo is detected only later at runtime. If the keys form a closed set, please use an enum annotation member; otherwise keep the annotation input as a string but convert it at the boundary to a small key type used by the maps and acquisition path.
There was a problem hiding this comment.
The limiter names are intentionally application-owned and therefore form an open set. Java enums would prevent applications from adding their own limiter names. A wrapper created after reading the annotation would still originate from a string and would not provide additional compile-time safety. Consumers can reuse one constant for the @Bean name and annotation value. Unknown names also fail closed on first use through the existing configuration check, so I’d keep the string-based contract.
|
|
||
| var response = new MockHttpServletResponse(); | ||
| assertFalse(limiter.preHandle( | ||
| new MockHttpServletRequest(), response, handlerMethod("limitedEndpoint"))); |
There was a problem hiding this comment.
This exercises the same handler method twice, so it does not prove the requirement that different endpoints sharing a name use one JVM-local semaphore. Please add a second handler annotated with @EndpointConcurrencyLimit("search"), acquire through one handler, and assert that the other receives HTTP 503 until the permit is released.
fpichler
left a comment
There was a problem hiding this comment.
Reviewed against CELDEV-1343.
https://synjira.atlassian.net/browse/CELDEV-1343