Skip to content

fix(dynamodb): handle IN (...) and SET list[N] UpdateExpression#261

Merged
vieiralucas merged 2 commits intofaiscadev:mainfrom
Bowbaq:fix-dynamodb-in-and-comparison-ops
Apr 12, 2026
Merged

fix(dynamodb): handle IN (...) and SET list[N] UpdateExpression#261
vieiralucas merged 2 commits intofaiscadev:mainfrom
Bowbaq:fix-dynamodb-in-and-comparison-ops

Conversation

@Bowbaq
Copy link
Copy Markdown
Contributor

@Bowbaq Bowbaq commented Apr 12, 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

Three silent-fallthrough bugs in the DynamoDB expression evaluator. All three hit callers using aws-sdk-*'s expression builder.

1. IN (...) silently accepted everything

evaluate_single_filter_condition had no IN branch. #s IN (:a, :b) fell through to evaluate_single_key_condition, matched no operator in its <=/>=/<>/=/</> loop, and returned true at the bottom of the function. Since evaluate_condition delegates to evaluate_filter_expression (post d3fe7c4), the same silent-true affected ConditionExpressions — every conditional write with an IN clause was silently accepted regardless of actual state.

Fix: new parse_in_expression / evaluate_in_match helpers wired into evaluate_single_filter_condition before its existing fallthrough. Case-insensitive, trimmed, tolerates missing spaces after commas — hand-built expressions like "#status IN (" + strings.Join(keys, ",") + ")" produce no-space forms that are valid per the DynamoDB grammar. Missing attributes never match.

2. SET #list[N] = :v silently created a top-level key

apply_set_assignment called resolve_attr_name on the whole "#items[0]" token. resolve_attr_name only matches #-prefixed names verbatim, so the [0] caused a miss — the function then did item.insert("#items[0]", :item), producing a new top-level attribute literally named "#items[0]" rather than mutating the L array.

Fix: new parse_list_index_suffix splits #items[0] into ("#items", 0) before the name-ref lookup. New assign_list_index walks the L array and replaces slot N (or appends if N == len, no-op beyond). Non-list-index SET paths (if_not_exists, list_append, arithmetic, plain assignment) are untouched. Deeper nested paths (a.b[0], list[0].field) are still unsupported and left for a follow-up.

3. attribute_exists (x) with a space silently passed

aws-sdk-go v2's expression.NewBuilder emits function calls with a space between the name and the opening paren:

