Visitar URL original
fix(#10694): a Redis key starting with $ is no longer the same key as the one without it by robfrank · Pull Request #10791 · ArcadeData/arcadedb · GitHub
Skip to content

fix(#10694): a Redis key starting with $ is no longer the same key as the one without it - #10791

Merged
robfrank merged 2 commits into
mainfrom
fix/10694-redis-dollar-key-aliasing
Oct 27, 2026
Merged

fix(#10694): a Redis key starting with $ is no longer the same key as the one without it#10791
robfrank merged 2 commits into
mainfrom
fix/10694-redis-dollar-key-aliasing

Conversation

@robfrank

@robfrank robfrank commented Oct 27, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Collaborator

Closes #10694

Summary

A database-backed Redis key lives in the database's global-variable map, whose accessors strip one leading $ because for SQL $name and name are one variable. On the Redis wire that made <db>.$k and <db>.k one key (SET overwrote, GETDEL/INCR hit the other name, "" and $ were one key), and the redis query language stripped the same $ deliberately (PR #8309 review), so both surfaces agreed on the wrong answer.

The fix keeps the map as the storage (Redis keys are still SQL variables: SET <db>.k is $k in SQL, which RedisWTest.globalVariablesSharedWithSQL pins) and escapes the key in ONE place, the new RedisVariableKeys.toVariableName: a key that starts with $ gets one more $, which the map's strip removes again, so the map is keyed by the client's key verbatim. A key without a leading $ is returned as is, with no allocation. Every call that hands a Redis key to the map goes through it:

  • wire: resolveKeyAndDatabase (GET/SET incl. NX/XX, GETDEL, EXISTS, INCR*/DECR*), transient HSET, transient HDEL, transient HGET/HMGET/HEXISTS (getTransientValue)
  • redis query language: SET/GET/GETDEL/EXISTS/INCR*/DECR*, and the MULTI/EXEC overlay publish. normalizeRamKey now returns the client's key unchanged (still refusing a reserved name), and normalizeRamReadKey is gone.

LocalDatabase is untouched: the $ strip is correct for SQL. The global-variable map is a runtime-only ConcurrentHashMap (LocalDatabase.globalVariables), never persisted, so there is no stored data under the old aliased names to migrate across an upgrade.

Behavior change: in the redis query language SET $seq 1 followed by GET seq no longer returns 1. Issue8254RedisRetryRamCommandDuplicationTest.dollarPrefixedKeyIsVisibleToAPlainGetInTheSameBlock pinned the old aliasing and is rewritten as dollarPrefixedKeyIsVisibleUnderItsOwnNameInTheSameBlock (the overlay/map agreement it was protecting is still asserted, under the key's own name).

Test plan

  • Issue10694RedisDollarKeyAliasingTest (new, 8 tests): round-trip property over k, $k, $$k, "", $, $$, a.b, $a.b, non-ASCII, 4 KB keys, for the database-prefixed, SELECTed and connection-local forms; the issue's repro (SET, INCR/DECR, GETDEL, EXISTS, SET NX/XX, <db>. vs <db>.$); transient HSET/HGET/HDEL/HEXISTS; query language vs wire agreement; MULTI/EXEC overlay; SQL $shared still reads the plain key. 7 of 8 red before the fix (the connection-local control was green).
  • Full redisw module suite: 224 tests; 16 errors in the first run were Unable to listen to a HTTP port in the configured port range 2480 - 2489 (other agents' servers), the two affected classes re-run green.

Coverage

Entry point Status
Wire GET/SET/SET NX/SET XX/GETDEL/EXISTS/INCR/INCRBY/INCRBYFLOAT/DECR/DECRBY (prefixed and SELECT) fixed, tested
Wire transient HSET / HGET / HMGET / HEXISTS / HDEL fixed, tested (HMGET shares getTransientValue with HGET)
redis language SET/GET/GETDEL/EXISTS/INCR*/DECR* fixed, tested
redis language MULTI/EXEC overlay publish fixed, tested
Connection-local bucket never aliased (plain ConcurrentHashMap), now pinned by the round-trip as the control
SQL $var / LocalDatabase strip intentionally unchanged

Known gaps

Adversarial pass

The Phase 1.5 independent-reviewer subagent could not be spawned in this run (no subagent tool available to the agent). A self-review covered: reserved-name handling with the escape ($parent is still refused, as before; consistent across both surfaces since normalizeRamKey validates the same name the map stores), HDEL's RID detection (a $... key is never a RID), and the overlay/map key agreement (both use the raw key, the map call escapes it).

Review cycles

Deferred items

  • "this is a behavior change for users who relied on SET $x followed by GET x in the redis language ... It deserves a release-note line." - release notes are written by the maintainer at release time; the behavior change is called out in the Summary above for that.

Final state

clean-approval

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Redis keys beginning with $ are now kept distinct from otherwise identical keys without the prefix.
    • Reads, writes, increments, deletes, existence checks, and hash operations now consistently use the same key identity.
    • Key behavior is consistent across database selection, connection-local storage, transactions, and the Redis query language.

@robfrank @claude
fix(#10694): a Redis key starting with $ is no longer the same key as…
fcefb91 
… the one without it

Both the RESP wire and the redis query language store a database-backed key
through RedisVariableKeys.toVariableName, which escapes a leading $ so the
global-variable map's SQL strip removes exactly what was added.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@robfrank robfrank added this to the 26.11.1 milestone Oct 27, 2026
@robfrank robfrank added bug redis Redis wire protocol module (redisw) labels Oct 27, 2026
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8de3f5c4-1440-456e-8fdf-cb31b231ee18
Commits

Reviewing files that changed from the base of the PR and between aef768d and 2ff53b7.

Files selected for processing: 6
  • redisw/src/main/java/com/arcadedb/redis/RedisNetworkExecutor.java
  • redisw/src/main/java/com/arcadedb/redis/RedisVariableKeys.java
  • redisw/src/main/java/com/arcadedb/redis/query/RedisQueryEngine.java
  • redisw/src/test/java/com/arcadedb/redis/Issue10694RedisDollarKeyAliasingTest.java
  • redisw/src/test/java/com/arcadedb/redis/Issue8254RedisRetryRamCommandDuplicationTest.java
  • redisw/src/test/java/com/arcadedb/redis/RedisVariableKeysTest.java

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

Redis database-backed key access now preserves leading dollar signs by converting keys before global-variable map access. RAM query operations use the same conversion for database access while retaining client keys for overlay lookups. Tests cover key distinction across wire, query, and transaction operations.

Changes: Redis key identity

Layer / File(s) Summary
Database-backed key mapping
redisw/src/main/java/com/arcadedb/redis/RedisVariableKeys.java, redisw/src/main/java/com/arcadedb/redis/RedisNetworkExecutor.java, redisw/src/test/java/com/arcadedb/redis/Issue10694RedisDollarKeyAliasingTest.java, redisw/src/test/java/com/arcadedb/redis/RedisVariableKeysTest.java		 RedisVariableKeys.toVariableName adds one leading $ when a key already starts with $. RedisNetworkExecutor uses the converted name for database-backed keys and transient hash operations. Tests cover conversion and distinct keys across database-prefixed, selected-database, and connection-local access.
RAM query key access
redisw/src/main/java/com/arcadedb/redis/query/RedisQueryEngine.java, redisw/src/test/java/com/arcadedb/redis/Issue10694RedisDollarKeyAliasingTest.java, redisw/src/test/java/com/arcadedb/redis/Issue8254RedisRetryRamCommandDuplicationTest.java		 RAM query operations retain the client key for overlay access and convert it for database map access. Tests cover query-language reads and writes, transactions, and distinct plain and dollar-prefixed keys.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: lvca

Merge Risk: Minimal · up to 2ff53

No unresolved issue is established that would prevent merging after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary fix: Redis keys beginning with $ no longer alias keys without $.
Description check Passed The description is detailed and covers the change, motivation, related issue, behavior change, implementation scope, tests, coverage, known gaps, and review status. It does not use the exact template …
Linked Issues check Passed Issue #10694 requires Redis keys to remain opaque and keeps k, $k, $$k, the empty key, and $ distinct for database-prefixed and connection-local access. RedisVariableKeys.toVariableName adds…
Out of Scope Changes check Passed The production changes directly implement issue #10694. The query-language changes keep the same Redis key identity across the two Redis surfaces. The utility test, integration tests, and update to th…
Full details: Description check

Explanation

The description is detailed and covers the change, motivation, related issue, behavior change, implementation scope, tests, coverage, known gaps, and review status. It does not use the exact template headings and does not explicitly confirm mvn clean package, but it is mostly complete.

Full details: Linked Issues check

Explanation

Issue #10694 requires Redis keys to remain opaque and keeps k, $k, $$k, the empty key, and $ distinct for database-prefixed and connection-local access. RedisVariableKeys.toVariableName adds one leading $ only when needed before global-variable map access. RedisNetworkExecutor applies it to resolved database keys and transient hash access. RedisQueryEngine applies it to RAM reads, writes, deletes, increments, existence checks, and MULTI/EXEC publication. Issue10694RedisDollarKeyAliasingTest covers representative keys, database prefixes, SELECT, connection-local access, affected commands, query-language agreement, transactions, and unchanged SQL access.

Full details: Out of Scope Changes check

Explanation

The production changes directly implement issue #10694. The query-language changes keep the same Redis key identity across the two Redis surfaces. The utility test, integration tests, and update to the existing transaction test verify the fix. No unrelated production behavior or unrelated feature is introduced.

Full details: Docstring Coverage

Explanation

Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 32 functions across 6 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codacy-production

codacy-production Bot commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

🟢 Coverage 100.00% diff coverage · -4.84% coverage variation

Metric Results
Coverage variation ✅ -4.84% coverage variation
Diff coverage ✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (aef768d) 251100 218555 87.04%
Head commit (2ff53b7) 283802 (+32702) 233274 (+14719) 82.20% (-4.84%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#10791) 15 15 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown

Review

Overall this is a clean, well-scoped fix. I found no security hazard, so I did not add the label.

What I checked

  • Escaping once in RedisVariableKeys.toVariableName is sound. LocalDatabase strips exactly one $ in get/set/setIfAbsent/setIfPresent/compute, so $k becomes $$k and is stored as $k. Plain keys are unchanged and allocation-free.
  • Reserved names still get refused. validateVariableName strips a $ again, so $parent is rejected on the wire and in the query language alike, as the PR says.
  • All the globalVariable call sites in redisw go through the helper. resolveKeyAndDatabase covers GET/SET/NX/XX/GETDEL/EXISTS/INCR*. I also saw hSet, hDel, getTransientValue and the overlay publish covered. The connection-local path is correctly left alone.
  • The overlay and the map agree because the overlay is keyed by the raw key and the escape is applied only at map calls.
  • Tests are thorough: a round-trip property over the awkward keys (empty, $, $$, non-ASCII, 4 KB), the issue's repro, the transient hash, and the query language against the wire. The rewritten The MULTI/EXEC retry fix leaves the RAM commands in the same block applied once per attempt, so a rolled-back EXEC still advances the counters it contains #8254 test documents the intentional behavior change.

Minor notes (non-blocking)

  1. In hSet, the escaped name is also what gets stored as the keys[i] map key. Make sure nothing later (an error message, or a reply echoing the key) reads keys[i] expecting the client's original id.
  2. normalizeRamKey is now a validate-and-return-input pattern that discards the result of validateVariableName. This is fine, but a short // throws if reserved at the call sites would help future readers.
  3. Compatibility: this is a behavior change for users who relied on SET $x followed by GET x in the redis language, and for SQL users who read $$k-style variables. It deserves a release-note line.
  4. RedisVariableKeys.toVariableName has no direct unit test. The round-trip covers it, but a tiny pure unit test for null-free inputs ("", "$", "k") would be cheap.

I have not run the tests locally. Looks good to merge once CI is green.

@mergify

mergify Bot commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

@robfrank @claude
address review: add a unit test for RedisVariableKeys.toVariableName
2ff53b7 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown

Review of #10791 (Redis $ key aliasing)

Verdict: looks good, no security concerns, no blocking issues.

What I checked

  • RedisVariableKeys.toVariableName adds one $ to keys that start with $, so the single-$ strip in LocalDatabase.set/get/computeGlobalVariable gives back the client's key verbatim. Other keys are returned as the same instance, so there is no allocation on the hot path.
  • Every wire call site I could find goes through the helper: resolveKeyAndDatabase, transient HSET, HDEL, and getTransientValue. So do the redis query language paths (set/get/getdel/compute/overlay publish). A grep of redisw/src/main for GlobalVariable found no unescaped caller.
  • Reserved-name handling stays consistent. The map strips one $, then validateVariableName strips another, so $parent is still refused. normalizeRamKey validates the raw key, which gives the same answer, so the refusal still happens when the command runs and not at publish time.
  • Nothing new here for an attacker to exploit. The keyspace is only made more precise, and the change adds no dependencies.

Tests

  • The new tests are good: a round trip over aliasing pairs, empty, unicode and 4KB keys; the SQL-visibility pin; and the query-language/wire agreement test.
  • I ran none of them myself.

Minor suggestions (non-blocking)

  1. This is a behavior change for the redis query language (SET $seq 1 then GET seq no longer matches). The PR description already notes it. A changelog or release-notes line would help users who relied on the old aliasing.
  2. RedisVariableKeys.toVariableName is public. Package-private would be enough for the com.arcadedb.redis classes, but RedisQueryEngine lives in the query subpackage, so public is needed. Fine as is.
  3. Data already stored under a $k name from before this change now reads under a different key. This is a runtime-only map, so nothing persists across restarts. A one-line mention in the PR would confirm that.

@codecov

codecov Bot commented Oct 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.98%. Comparing base (aef768d) to head (2ff53b7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../java/com/arcadedb/redis/RedisNetworkExecutor.java 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main   #10791      +/-   ##
============================================
+ Coverage     78.96%   78.98%   +0.01%     
  Complexity     4766     4766              
============================================
  Files          2298     2299       +1     
  Lines        251100   251101       +1     
  Branches      54762    54762              
============================================
+ Hits         198290   198322      +32     
+ Misses        32586    32555      -31     
  Partials      20224    20224

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lvca lvca mentioned this pull request Oct 27, 2026
Merged
@robfrank
robfrank merged commit 8090fed into main Oct 27, 2026
37 of 39 checks passed
@robfrank robfrank mentioned this pull request Oct 28, 2026
Closed
robfrank added a commit that referenced this pull request Nov 5, 2026
@robfrank
fix(#10694): a Redis key starting with $ is no longer the same key as…
2e3bdc6 
… the one without it (#10791)

(cherry picked from commit 8090fed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug redis Redis wire protocol module (redisw)

Projects

None yet

Milestone

26.11.1

Development

Successfully merging this pull request may close these issues.

On the Redis wire a key starting with $ is the same key as the one without it: GET, SET, GETDEL, EXISTS and INCR alias <db>.$k onto <db>.k, so one write overwrites another key

1 participant

@robfrank