Skip to content

fix(dynamodb): handle AND/OR/parens in ConditionExpression#257

Merged
vieiralucas merged 1 commit intofaiscadev:mainfrom
Bowbaq:fix-condition-expression-compound-parens
Apr 11, 2026
Merged

fix(dynamodb): handle AND/OR/parens in ConditionExpression#257
vieiralucas merged 1 commit intofaiscadev:mainfrom
Bowbaq:fix-condition-expression-compound-parens

Conversation

@Bowbaq
Copy link
Copy Markdown
Contributor

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

evaluate_condition had no handling for AND, OR, or parentheses — it passed the full ConditionExpression to parse_simple_comparison, which splits on the first <>/= it finds. Any consumer composing conditions through aws-sdk's expression builder hit this: the builder wraps clauses in parens by default, so (#0 <> :0) AND (#0 <> :1) split into left="(#0 " and a garbage right-hand side, and the result was accidental — silent failure for <>, silent success for =.

Repro against 0.7.0:

// item { state: "active" }, names { #s → state }, values { :c → "complete", :f → "failed" }
evaluate_condition("(#s <> :c) AND (#s <> :f)", Some(&item), &names, &values)
// → Err(ConditionalCheckFailedException)  — both clauses are true, should be Ok

Fix: evaluate_condition now delegates to a recursive helper that mirrors evaluate_filter_expression — split on OR, split on AND, strip outer parens, recurse, evaluate a single leaf at the bottom. The leaf (attribute_exists / attribute_not_exists / = / <>) is extracted into evaluate_single_condition with no behavioral change, reusing existing helpers.

Test plan

  • Six new unit tests in service.rs covering bare <>, parenthesised <>/=, compound (<>) AND (<>), compound (=) AND (=) mismatch, and compound (=) OR (=). Each was verified to fail against main before the fix and pass after.
  • cargo test -p fakecloud-dynamodb — 52 pass
  • cargo test --workspace --exclude fakecloud-e2e — all pass
  • cargo test -p fakecloud-e2e --test dynamodb — 36 pass
  • cargo clippy --workspace -- -D warnings — clean
  • cargo fmt --check — clean
  • Not run: real-AWS behavioral parity. I don't have credentials configured for this, and there's no public workflow that runs the real-AWS suite on contributor PRs. The fix matches AWS's documented ConditionExpression grammar (AND, OR, NOT, parentheses), and the bug was specifically that fakecloud didn't.

Summary by cubic

Fix ConditionExpression evaluation in fakecloud-dynamodb to correctly support AND, OR, and parentheses. This aligns behavior with DynamoDB and fixes wrong results from SDK-built expressions.

  • Bug Fixes
    • Added recursive evaluation with correct precedence (OR, then AND) and outer-paren stripping; leaf checks for attribute_exists, attribute_not_exists, =, and <> are unchanged.
    • Added unit tests for parenthesized and compound cases to prevent regressions.

Written for commit 867427f. Summary will update on new commits.

