Repository navigation
Fix set(key, value, Expiration, SetOption) ignoring KEEPTTL and unix-timestamp expirations - #7316
Merged
Merged
Conversation
…l Spring Data Redis modules Signed-off-by: 심현민 <[email protected]>
isUnixTimestamp() was never checked either, so Expiration.unixTimestamp(...) fell into the relative-time branch and sent its absolute epoch-millis value as a PX delay, silently producing a multi-decade TTL instead of throwing. Mirrors the maintainer's own getEx() fix style (always PXAT + millis, no separate EXAT/seconds branch). Signed-off-by: 심현민 <[email protected]>
…supports them Spring Data Redis's Expiration class didn't gain isKeepTtl()/isUnixTimestamp() until 2.4 and 2.6 respectively. Modules 20-23 (SDR 2.0-2.3) have neither method at all, so the previous patch didn't compile there - reverted, no fix needed since the feature literally doesn't exist in that Expiration version. Modules 24-25 (SDR 2.4-2.5) have isKeepTtl() but not isUnixTimestamp() - kept only the KEEPTTL branch there. Added a matrix test to module 26 (SDR 2.6, the oldest module with both capabilities) to widen version coverage. Signed-off-by: 심현민 <[email protected]>
mrniko
requested changes
Aug 24, 2026
mrniko
left a comment
Member
There was a problem hiding this comment.
Please apply same changes to RedissonReactiveStringCommands
…nReactiveStringCommands The reactive sibling of RedissonConnection.set() had the exact same gap: only expiration.isPersistent() was checked, so Expiration.keepTtl() sent its reserved negative sentinel as a literal PX value (Redis rejects it), and Expiration.unixTimestamp(...) sent the absolute epoch value as a relative PX delay instead of PXAT, silently producing a multi-decade TTL. Same scope rules as the blocking fix: modules 20-23 unaffected (Expiration has neither method there), 24-25 get the KEEPTTL branch only, 26-41 get both branches. Confirmed by individually compiling all 12 affected modules. Verified RedissonReactiveClusterStringCommands does not override set(), so it inherits this fix automatically. Also checked RedissonClusterConnection (blocking) does not override set() either, so no further gap there. Added a reactive matrix test (all Expiration kinds x all SetOption values, checking TTL correctness for unixTimestamp rather than only exceptions) to modules 24, 25, 26, 30, 40, 41, run against a real Redis instance. Signed-off-by: 심현민 <[email protected]>
Member
|
Thanks for contribution |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7315
RedissonConnection.set(byte[], byte[], Expiration, SetOption)never checkedexpiration.isKeepTtl()orexpiration.isUnixTimestamp(), so both fell through to the relative-time branch.Expiration.keepTtl()sent its reserved negative sentinel to Redis as a literalPXvalue, which Redis rejects withERR invalid expire time in 'set' command.Expiration.unixTimestamp(...)sent its absolute epoch-millis value as aPXdelay instead of aPXATdeadline. This does not throw, so it silently produced a TTL decades in the future instead of the intended expiry time.Scope
Expiration.isKeepTtl()/isUnixTimestamp()were added to Spring Data Redis incrementally, so the fix isn't uniform across every module:Expirationhas neither method there, so neither bug can occurisKeepTtl()branch onlyI compiled all 16 potentially affected modules individually (
mvn -DskipTests compile, not a reactor-wide build) to confirm each module only gets the branch itsExpirationversion actually supports.Changes
RedissonConnection.set(byte[], byte[], Expiration, SetOption): added the missingisKeepTtl()branch (mirroring the already-correct siblingsetGet(byte[], byte[], Expiration, SetOption)) and, where supported, the missingisUnixTimestamp()branch (mirroring the already-mergedgetEx(byte[], Expiration)fix: always sendsPXATwith the millisecond value, no separate seconds/EXATbranch, matching that method's style).Expirationkind times everySetOption, checking TTL correctness for theunixTimestampcases rather than only checking for an exception. Added two more focused tests confirming theunixTimestampfix produces the right TTL both when theExpirationwas built with aMILLISECONDSvalue and with aSECONDSvalue.Testing
Ran the full matrix against a real Redis instance across modules spanning the supported version range:
Before this fix, the matrix failed on the
keepTtlrow under all threeSetOptionvalues with the exact error from the issue (ERR invalid expire time in 'set' command,PX -2000), and on theunixTimestamprow (in modules where thatExpirationkind exists) under all threeSetOptionvalues, reporting a ~56-year TTL instead of the intended ~2-minute one. All other combinations already passed and are unaffected by this change.Also reproduced and re-verified directly against
redis-cli(bypassing redisson entirely) to confirm the malformedKEEPTTLcommand and the fix'sKEEPTTL/KEEPTTL NX/KEEPTTL XX/PXAT ... NX/PXAT ... XXsyntax are all exactly what Redis expects.