Aias00 commented on code in PR #7166:
URL: https://github.com/apache/shenyu/pull/7166#discussion_r4071688485
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-token-limiter/src/main/java/org/apache/shenyu/plugin/ai/token/limiter/AiTokenLimiterPlugin.java:
##########
@@ -169,10 +170,11 @@ private String getCacheKey(final ServerWebExchange
exchange, final String tokenL
}
private void recordTokensUsage(final ReactiveRedisTemplate
reactiveRedisTemplate, final String cacheKey, final Long tokens, final Long
windowSeconds) {
- // Record token usage with expiration
- reactiveRedisTemplate.opsForValue()
- .increment(cacheKey, tokens)
- .flatMap(currentValue ->
reactiveRedisTemplate.expire(cacheKey, Duration.ofSeconds(windowSeconds)))
+ // The counter is given its window when it is created: re-issuing the
expiration after every increment
+ // would push the window forward, so a sustained traffic would never
reset the token budget.
+ final ReactiveValueOperations valueOperations =
reactiveRedisTemplate.opsForValue();
+ valueOperations.setIfAbsent(cacheKey, 0L,
Duration.ofSeconds(windowSeconds))
Review Comment:
This line throws `ClassCastException` at runtime on the first request that
records token usage.
`reactiveRedisTemplate` is declared as the **raw** type
`ReactiveRedisTemplate`, which is why passing a `Long` here compiles — but the
instance actually created in `AiTokenLimiterPluginHandler` is:
```java
new ShenyuReactiveRedisTemplate<>(connectionFactory,
ShenyuRedisSerializationContext.stringSerializationContext())
```
and `ShenyuRedisSerializationContext.stringSerializationContext()` sets
`StringRedisSerializer` for **both** key and value:
```java
RedisSerializer<String> serializer = new StringRedisSerializer();
return RedisSerializationContext.<String, String>newSerializationContext()
.key(serializer).value(serializer)...build();
```
So `0L` goes through `StringRedisSerializer#serialize(String)`, whose bridge
method does a `checkcast` to `String`. I verified this directly against
spring-data-redis 3.3.1:
```java
RedisSerializer raw = new StringRedisSerializer();
raw.serialize(Long.valueOf(0L));
// -> java.lang.ClassCastException:
// class java.lang.Long cannot be cast to class java.lang.String
```
The previous code never hit this because `increment` is a raw `INCR` that
does not go through the value serializer — Redis created the key as an integer
on its own.
Fix: pass a `String`, which is what the template is actually typed for:
```java
valueOperations.setIfAbsent(cacheKey, "0", Duration.ofSeconds(windowSeconds))
.then(valueOperations.increment(cacheKey, tokens))
```
`INCR` then still works (Redis parses `"0"` as an integer), and the read at
`AiTokenLimiterPlugin:132` (`opsForValue().get(cacheKey)`) is unaffected.
Worth double-checking the deserialization side too: with `V = String`,
`opsForValue().get()` returns a `String`, so whatever converts it to a number
needs to handle that (it already does today, since the old code also stored a
raw integer string).
Note that `testRecordTokensUsageGivesTheCounterItsWindowWhenItIsCreated`
cannot catch this — `ReactiveRedisTemplate` and `ReactiveValueOperations` are
both mocks, so no serializer is ever involved. A unit test with mocks will stay
green no matter what type you hand to `setIfAbsent`. If you want real coverage,
either assert against a `ReactiveValueOperations<String, String>` (so the
compiler rejects a `Long`), or exercise the serializer directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
