Skip to content

fix(dynamodb): paren-aware comma split in parse_update_clauses#368

Merged
vieiralucas merged 2 commits intofaiscadev:mainfrom
Bowbaq:fix/dynamodb-list-append-comma-split
Apr 14, 2026
Merged

fix(dynamodb): paren-aware comma split in parse_update_clauses#368
vieiralucas merged 2 commits intofaiscadev:mainfrom
Bowbaq:fix/dynamodb-list-append-comma-split

Conversation

@Bowbaq
Copy link
Copy Markdown
Contributor

@Bowbaq Bowbaq commented Apr 14, 2026

Heads up — this PR was written by an AI agent (Claude Code) under my direction. I don't write Rust day-to-day, so I can't speak to the idiom-level quality of the diff off the top of my head. The test coverage and reasoning should stand on their own; happy to revise anything that doesn't fit the project's style.

Summary

UpdateItem with a list_append expression silently no-oped:

SET #0 = list_append(#0, :0)

The item's target attribute was left unchanged even though the call returned no error, and the attribute already existed as an empty list (so if_not_exists was not the issue). Confirmed broken in v0.7.1 and v0.9.0 against real DynamoDB.

Root cause

parse_update_clauses split each action clause's content on every comma naively:

let assignments = content.split(',').map(...).collect();

The expression #0 = list_append(#0, :0) was torn apart at the comma inside the function call, producing two bogus tokens:

  1. #0 = list_append(#0 — sent to apply_set_list_append as rest = "#0" (no closing ))
  2. :0) — treated as a separate malformed assignment, silently ignored

In apply_set_list_append, rest.strip_suffix(')') found no ) and returned early — no write, no error.

Fix

Replace the naive split(',') with the existing split_on_top_level_keyword(content, ",") helper, which is already paren-depth-aware and is used by split_on_and / split_on_or for exactly the same reason. Commas inside balanced parentheses are now left intact.

Test plan

Five new unit tests (all failing before the fix):

  • test_parse_update_clauses_list_append_single_assignment — parse level: list_append(a, b) stays as 1 assignment, not 2
  • test_parse_update_clauses_list_append_mixed_with_plain_set — parse level: outer comma still splits two SET assignments correctly
  • test_list_append_into_empty_list — exact repro from the bug report (empty existing list)
  • test_list_append_into_nonempty_list — append to a non-empty existing list
  • test_list_append_combined_with_plain_setlist_append and plain SET in the same expression both apply
  • cargo test -p fakecloud-dynamodb — 98 pass (was 93, +5 new)
  • cargo clippy -p fakecloud-dynamodb -- -D warnings — clean
  • cargo fmt --check — clean
  • Not run: real-AWS behavioral parity. I don't have credentials configured for this. The fix matches AWS's documented UpdateExpression SET syntax for list_append, and the bug was purely in the comma tokeniser.

Summary by cubic

Fixes parenthesis-aware splitting of SET clauses in fakecloud-dynamodb UpdateExpression parsing so list_append(#a, :b) applies correctly instead of no-oping.

  • Bug Fixes

    • Replace naive comma split in parse_update_clauses with split_on_top_level_keyword; add tests for list_append parsing and execution, including mixed SETs and empty/non-empty list appends.
  • Refactors

    • Run cargo fmt (style-only).

Written for commit 89f8716. Summary will update on new commits.

list_append(#a, :b) was silently no-oping because parse_update_clauses
split clause content on all commas without regard for parentheses depth.
SET #0 = list_append(#0, :0) was torn into two tokens at the inner comma:
  - "#0 = list_append(#0"  → apply_set_list_append got rest="#0", no ')' → early return
  - ":0)"                  → treated as a malformed assignment, silently dropped

Fix: replace content.split(',') with split_on_top_level_keyword(content, ","),
the existing paren-depth-aware helper already used by split_on_and/split_on_or.

Five new unit tests added (all failing before the fix):
- parse level: list_append(a,b) stays as 1 assignment; outer comma still
  splits two SET assignments
- integration: empty-list repro, nonempty-list append, list_append mixed
  with a plain SET in the same expression

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vieiralucas
Copy link
Copy Markdown
Member

Thanks for the thorough debugging and the test coverage — root cause analysis is spot on, and reusing the existing split_on_top_level_keyword helper is exactly the right call. Merging now.

@vieiralucas vieiralucas merged commit fba5b92 into faiscadev:main Apr 14, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants