Report exact dbid for COPY in ACL LOG when db= access is denied

[enjoy-binbin]

Rework commandDbIdArgs so each helper returns the argv index of every dbid
argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr
directly, replacing the cmd->proc-based offset table. The caller now reads
the dbid value via getLongLongFromObject(argv[positions[i]], ...). New
db=-checked commands only need to implement their own *DbIdArgs helper.

The fix is for COPY: the old chain fell through to 0, so the "object"
field in ACL LOG was always showed "copy", it now reports the first
denied dbid.

Based on #3801, also see it for more details, DB ACL was added in #2309.

[coderabbitai]

📝 Walkthrough

Walkthrough

Helpers now return argv-index positions for DB-id tokens; callers (ACL checks and MULTI) read dbids via those positions and set key/index reporting from the offending argument position. Tests updated to assert the reported db argument value is included in DRYRUN errors and ACL LOG entries.

Changes

ACL Database ID Position Tracking

Layer / File(s)	Summary
Command DB-id helper interface src/server.h	Add documentation for commandDbIdArgs and add an int **positions out-parameter to DB-id helper prototypes.
DB-id helper implementations src/db.c	selectDbIdArgs, swapdbDbIdArgs, moveDbIdArgs, and copyDbIdArgs now allocate and return argv-position arrays (indices of DB-id tokens) instead of parsed DB-id values; copyDbIdArgs populates positions during its second pass.
ACL and MULTI consumers src/acl.c, src/multi.c	ACLSelectorCheckCmd treats helper return as positions, reads dbids from argv[positions[i]], sets *keyidxptr to the offending argument position and frees positions on all paths; queueMultiCommand parses transaction DB from mc->argv[positions[0]], updates transaction_db_id only when count>0, and always frees positions when non-NULL.
Tests: DRYRUN and ACL LOG tests/unit/acl-v2.tcl	Update ACL DRYRUN assertions to expect the specific denied database id (e.g., database 2) and add ACL LOG tests validating reason == "database" and object equals the offending db argument; add cleanup after invalid-db tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• valkey-io/valkey#3801: Also modifies copyDbIdArgs() and related COPY DB clause handling and tests.
• valkey-io/valkey#3804: Also touches ACLSelectorCheckCmd database-denial keyidxptr logic for commands that use cmd->get_dbid_args.

Suggested reviewers

• dvkashapov
• zuiderkwast
• nmvk

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main change: reporting the exact dbid for COPY commands in ACL LOG when database access is denied, which aligns with the core fix across all modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The PR description clearly explains the changes: reworking commandDbIdArgs to return argv positions, updating ACLSelectorCheckCmd to use those positions, and fixing a COPY command bug where ACL LOG was incorrectly showing 'copy' instead of the denied dbid.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/server.h (1)

2554-2554: ⚡ Quick win

Consider documenting the typedef parameters, especially the new positions out-parameter.

The commandDbIdArgs typedef signature has been extended with a new out-parameter, but there's no documentation explaining:

• The purpose of the positions parameter
• Whether positions can be NULL (appears optional based on PR context)
• Memory ownership (who allocates, who frees the returned array)
• The relationship between count and the positions array size

📝 Suggested documentation

+/* Helper function to get database ID arguments from a command.
+ * Returns an array of database IDs, with count set to the number of IDs.
+ * If positions is non-NULL, it will be set to a newly allocated array
+ * containing the argv index of each database ID (caller must free).
+ * Returns NULL if the command has no database ID arguments. */
 typedef int *commandDbIdArgs(robj **argv, int argc, int *count, int **positions);

As per coding guidelines: "Document why code exists, not just what it does; document all functions in C code"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server.h` at line 2554, Add a header comment above the typedef
commandDbIdArgs that documents each parameter: explain that argv/argc are the
command arguments, count is an out-parameter returning the number of DB-IDs
found, and positions is an optional out-parameter that, when non-NULL, will be
set to point to an array of integer positions (size equal to *count) indicating
where DB-ID arguments were found; state whether positions may be NULL, who
allocates the returned positions array (caller or callee), and who is
responsible for freeing it, and clarify the relationship between count and the
positions array length. Make the wording explicit and follow the file's existing
C-doc style so callers know allocation/ownership and nullability semantics for
commandDbIdArgs.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/server.h`:
- Line 2554: Add a header comment above the typedef commandDbIdArgs that
documents each parameter: explain that argv/argc are the command arguments,
count is an out-parameter returning the number of DB-IDs found, and positions is
an optional out-parameter that, when non-NULL, will be set to point to an array
of integer positions (size equal to *count) indicating where DB-ID arguments
were found; state whether positions may be NULL, who allocates the returned
positions array (caller or callee), and who is responsible for freeing it, and
clarify the relationship between count and the positions array length. Make the
wording explicit and follow the file's existing C-doc style so callers know
allocation/ownership and nullability semantics for commandDbIdArgs.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b7cb4114-eb5f-42f1-a283-10ac3852df05

📥 Commits

Reviewing files that changed from the base of the PR and between 1206103 and 664d2f1.

📒 Files selected for processing (5)

• src/acl.c
• src/db.c
• src/multi.c
• src/server.h
• tests/unit/acl-v2.tcl

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.70%. Comparing base (1206103) to head (396d5e3).
⚠️ Report is 4 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3888      +/-   ##
============================================
+ Coverage     76.66%   76.70%   +0.03%     
============================================
  Files           162      162              
  Lines         80728    80731       +3     
============================================
+ Hits          61893    61922      +29     
+ Misses        18835    18809      -26     

Files with missing lines	Coverage Δ
src/acl.c	92.65% <100.00%> (-0.03%)	⬇️
src/db.c	94.84% <100.00%> (-0.01%)	⬇️
src/multi.c	97.92% <100.00%> (+0.72%)	⬆️
src/server.h	100.00% <ø> (ø)

... and 21 files with indirect coverage changes

🚀 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.