evaluate_condition passed the whole ConditionExpression string to
parse_simple_comparison, which splits on the first `<>`/`=` it finds.
For any expression composed through the AWS SDK's expression builder —
which wraps clauses in parentheses by default — this produced garbage
attribute and value references and returned the wrong result:

  (#s <> :c) AND (#s <> :f)  →  silently fails
  (#s = :c)                  →  silently succeeds on mismatch

Any consumer using `NotEqual().And(...)` or similar compound shapes
through aws-sdk-go / aws-sdk-rust was affected.

Rewrite evaluate_condition to delegate to a recursive helper that
mirrors evaluate_filter_expression: split on OR (lowest precedence),
split on AND, strip outer parentheses, recurse, and evaluate a single
leaf comparison at the bottom. Leaf semantics are unchanged — the
existing attribute_exists / attribute_not_exists / = / <> logic is
extracted into evaluate_single_condition with no behavioral difference.

Six new unit tests cover the condition shapes that were broken:
parenthesised <>, parenthesised = mismatch, compound (<>) AND (<>),
compound (=) AND (=) mismatch, compound (=) OR (=), plus a baseline
bare <> test as a regression guard.
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.

2 issues found across 1 file

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/fakecloud-dynamodb/src/service.rs">

<violation number="1" location="crates/fakecloud-dynamodb/src/service.rs:3922">
P2: ConditionExpression parsing can panic on non‑ASCII input because split_on_or/and slice the UTF‑8 string at byte indices; the new call introduces this risk into ConditionExpression handling.</violation>

<violation number="2" location="crates/fakecloud-dynamodb/src/service.rs:3930">
P2: ConditionExpression logical parsing is whitespace-fragile: AND/OR are only recognized as literal " AND "/" OR ", causing compound expressions without that exact spacing to be mis-evaluated.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

let trimmed = expr.trim();

// Split on OR first (lower precedence), respecting parentheses.
let or_parts = split_on_or(trimmed);
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 11, 2026

Choose a reason for hiding this comment

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

P2: ConditionExpression parsing can panic on non‑ASCII input because split_on_or/and slice the UTF‑8 string at byte indices; the new call introduces this risk into ConditionExpression handling.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-dynamodb/src/service.rs, line 3922:

<comment>ConditionExpression parsing can panic on non‑ASCII input because split_on_or/and slice the UTF‑8 string at byte indices; the new call introduces this risk into ConditionExpression handling.</comment>

<file context>
@@ -3895,57 +3895,88 @@ fn evaluate_condition(
+    let trimmed = expr.trim();
+
+    // Split on OR first (lower precedence), respecting parentheses.
+    let or_parts = split_on_or(trimmed);
+    if or_parts.len() > 1 {
+        return or_parts.iter().any(|part| {
</file context>
Fix with Cubic


if let Some(inner) = extract_function_arg(cond, "attribute_not_exists") {
// Then split on AND (higher precedence), respecting parentheses.
let and_parts = split_on_and(trimmed);
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 11, 2026

Choose a reason for hiding this comment

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

P2: ConditionExpression logical parsing is whitespace-fragile: AND/OR are only recognized as literal " AND "/" OR ", causing compound expressions without that exact spacing to be mis-evaluated.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fakecloud-dynamodb/src/service.rs, line 3930:

<comment>ConditionExpression logical parsing is whitespace-fragile: AND/OR are only recognized as literal " AND "/" OR ", causing compound expressions without that exact spacing to be mis-evaluated.</comment>

<file context>
@@ -3895,57 +3895,88 @@ fn evaluate_condition(
 
-    if let Some(inner) = extract_function_arg(cond, "attribute_not_exists") {
+    // Then split on AND (higher precedence), respecting parentheses.
+    let and_parts = split_on_and(trimmed);
+    if and_parts.len() > 1 {
+        return and_parts.iter().all(|part| {
</file context>
Fix with Cubic

@vieiralucas
Copy link
Copy Markdown
Member

Thank you @Bowbaq

@vieiralucas vieiralucas merged commit 5f626f5 into faiscadev:main Apr 11, 2026
1 check passed
vieiralucas added a commit that referenced this pull request Apr 11, 2026
…aluation

- Delete evaluate_condition_expression and evaluate_single_condition, which
  duplicated evaluate_filter_expression and evaluate_single_filter_condition
  with divergent feature sets (the root cause of the bug PR #257 fixed)
- evaluate_condition now delegates to evaluate_filter_expression with an empty
  HashMap for missing items, which correctly models "item doesn't exist"
- Conditions automatically gain begins_with, contains, BETWEEN, and all
  comparison operators that were previously only available in FilterExpression
- Add NOT operator support to evaluate_filter_expression (DynamoDB grammar)
- Remove dead parse_simple_comparison function (subsumed by evaluate_single_key_condition)
- Add test helpers (cond_item, cond_names, cond_values) to reduce boilerplate
- Add tests for NOT, begins_with, contains in conditions, and missing-item edge cases
vieiralucas added a commit that referenced this pull request Apr 11, 2026
…aluation

- Delete evaluate_condition_expression and evaluate_single_condition, which
  duplicated evaluate_filter_expression and evaluate_single_filter_condition
  with divergent feature sets (the root cause of the bug PR #257 fixed)
- evaluate_condition now delegates to evaluate_filter_expression with an empty
  HashMap for missing items, which correctly models "item doesn't exist"
- Conditions automatically gain begins_with, contains, BETWEEN, and all
  comparison operators that were previously only available in FilterExpression
- Add NOT operator support to evaluate_filter_expression (DynamoDB grammar)
- Remove dead parse_simple_comparison function (subsumed by evaluate_single_key_condition)
- Add test helpers (cond_item, cond_names, cond_values) to reduce boilerplate
- Add tests for NOT, begins_with, contains in conditions, and missing-item edge cases
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