(attribute_exists (#0)) AND ((attribute_not_exists (#1)) OR (#1 = :0))

extract_function_arg was only matching attribute_exists( with no space, so any ConditionExpression built through the official SDK's expression builder fell through every branch of evaluate_single_filter_condition, hit evaluate_single_key_condition, failed to match any comparison operator, and returned true at the bottom of the function. Every attribute_exists / attribute_not_exists check that went through the SDK builder was silently accepted regardless of item state — including the classic "claim-lease" compound pattern above.

Caught this by tracing the exact string fakecloud received while running orderbot's ReceivingSessionService.ClaimLease integration tests:

cond := expression.AttributeExists(expression.Name("store_id")).And(
    expression.AttributeNotExists(expression.Name("active_viewer_tab_id")).Or(
        expression.Name("active_viewer_tab_id").Equal(expression.Value(tabID)),
    ),
)

Fix: extract_function_arg now accepts both func_name( and func_name (, mirroring the pattern already in place for begins_with and contains in the filter leaf.

Test plan

  • 11 new unit tests in service.rs:
    • IN: filter match, filter no-match, no-space-after-comma form, missing attribute, compound (#s IN (:a, :b)) AND (#p = :h) (both match and mismatch), condition match, condition no-match.
    • SET list-index: replace slot 0 of a 2-element list, replace slot 1 of a 3-element list, literal attribute name (no #ref).
    • Function-with-space: the exact (attribute_exists (#0)) AND ((attribute_not_exists (#1)) OR (#1 = :0)) shape against four item states (free, missing, held-by-other, self-held).
  • cargo test -p fakecloud-dynamodb — 68 pass (was 57, +11 new)
  • cargo test -p fakecloud-e2e --test dynamodb — 36 pass
  • cargo clippy -p fakecloud-dynamodb -- -D warnings — clean
  • cargo fmt --check — clean
  • End-to-end verification: ran the downstream orderbot integration suite against a locally-built binary from this branch via FAKECLOUD_BIN=/tmp/fakecloud-debug go test ./lambdas/api/services/... — 114 service tests pass, including the three that were previously failing on v0.7.1: TestClaimLease_NotFound, TestClaimLease_RejectsWhenHeldByOtherTab, TestLeaseLifecycle_TwoTabs.
  • Not run: real-AWS behavioral parity. I don't have credentials configured, and there's no public workflow that runs the real-AWS suite on contributor PRs. The fixes match AWS's documented ConditionExpression operators and UpdateExpression SET syntax for list elements.

Both are silent-fallthrough bugs in the expression evaluator.

evaluate_single_filter_condition had no handling for IN. Expressions
like "#s IN (:a, :b)" fell through to evaluate_single_key_condition,
hit none of the comparison operators, and returned `true` — so every
item matched every IN filter, and because evaluate_condition now
delegates to evaluate_filter_expression, every conditional write with
an IN clause was silently accepted regardless of state.

apply_set_assignment called resolve_attr_name on the whole "#items[0]"
token, which never hits the name map, then did item.insert("#items[0]",
:v) — producing a top-level attribute literally named "#items[0]"
rather than mutating the L array.

Fix: new parse_in_expression / evaluate_in_match helpers wired into
evaluate_single_filter_condition before the key-condition fallthrough
(case-insensitive " IN ", tolerates no-space-after-comma forms that
hand-built "IN (" + strings.Join(keys, ",") + ")" expressions produce).
New parse_list_index_suffix / assign_list_index helpers split the [N]
suffix off before resolve_attr_name, then replace slot N of the L array
(or append if N == len, no-op beyond).

10 new unit tests: IN match/no-match/no-space/missing-attr/compound in
both filter and condition paths, SET list-index replace slot 0, replace
slot 1 of 3, literal attribute name. Each was verified to fail against
upstream/main before the fix and pass after.

Co-Authored-By: Claude Opus 4.6 (1M context) <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

@Bowbaq Bowbaq force-pushed the fix-dynamodb-in-and-comparison-ops branch from 627ef70 to ec2777f Compare April 12, 2026 02:01
aws-sdk-go v2's expression.NewBuilder emits function calls with a
space between the name and the opening paren:

    (attribute_exists (#0)) AND ((attribute_not_exists (faiscadev#1)) OR (faiscadev#1 = :0))

extract_function_arg was only matching "attribute_exists(" with no
space, so any ConditionExpression built through the official SDK's
builder fell through every branch of evaluate_single_filter_condition,
hit evaluate_single_key_condition, failed to match any comparison
operator, and returned `true` at the bottom of the function. Every
claim-lease / "update if exists" write was silently accepted
regardless of item state.

The begins_with and contains branches already accept both forms
("begins_with(" and "begins_with (") — this mirrors that pattern
for the attribute_exists / attribute_not_exists path.

Verified fix end-to-end against the orderbot claim-lease tests
(TestClaimLease_NotFound, TestClaimLease_RejectsWhenHeldByOtherTab,
TestLeaseLifecycle_TwoTabs) by running them with FAKECLOUD_BIN
pointing at a locally-built binary — 114 service tests pass, 0 fail.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vieiralucas vieiralucas merged commit f3968eb into faiscadev:main Apr 12, 2026
21 checks passed
@Bowbaq Bowbaq deleted the fix-dynamodb-in-and-comparison-ops branch April 12, 2026 08:17
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