Skip to content

fix(dynamodb): support BatchExecuteStatement responses - #2466

Merged
vieiralucas merged 5 commits into
faiscadev:mainfrom
dougludlow:fix/dynamodb-batch-execute-statement-clean
Aug 5, 2026
Merged

fix(dynamodb): support BatchExecuteStatement responses#2466
vieiralucas merged 5 commits into
faiscadev:mainfrom
dougludlow:fix/dynamodb-batch-execute-statement-clean

Conversation

@dougludlow

@dougludlow dougludlow commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Problem

Swell uses DynamoDB BatchExecuteStatement for local broker status updates. Under fakecloud, parameterized single-item SELECTs returned an Items array, so the AWS SDK client saw no Item. UPDATE ... RETURNING ALL NEW * also included the RETURNING clause in the WHERE predicate, causing a validation error.

Fix

  • Convert successful batch SELECT responses from a single Items entry to the API response's Item shape.
  • Preserve the existing single-item limitation for scans and secondary-index SELECTs as per-statement validation responses.
  • Strip RETURNING before evaluating the UPDATE predicate, bind positional parameters into the existing update-expression evaluator, and support list_append(if_not_exists(...), ...).

Validation

cargo test -p fakecloud-dynamodb batch_execute_statement_returns_single_items_and_updates


Summary by cubic

Fixes DynamoDB BatchExecuteStatement to match AWS for single-item SELECTs and UPDATEs with RETURNING. Normalizes batch SELECT shape, makes PartiQL parsing UTF-8 and quote aware, and rejects unsupported queries.

  • Bug Fixes
    • Batch SELECT: return per-statement Item only when WHERE is exactly equality on the full primary key (attr = ? for each key) joined by AND; accept "pk"=? and arbitrary = spacing; require whole-identifier matches; skip quoted literals; handle composite keys; reject scans, extra predicates, non-key WHEREs, and secondary-index SELECTs with ValidationError; empty result returns {}.
    • UPDATE: bind positional ? across SET/WHERE via a rebuilt UpdateExpression; support repeated SET and list_append(if_not_exists(...)); RETURNING ALL NEW * returns the updated Item (even without WHERE); keywords inside single- or double-quoted text are treated as data; preserve non-ASCII values and attribute names.
    • Parsing/resolution: UTF-8-safe, quote-aware splitting for RETURNING, commas, and AND across SET/WHERE/operands; handle double-quoted attribute names and string/number literals; reuse arithmetic operand logic for list_append; trim quotes when resolving attribute names/paths.

Written for commit 05ed33e. Summary will update on new commits.

Review in cubic

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 3 files

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/batch.rs">

