Skip to content

fix(dynamodb): accept newline as UpdateExpression clause separator#449

Merged
hectorvent merged 1 commit intofloci-io:mainfrom
hampsterx:fix/dynamodb-update-expression-newline
Apr 15, 2026
Merged

fix(dynamodb): accept newline as UpdateExpression clause separator#449
hectorvent merged 1 commit intofloci-io:mainfrom
hampsterx:fix/dynamodb-update-expression-newline

Conversation

@hampsterx
Copy link
Copy Markdown
Contributor

Summary

Go AWS SDK v2 expression.Builder joins top-level UpdateExpression clauses with \n (e.g. "SET #a = :a\nADD #b :b"). Floci silently dropped clauses on these inputs because the clause-boundary parser only accepted a literal space as a word boundary. Fixes #430.

Three related fixes in DynamoDbService:

  1. indexOfKeyword word-boundary relaxation. Accept any whitespace (space, tab, CR, LF) before a clause keyword via Character.isWhitespace, not just ' '. Before the fix, SET x = :a\nADD y :b lost both clauses: applySetClause greedily consumed ":a\nADD y :b" as the SET value, failed the placeholder lookup, and neither SET nor ADD ran.
  2. indexOfKeyword loops past non-boundary hits. String.indexOf on the first non-boundary occurrence used to return -1 even when a later valid occurrence existed. An attribute named oldSET in "REMOVE oldSET SET newAttr = :v" masked the real SET. Loop until a boundary match is found or the string ends.
  3. applyAddClause / applyDeleteClause / applyRemoveClause clause advancement alignment. Before this PR they advanced on the next comma before checking for a clause keyword, so an intra-clause comma in a following SET (e.g. "ADD a :v SET b = :b, c = :c") consumed the SET keyword. Aligned with applySetClause's existing "earlier of next clause keyword or next comma wins" rule.

Test cases added

All in DynamoDbServiceTest.java:

Case Expression shape What it exercises
1 SET #n = :n\nADD #c :inc Newline between SET and ADD (primary repro)
2 ADD #c :inc\nSET #n = :n Newline between ADD and SET
3 SET #n = :n\tADD #c :inc Tab separator
4 SET #n = :n\r\nADD #c :inc CRLF separator
5 SET #a = :a, #b = :b\nADD #c :inc\nDELETE #d :d Canonical Go SDK three-clause shape
6 REMOVE #t\nSET #n = :n REMOVE then SET across newline
7 DELETE #tag :d\nADD #c :inc DELETE then ADD across newline
8 ADD #c :inc SET #n = :n, extra = :o Intra-SET comma must not swallow SET keyword
9 REMOVE #t SET #n = :n, extra = :o Same guard for REMOVE->SET
10 DELETE #tag :d SET #n = :n, extra = :o Same guard for DELETE->SET
11 REMOVE oldSET SET newAttr = :v indexOfKeyword loops past oldSET suffix
12 SET prefixSET = :v1, other = :v2 Attribute name containing SET not mis-parsed

Test plan

  • ./mvnw test -Dtest=DynamoDbServiceTest (57 tests pass, 12 new)
  • ./mvnw test full suite (2062 tests pass)
  • Codex + Gemini agentic reviews on the diff (findings addressed or deliberately out of scope)

Out of scope

  • The leading-keyword match in applyUpdateExpression (lines 892-904) still hardcodes a trailing literal space, so SET\t#a = :a at the start of an expression is still unsupported. No known SDK emits that shape and Go SDK only emits \n between clauses, not leading.
  • The keyword constants passed to indexOfKeyword ("SET ", "REMOVE ", etc.) keep the trailing literal space. Switching to regex-based whitespace matching is a larger refactor and not needed to fix [BUG] DynamoDB UpdateExpression parser fails when clause types are separated by newline #430.
  • Rewriting the hand-rolled parser with a formal grammar is a separate discussion.

Backwards compatibility

  • Previously rejected inputs (newline/tab/CR-separated clauses) now apply correctly. No behaviour change for inputs the parser already accepted; all 45 pre-existing DynamoDbServiceTest cases still pass unchanged.

Go AWS SDK v2 expression.Builder joins top-level UpdateExpression
clauses with '\n' (e.g. "SET #a = :a\nADD #b :b"). Two bugs in the
clause walker silently dropped clauses on these inputs:

1. indexOfKeyword only accepted a literal space as the left word
   boundary, so a keyword preceded by \n/\t/\r was rejected. This
   meant findNextClauseKeyword returned -1 for the next clause, and
   the current clause helper consumed the next clause's text as its
   own operand. For "SET x = :a\nADD y :b" applySetClause swallowed
   ":a\nADD y :b" as the SET value, failed the placeholder lookup,
   and neither SET nor ADD ran. Relaxed the boundary check to accept
   any whitespace via Character.isWhitespace.

2. indexOfKeyword returned -1 at the first non-boundary occurrence of
   a keyword, even when a valid match existed later. An attribute
   named oldSET in "REMOVE oldSET SET newAttr = :v" masked the real
   SET. Looped past non-boundary hits until a boundary match or end
   of string.

3. applyRemoveClause, applyAddClause, and applyDeleteClause advanced
   on the next comma before checking for a clause keyword, so an
   intra-clause comma in a following SET (e.g. "ADD a :v SET b = :b,
   c = :c") consumed the SET keyword. Aligned all three helpers with
   applySetClause's "earlier of next clause keyword or next comma
   wins" rule.

Closes floci-io#430.
Copy link
Copy Markdown
Collaborator

@hectorvent hectorvent left a comment

Choose a reason for hiding this comment

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

@hampsterx Thanks! This is a great addition to floci.

@hectorvent hectorvent merged commit ecfef11 into floci-io:main Apr 15, 2026
10 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.

[BUG] DynamoDB UpdateExpression parser fails when clause types are separated by newline

2 participants