fix(redis): reject invalid key types before generating update commands - #2437
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a Redis plugin bug where RedisScriptExecutor.update could throw a NullPointerException when comparing Redis key types that may be null (e.g., when a key does not exist and getKeyType returns null). The fix makes the type comparison null-safe to prevent crashes during key updates.
Changes:
- Replace
oldKey.getType().equals(newKey.getType())withObjects.equals(oldKey.getType(), newKey.getType())to avoid NPEs when either type isnull. - Add the required
java.util.Objectsimport.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
openai0229
left a comment
There was a problem hiding this comment.
This only moves the null dereference. When the new type is null and the old type is non-null, typeChanged becomes true and RedisDataType.fromCode(newKey.getType()).getScript() still dereferences null. When both types are null, the else branch does the same through typeSource. Please validate both old/new type contracts before either branch (including unknown codes), define whether invalid updates should be rejected or treated as no-op, and add focused tests for one-null, both-null, and unknown-type cases.
…nd#2437) Reviewer feedback: Objects.equals only prevented the .equals() NPE but RedisDataType.fromCode(newKey.getType()) still NPEs when newKey.getType() is null (key doesn't exist). Add null guards for BOTH old and new types before the typeChanged check; if either is null, fall through to the rename path instead of entering the type-change branch. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
|
Good catch — thank you. The previous |
|
Thanks for the follow-up. The current guard still does not make this update path safe.
The normal UI only submits Please validate both old and new types against the supported set before |
Address review on OtterMind#2437: the previous null-guard only prevented the .equals() NPE, but RedisDataType.fromCode(null/none/unknown) returns NONE and falls back to StringTypeScript, so malformed/stale input still ran type-specific writes, DEL, RENAME (failing with "no such key"), or EXPIRE. Validate both old and new key types against {string,list,set,zset,hash,stream} up front and reject (BusinessException) before any script is generated, so no Redis command can be emitted for invalid input. Tests cover old-null, new-null, both-null, none, unknown code, and rename of a missing key — each asserts the update is rejected. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the thorough review — addressed in
Full redis module suite: 59 tests, 0 failures. |
…tterMind#2436) oldKey.getType().equals(newKey.getType()) NPEs when either type is null (getKeyType returns null for a non-existent key). Use Objects.equals. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
…nd#2437) Reviewer feedback: Objects.equals only prevented the .equals() NPE but RedisDataType.fromCode(newKey.getType()) still NPEs when newKey.getType() is null (key doesn't exist). Add null guards for BOTH old and new types before the typeChanged check; if either is null, fall through to the rename path instead of entering the type-change branch. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
Address review on OtterMind#2437: the previous null-guard only prevented the .equals() NPE, but RedisDataType.fromCode(null/none/unknown) returns NONE and falls back to StringTypeScript, so malformed/stale input still ran type-specific writes, DEL, RENAME (failing with "no such key"), or EXPIRE. Validate both old and new key types against {string,list,set,zset,hash,stream} up front and reject (BusinessException) before any script is generated, so no Redis command can be emitted for invalid input. Tests cover old-null, new-null, both-null, none, unknown code, and rename of a missing key — each asserts the update is rejected. Co-Authored-By: Claude <noreply@anthropic.com>
Constraint: Preserve the existing both-null no-op while treating single-null update inputs as malformed Rejected: Leaving key==null as a pass-through | It still permits stale direct API input to reach rename/update paths Confidence: high Scope-risk: narrow Tested: mvn -B -f chat2db-community-server/pom.xml -pl :chat2db-community-redis -am -Dmaven.test.skip=false -DskipTests=false -Dtest=RedisScriptExecutorUpdateTest -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.test.failure.ignore=false test Tested: git diff --check
3da229e to
e29b12b
Compare
…-type-npe-2436-followup
Signed-off-by: liuhy <liuhongyu@apache.org>
|
@openai0229 The requested Redis null-type handling is addressed on current head |
openai0229
left a comment
There was a problem hiding this comment.
Re-reviewed synchronized head 545fe7c. The previous requested changes are addressed: both present key types are validated before command generation, while null key objects retain create/delete and no-op semantics. No blocking findings.
The synchronized Redis reactor passed 316 tests (66 in the Redis module), with zero failures or skips, and all 8 modules packaged successfully. The three PR files are unchanged from the previously reviewed version, where 14 invalid inputs emitted no commands and captured commands were replayed/read back in isolated Redis 7.4.2 (40 cases).
Related issue
Closes #2436
Summary
Redis key updates with a missing or unsupported type can throw a NullPointerException or fall back to string commands, deleting the old key and writing a replacement. Validate every present old/new key before generating commands and reject null, blank, none, or unknown type codes with BusinessException.
A null key object remains a valid create/delete signal; two null objects remain a no-op. Supported types are string, list, set, zset, hash, and stream.
Affected surfaces
Verification
git diff --check main...HEAD: passed.mvn -B -f chat2db-community-server/pom.xml -pl :chat2db-community-redis -am -Dmaven.test.skip=false -DskipTests=false '-Dsurefire.includes=**/*Test.java' -Dmaven.test.failure.ignore=false package: passed on the synchronized branch; 317 tests, 0 failures, 0 errors, 0 skipped. Redis module: 66 tests. All 8 reactor modules built successfully.Risk and compatibility
Reviewer map
RedisScriptExecutor.updateandvalidateKeyType; regressions inRedisScriptExecutorUpdateTest.Contributor declaration
AI assistance: The original patch and follow-up fixes were authored with Claude Code. Codex re-reviewed the final implementation, ran the tests and Redis command verification above, and synchronized the latest main.