<violation number="1" location="crates/fakecloud-dynamodb/src/service/batch.rs:1574">
P2: Projected single-item reads return extra attributes: `SELECT pk FROM ... WHERE pk = ?` is converted to `Item` without applying the projection. Applying the PartiQL projection before constructing this response would keep BatchExecuteStatement results aligned with the requested attribute set.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread crates/fakecloud-dynamodb/src/service/helpers/partiql.rs
Comment thread crates/fakecloud-dynamodb/src/service/helpers/partiql.rs Outdated
.cloned()
.unwrap_or_default();
if items.len() == 1 {
return json!({ "Item": items[0] });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Projected single-item reads return extra attributes: SELECT pk FROM ... WHERE pk = ? is converted to Item without applying the projection. Applying the PartiQL projection before constructing this response would keep BatchExecuteStatement results aligned with the requested attribute set.

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/batch.rs, line 1574:

<comment>Projected single-item reads return extra attributes: `SELECT pk FROM ... WHERE pk = ?` is converted to `Item` without applying the projection. Applying the PartiQL projection before constructing this response would keep BatchExecuteStatement results aligned with the requested attribute set.</comment>

<file context>
@@ -1549,6 +1549,46 @@ impl DynamoDbService {
+        .cloned()
+        .unwrap_or_default();
+    if items.len() == 1 {
+        return json!({ "Item": items[0] });
+    }
+    if items.is_empty() {
</file context>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. This is a valid broader PartiQL projection gap, but it predates this BatchExecuteStatement response conversion: execute_partiql_in_state already returns the complete item for SELECT pk .... I am leaving it out of this targeted compatibility fix to avoid changing generic ExecuteStatement semantics without dedicated coverage.

Comment thread crates/fakecloud-dynamodb/src/service/batch.rs
Comment thread crates/fakecloud-dynamodb/src/service/helpers/partiql.rs Outdated
Comment thread crates/fakecloud-dynamodb/src/service/helpers/partiql.rs Outdated
Comment thread crates/fakecloud-dynamodb/src/service/batch.rs Outdated

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

2 issues found across 4 files (changes from recent commits).

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/helpers/paths.rs">

<violation number="1" location="crates/fakecloud-dynamodb/src/service/helpers/paths.rs:6">
P2: `trim_matches('"')` strips every leading and trailing quote character, so a quoted identifier whose actual attribute name ends/starts with a quote (PartiQL-escaped as `"a""`) is corrupted to `a""`, producing a key that no longer matches the stored item; update/projection/condition lookups would silently miss. Since resolve_attr_name is shared and already applied to (space-)trimmed segment names, a unquote that removes only a single surrounding pair (e.g. `name.strip_prefix('"').and_then(|s| s.strip_suffix('"'))`) is safer and more precise than stripping every boundary quote.</violation>
</file>

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

<violation number="1" location="crates/fakecloud-dynamodb/src/service/helpers/mod.rs:760">
P2: Single-quoted string literals are stored without unescaping PartiQL's doubled-quote escape. A SET like `note = 'it''s here'` persists `it''s here` rather than `it's here`, deviating from real PartiQL semantics. Consider replacing each `''` with `'` before emitting the `S` value, and cover a quoted literal containing a quote in the regression test.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread crates/fakecloud-dynamodb/src/service/helpers/partiql.rs Outdated
use super::*;

pub(crate) fn resolve_attr_name(name: &str, expr_attr_names: &HashMap<String, String>) -> String {
let name = name.trim_matches('"');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: trim_matches('"') strips every leading and trailing quote character, so a quoted identifier whose actual attribute name ends/starts with a quote (PartiQL-escaped as "a"") is corrupted to a"", producing a key that no longer matches the stored item; update/projection/condition lookups would silently miss. Since resolve_attr_name is shared and already applied to (space-)trimmed segment names, a unquote that removes only a single surrounding pair (e.g. name.strip_prefix('"').and_then(|s| s.strip_suffix('"'))) is safer and more precise than stripping every boundary quote.

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/helpers/paths.rs, line 6:

<comment>`trim_matches('"')` strips every leading and trailing quote character, so a quoted identifier whose actual attribute name ends/starts with a quote (PartiQL-escaped as `"a""`) is corrupted to `a""`, producing a key that no longer matches the stored item; update/projection/condition lookups would silently miss. Since resolve_attr_name is shared and already applied to (space-)trimmed segment names, a unquote that removes only a single surrounding pair (e.g. `name.strip_prefix('"').and_then(|s| s.strip_suffix('"'))`) is safer and more precise than stripping every boundary quote.</comment>

<file context>
@@ -3,6 +3,7 @@
 use super::*;
 
 pub(crate) fn resolve_attr_name(name: &str, expr_attr_names: &HashMap<String, String>) -> String {
+    let name = name.trim_matches('"');
     if name.starts_with('#') {
         expr_attr_names
</file context>

Comment thread crates/fakecloud-dynamodb/src/service/batch.rs Outdated
resolve_path(reference, item, expr_attr_names)
resolve_path(reference, item, expr_attr_names).or_else(|| {
reference
.strip_prefix('\'')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Single-quoted string literals are stored without unescaping PartiQL's doubled-quote escape. A SET like note = 'it''s here' persists it''s here rather than it's here, deviating from real PartiQL semantics. Consider replacing each '' with ' before emitting the S value, and cover a quoted literal containing a quote in the regression test.

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/helpers/mod.rs, line 760:

<comment>Single-quoted string literals are stored without unescaping PartiQL's doubled-quote escape. A SET like `note = 'it''s here'` persists `it''s here` rather than `it's here`, deviating from real PartiQL semantics. Consider replacing each `''` with `'` before emitting the `S` value, and cover a quoted literal containing a quote in the regression test.</comment>

<file context>
@@ -751,11 +751,22 @@ pub(crate) fn resolve_ref_or_path(
-    resolve_path(reference, item, expr_attr_names)
+    resolve_path(reference, item, expr_attr_names).or_else(|| {
+        reference
+            .strip_prefix('\'')
+            .and_then(|x| x.strip_suffix('\''))
+            .map(|x| json!({ "S": x }))
</file context>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes FakeCloud DynamoDB PartiQL handling to better match AWS for BatchExecuteStatement, specifically normalizing single-item SELECT response shapes and improving UPDATE parsing/binding for RETURNING and complex SET expressions.

Changes:

  • BatchExecuteStatement: transform eligible single-item SELECT responses from {"Items":[...]} to {"Item":{...}} and return per-statement ValidationError for unsupported SELECT shapes.
  • PartiQL UPDATE: strip RETURNING before predicate evaluation, bind positional parameters via update-expression evaluation, and add support for list_append(if_not_exists(...), ...) patterns.
  • Update-expression helpers: improve attribute name/path handling for quoted identifiers and literal operands; add targeted regression tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
crates/fakecloud-dynamodb/src/service/helpers/paths.rs Trims double quotes in attribute-name resolution to support quoted identifiers.
crates/fakecloud-dynamodb/src/service/helpers/partiql.rs Refactors PartiQL UPDATE execution (RETURNING handling, parameter binding via update-expression), adds parsing helpers and tests.
crates/fakecloud-dynamodb/src/service/helpers/mod.rs Extends update-expression operand resolution (quoted refs, if_not_exists in list_append, simple literals).
crates/fakecloud-dynamodb/src/service/batch.rs Adds BatchExecuteStatement response normalization for single-item key SELECTs and regression tests.
Suppressed comments (3)

crates/fakecloud-dynamodb/src/service/helpers/partiql.rs:596

  • split_partiql_returning_clause iterates over byte offsets (0..upper.len()) and then slices strings with upper[index..] / upper[..index]. This will panic on non-ASCII input outside single quotes (e.g. a quoted attribute name containing UTF-8) because many byte offsets are not valid UTF-8 boundaries. Consider iterating over char_indices() and comparing against ASCII bytes instead of slicing at arbitrary byte indices.
        if !in_quote
            && upper[index..].starts_with("RETURNING")
            && index > 0
            && upper[..index].ends_with(char::is_whitespace)
            && upper[index + 9..].starts_with(char::is_whitespace)

crates/fakecloud-dynamodb/src/service/helpers/partiql.rs:649

  • split_repeated_set_clauses walks raw bytes and appends each byte as a char (result.push(byte as char)). This will corrupt any non-ASCII characters (e.g. quoted attribute names with UTF-8) and can also break keyword detection because expression[index..] slices at arbitrary byte offsets. This should operate on UTF-8 char boundaries (e.g. char_indices()), or build the output as bytes and String::from_utf8 it.
    while index < expression.len() {
        let byte = expression.as_bytes()[index];
        match byte {
            b'\'' if !in_double_quote => in_single_quote = !in_single_quote,
            b'"' if !in_single_quote => in_double_quote = !in_double_quote,

crates/fakecloud-dynamodb/src/service/helpers/partiql.rs:623

  • prepare_partiql_update_expression rewrites every ? that is outside single quotes. This can also rewrite ? characters inside double-quoted attribute names (e.g. SET "a?b" = ?), corrupting the identifier and shifting positional parameter binding. Track double-quoted spans too and only rewrite ? when not inside either quote type.

        if !in_quote && character == '?' {
            expression.push_str(&format!(":p{parameter_index}"));
            parameter_index += 1;
            continue;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +758 to +769
resolve_path(reference, item, expr_attr_names).or_else(|| {
reference
.strip_prefix('\'')
.and_then(|x| x.strip_suffix('\''))
.map(|x| json!({ "S": x }))
.or_else(|| {
reference
.parse::<f64>()
.ok()
.map(|_| json!({ "N": reference }))
})
})
Comment on lines +1574 to +1582
let where_clause = &rest[5..];
if table.is_none_or(|table| {
table.key_schema.iter().any(|key| {
!where_clause.contains(&format!("\"{}\" = ?", key.attribute_name))
&& !where_clause.contains(&format!("{} = ?", key.attribute_name))
})
}) {
return batch_single_item_select_error();
}
Comment on lines 483 to 487
let where_pos = find_outside_quotes(&after_set.to_ascii_uppercase(), "WHERE");
let (set_clause, where_clause) = if let Some(wp) = where_pos {
(&after_set[..wp], after_set[wp + 5..].trim())
} else {
(after_set, "")
@vieiralucas
vieiralucas requested a lite review from Copilot August 4, 2026 23:21

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 3 files (changes from recent commits).

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/helpers/mod.rs">

<violation number="1" location="crates/fakecloud-dynamodb/src/service/helpers/mod.rs:421">
P2: PartiQL updates whose quoted string contains a comma still silently lose the update; the assignment splitter should preserve commas inside single-quoted literals, not only ignore quoted clause keywords.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

let after_pos = abs_pos + kw.len();
let after_ok = after_pos >= expr.len() || is_boundary(expr.as_bytes()[after_pos]);
if before_ok && after_ok {
if before_ok && after_ok && !in_quote[abs_pos] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: PartiQL updates whose quoted string contains a comma still silently lose the update; the assignment splitter should preserve commas inside single-quoted literals, not only ignore quoted clause keywords.

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/helpers/mod.rs, line 421:

<comment>PartiQL updates whose quoted string contains a comma still silently lose the update; the assignment splitter should preserve commas inside single-quoted literals, not only ignore quoted clause keywords.</comment>

<file context>
@@ -397,14 +397,28 @@ pub(crate) fn parse_update_clauses(expr: &str) -> Vec<(UpdateAction, Vec<String>
             let after_pos = abs_pos + kw.len();
             let after_ok = after_pos >= expr.len() || is_boundary(expr.as_bytes()[after_pos]);
-            if before_ok && after_ok {
+            if before_ok && after_ok && !in_quote[abs_pos] {
                 positions.push((abs_pos, action));
             }
</file context>

Comment thread crates/fakecloud-dynamodb/src/service/batch.rs Outdated
Comment thread crates/fakecloud-dynamodb/src/service/batch.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

crates/fakecloud-dynamodb/src/service/helpers/paths.rs:7

  • trim_matches('"') removes all leading/trailing " characters, not just one outer pair. This will mis-handle valid quoted identifiers that contain escaped quotes at the ends (SQL-style "" inside a quoted identifier), because trailing/leading escaped quotes can be stripped away. Prefer stripping at most one leading and one trailing quote (e.g., via strip_prefix('"') + strip_suffix('"')) so only the wrapper quotes are removed.
    let name = name.trim_matches('"');
    if name.starts_with('#') {

crates/fakecloud-dynamodb/src/service/helpers/partiql.rs:606

  • The quote tracking flips in_quote on every single-quote character, but PartiQL/SQL string literals escape quotes by doubling them (''). With such inputs, this scan will incorrectly exit/re-enter quote mode mid-literal, which can cause RETURNING inside a string (or after it) to be mis-detected and split incorrectly. A concrete fix is to treat '' as an escaped quote when inside a literal (consume both quotes without toggling), so quote state remains correct.
fn split_partiql_returning_clause(where_clause: &str) -> (&str, bool) {
    // `to_ascii_uppercase` preserves byte length and never touches non-ASCII
    // bytes, so char boundaries stay aligned between `upper` and `where_clause`.
    // Iterate real char boundaries (not raw byte indices) so a non-ASCII byte
    // in the predicate cannot make a slice land mid-character and panic.
    let upper = where_clause.to_ascii_uppercase();
    let mut in_quote = false;
    for (index, ch) in upper.char_indices() {
        if ch == '\'' {
            in_quote = !in_quote;
        }
        if !in_quote
            && upper[index..].starts_with("RETURNING")
            && index > 0
            && upper[..index].ends_with(char::is_whitespace)
            && upper[index + 9..].starts_with(char::is_whitespace)
        {
            return (where_clause[..index].trim(), true);
        }
    }
    (where_clause, false)
}

crates/fakecloud-dynamodb/src/service/batch.rs:1582

  • This validation only checks that each key attribute appears somewhere as key = ?, but it does not reject additional non-key predicates (e.g., WHERE pk = ? AND non_key = ?). That contradicts the intended “reject non-key WHEREs” behavior described in the PR and can allow unsupported queries through. Consider explicitly parsing the WHERE clause into top-level conjuncts (split on AND outside quotes/parentheses) and requiring the set of predicates to be exactly the key-schema equality checks (and nothing else).
    let where_clause = &rest[5..];
    if table.is_none_or(|table| {
        table
            .key_schema
            .iter()
            .any(|key| !where_matches_key_equals(where_clause, &key.attribute_name))
    }) {
        return batch_single_item_select_error();
    }

Comment on lines +752 to +767
let reference = reference.trim().trim_matches('"');
if reference.starts_with(':') {
return expr_attr_values.get(reference).cloned();
}
resolve_path(reference, item, expr_attr_names)
resolve_path(reference, item, expr_attr_names).or_else(|| {
reference
.strip_prefix('\'')
.and_then(|x| x.strip_suffix('\''))
.map(|x| json!({ "S": x }))
.or_else(|| {
reference
.parse::<f64>()
.ok()
.map(|_| json!({ "N": reference }))
})
})
dougludlow and others added 3 commits August 4, 2026 21:40
Follow-ups on top of the BatchExecuteStatement single-item/RETURNING fix:

- Make the SET/RETURNING scanners UTF-8 safe: split_repeated_set_clauses and
  split_partiql_returning_clause walked raw byte indices and rebuilt the
  expression with 'byte as char', corrupting non-ASCII values/attribute names
  and panicking when a slice landed mid-character. Walk char boundaries and
  copy chars.
- Make parse_update_clauses single-quote-aware so a keyword inside a string
  literal (SET note = 'please REMOVE this') is treated as data, not a spurious
  clause boundary that silently dropped the write.
- Tolerate arbitrary spacing around '=' and require a whole-identifier match in
  the batch single-item key check (where_matches_key_equals), so "pk"=? is
  accepted and mypk = ? is not mistaken for key pk.
- Dedup: resolve the list_append if_not_exists operand through the existing
  evaluate_arithmetic_operand instead of a near-identical copy.
- Fix a clippy while-let-on-iterator warning in prepare_partiql_update_expression.
- Add regression tests: non-ASCII values/attrs, keyword-in-literal, key-check
  spacing/word-boundary.
@vieiralucas
vieiralucas force-pushed the fix/dynamodb-batch-execute-statement-clean branch from c6a3b78 to 1f0c90a Compare August 5, 2026 00:40
@vieiralucas
vieiralucas requested a lite review from Copilot August 5, 2026 00:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

crates/fakecloud-dynamodb/src/service/batch.rs:1582

  • BatchExecuteStatement SELECT single-item detection accepts WHERE clauses that include extra non-key predicates as long as each key appears somewhere (e.g. WHERE pk = ? AND data = ? passes the key check). This contradicts the stated limitation (“only single item select”) and can yield multi-item reads without returning a ValidationError. Consider validating that the WHERE clause is exactly a conjunction of attr = ? predicates (one per key attribute) with no extra terms, and rejecting anything else.
    let where_clause = &rest[5..];
    if table.is_none_or(|table| {
        table
            .key_schema
            .iter()

crates/fakecloud-dynamodb/src/service/helpers/mod.rs:767

  • resolve_ref_or_path now falls back to interpreting unknown tokens as string/number literals. This makes the shared UpdateExpression evaluator accept invalid UpdateItem expressions like SET a = 1 or SET a = 'x' (AWS requires :placeholders), reducing AWS-fidelity and potentially masking user mistakes. It would be safer to keep UpdateExpression strict and support PartiQL literals in the PartiQL path only (e.g. translate literals into generated :pN ExpressionAttributeValues in prepare_partiql_update_expression, or use a separate resolver for PartiQL).
    let reference = reference.trim().trim_matches('"');
    if reference.starts_with(':') {
        return expr_attr_values.get(reference).cloned();
    }
    resolve_path(reference, item, expr_attr_names).or_else(|| {
        reference
            .strip_prefix('\'')
            .and_then(|x| x.strip_suffix('\''))
            .map(|x| json!({ "S": x }))
            .or_else(|| {
                reference
                    .parse::<f64>()
                    .ok()
                    .map(|_| json!({ "N": reference }))
            })
    })

crates/fakecloud-dynamodb/src/service/helpers/partiql.rs:659

  • split_repeated_set_clauses only recognizes repeated SET keywords when they are uppercase (starts_with("SET")). PartiQL keywords are case-insensitive, so mixed/lowercase forms like ... set a = ? SeT b = ? won’t be normalized and can break parameter binding/update evaluation.
            && expression[index..].starts_with("SET")

Address bot review findings on the BatchExecuteStatement follow-ups:

- split_on_top_level_keyword now skips single- and double-quoted spans, so a
  comma inside a string literal (SET addr = 'City, State') or quoted identifier
  no longer tears an assignment apart and silently drops the update. Fixes the
  same class for list_append operands and WHERE/AND splitting.
- parse_update_clauses quote guard now tracks double quotes too, so a keyword
  inside a quoted attribute name is not read as a clause boundary.
- where_matches_key_equals is now quote-aware and UTF-8 safe: it skips
  single-quoted literals (so a value like 'pk = ?' is not mistaken for the key
  predicate) and matches the exact quoted token, preserving internal whitespace
  in a quoted key name; still tolerant of arbitrary spacing around '='.
- Strip a trailing RETURNING from the whole post-SET segment before the
  SET/WHERE split, so a WHERE-less UPDATE ... RETURNING no longer corrupts the
  update expression.
- Regression tests for each.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/fakecloud-dynamodb/src/service/helpers/mod.rs:770

  • resolve_ref_or_path treats any f64-parsable token as a DynamoDB N value but does not reject non-finite values (e.g. NaN, inf, or overflow like 1e999) and will also preserve a leading +, which is not a valid JSON number string. This can create invalid persisted Number attribute values.
                reference
                    .parse::<f64>()
                    .ok()
                    .map(|_| json!({ "N": reference }))
            })

crates/fakecloud-dynamodb/src/service/helpers/partiql.rs:610

  • split_partiql_returning_clause only tracks single-quoted literals. If a double-quoted identifier contains RETURNING (e.g. SET "foo RETURNING bar" = ? RETURNING ALL NEW *), the scan can split inside the identifier and strip part of the SET clause, corrupting the update expression.
    let upper = where_clause.to_ascii_uppercase();
    let mut in_quote = false;
    for (index, ch) in upper.char_indices() {
        if ch == '\'' {
            in_quote = !in_quote;

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 5 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread crates/fakecloud-dynamodb/src/service/batch.rs Outdated
Replace the substring key heuristic with a structural gate (Cubic P2s): split
the WHERE on quote-aware top-level AND and require the conjuncts to be exactly
the primary-key attributes, each as 'attr = ?', and nothing else.

This rejects an extra non-key predicate (WHERE pk = ? AND data = ?), which AWS
BatchExecuteStatement does not allow, while still accepting tight spacing
('pk'=?), newlines around '=', quoted key names with internal whitespace, and
composite keys. A key-looking pattern inside a string literal ('pk = ?') is
data, not a predicate. Adds unit + table-level regression tests.
@vieiralucas

Copy link
Copy Markdown
Member

Latest Cubic run: dispositions.

  • Literal-valued UPDATE assignments becoming no-ops (P1): not reproducible. Literal SET values resolve through resolve_ref_or_path (string 'ready' -> S, numeric 42 -> N); the literal_update/literal_select regression asserts a literal UPDATE round-trips. Left as-is.
  • Attribute name containing RETURNING (P1): already handled. split_partiql_returning_clause requires whitespace word boundaries, covered by returning_clause_requires_keyword_boundaries.
  • Extra non-key predicate returned as an Item / whitespace / 'pk = ?' literal (P2s): fixed in 05ed33e by replacing the substring key check with a structural full-key gate (quote-aware AND split, conjuncts must be exactly the key attributes as attr = ?).
  • PartiQL SELECT projection not applied (P2): pre-existing gap in execute_partiql_in_state (affects non-batch too), out of scope for this response-shape change.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

crates/fakecloud-dynamodb/src/service/helpers/partiql.rs:666

  • Same issue as above: ends_with(char::is_whitespace) / starts_with(char::is_whitespace) are unlikely to compile on stable Rust. Prefer explicit ASCII-whitespace checks on the bytes before/after the matched keyword boundary.
        if !in_single_quote
            && !in_double_quote
            && expression[index..].starts_with("SET")
            && index > 0
            && expression[..index].ends_with(char::is_whitespace)
            && expression[index + 3..].starts_with(char::is_whitespace)

crates/fakecloud-dynamodb/src/service/helpers/mod.rs:756

  • resolve_ref_or_path now supports additional operand forms (double-quoted identifiers, single-quoted string literals, and numeric literals), but the doc comment still says it only resolves placeholders or document paths. Updating the comment will prevent future confusion about what this helper accepts and why (especially since this behavior is PartiQL-specific).
/// Resolve a SET-RHS operand that may be either a value placeholder
/// (``:foo``) or a document path (top-level attribute, ``#name``, or a
/// dotted path like ``profile.email`` / ``#web.#count``).
pub(crate) fn resolve_ref_or_path(
    reference: &str,
    item: &HashMap<String, AttributeValue>,
    expr_attr_names: &HashMap<String, String>,
    expr_attr_values: &HashMap<String, Value>,
) -> Option<Value> {
    let reference = reference.trim().trim_matches('"');

Comment on lines +600 to +605
if !in_quote
&& upper[index..].starts_with("RETURNING")
&& index > 0
&& upper[..index].ends_with(char::is_whitespace)
&& upper[index + 9..].starts_with(char::is_whitespace)
{
Comment on lines +1633 to +1638
if let Some(rest) = c.strip_prefix(attr) {
// The bare name must be a whole identifier, not a prefix of a longer one.
if !rest.starts_with(|ch: char| ch.is_alphanumeric() || ch == '_') {
return is_equals_placeholder(rest);
}
}
@vieiralucas

Copy link
Copy Markdown
Member

Copilot's two latest comments (str::starts_with/ends_with with a char predicate/closure not being supported on stable Rust) are false positives: a FnMut(char) -> bool implements std::str::pattern::Pattern, so both char::is_whitespace and the inline closure are valid stable Rust. The crate builds green under clippy -p fakecloud-dynamodb --all-targets -- -D warnings and all lib tests pass, in CI and locally. No change needed.

@vieiralucas
vieiralucas merged commit e322ebf into faiscadev:main Aug 5, 2026
157 checks passed
@vieiralucas

Copy link
Copy Markdown
Member

Thanks a lot for this, @dougludlow. Getting BatchExecuteStatement to match AWS response shapes (single-item Item vs Items, and UPDATE ... RETURNING) is exactly the kind of fidelity that makes local PartiQL against fakecloud actually usable, and your regression coverage made the follow-ups easy to reason about. I pushed a few hardening follow-ups before merging: made the SET/RETURNING scanners UTF-8 safe (non-ASCII values and attribute names no longer corrupt or panic), made keyword and comma splitting quote-aware so a string literal like 'please REMOVE this' or 'City, State' can't silently drop the write, and replaced the batch single-item key detection with a structural full-key gate (quote-aware AND split, conjuncts must be exactly the key attributes as attr = ?) so an extra non-key predicate is rejected the way AWS does. Merged. Really appreciate the contribution.

@dougludlow

Copy link
Copy Markdown
Contributor Author

Thanks for merging this. We use these fixes for a local AWS/Cognito development stack and are waiting to update from the official image. Is there a plan or rough timing for the next Fakecloud release and ghcr.io/faiscadev/fakecloud image that includes this and #2465?

@dougludlow
dougludlow deleted the fix/dynamodb-batch-execute-statement-clean branch August 6, 2026 16:21
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.

3 participants