Fix array decoding to handle quoted elements

[michaelst]

Problem

The logical replication decoder in LiveSync.Replication parses Postgres array columns naïvely:

defp load_value({:array, type}, value) do
  value
  |> String.replace_leading("{", "")
  |> String.replace_trailing("}", "")
  |> String.split(",")
  |> Enum.map(&load_value(type, &1))
end

Postgres delivers array columns in its text output format, where any element containing a comma, double quote, brace, backslash or whitespace (or an empty string) is double-quoted, with " and \ backslash-escaped inside, and an unquoted NULL represents nil:

{"a, b","has \"q\"","back\\slash",NULL,plain}

Splitting on , breaks quoted elements apart and leaves the array-literal quote characters attached. A single text[] element such as DELETE FROM t RETURNING id, name, tags decoded into three fragments ("DELETE … RETURNING id, name, tags"), and an SQL NULL element became the literal string "NULL".

How it showed up

In a downstream app, a query's statements (text[]) column is watched via LiveSync. After the row was updated (an approved query auto-running and setting executed_at), the replicated statements arrived fragmented at the commas of a RETURNING id, name, tags clause and wrapped in stray quotes — so the SQL preview rendered mangled until a refresh reloaded the value through Ecto's correct array decoder.

Fix

Walk the array literal instead of splitting, respecting quoting, \"/\\ escaping, and unquoted NULL → nil. The decoder is a single recursive parse_array/2 plus two small helpers (parse_quoted/2, parse_unquoted/2); elements are still returned as raw strings/nil and cast per-element as before.

Test

Added a replication test that inserts and updates an array column with elements exercising every quoting form — embedded commas, double quotes, backslashes, braces, an empty string, a quoted "NULL" string, a real NULL, and a plain unquoted value — and asserts the decoded struct round-trips exactly. It fails on the old splitter and passes with the fix.

CI / toolchain

• Switched CI from erlef/setup-beam to jdx/mise-action so it uses the same .tool-versions/mise toolchain as local dev.
• Bumped to Elixir 1.19 / OTP 28. The pinned OTP 27.1.3 could no longer complete the TLS handshake with builds.hex.pm (key_usage_mismatch), which failed mix local.rebar and broke every CI job.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix Postgres text[] decoding for quoted/escaped array elements

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Replace naïve comma-splitting with a Postgres-compatible array text parser.
• Correctly handle quoted elements, escapes, unquoted NULL, and nested arrays.
• Add a replication test covering commas/quotes/braces/backslashes/empty strings/NULL.

Diagram

graph TD
  PG[(Postgres)] --> REP["LiveSync.Replication"] --> PARSER["parse_array/1"] --> CAST["load_value/2"] --> APP["Subscriber"]
  TEST[/"ReplicationTest"/] --> REP

  subgraph Legend
    direction LR
    _db[(Database)] ~~~ _mod["Module"] ~~~ _test[/Test/]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use Postgrex/Ecto array decoder for text format

• ➕ Leverages battle-tested parsing (quoting/escaping/NULL handling)
• ➕ Reduces maintenance burden and edge-case risk
• ➖ May require pulling in internal Postgrex parsing APIs or additional dependencies
• ➖ Harder to integrate if replication pipeline is intentionally minimal/text-only

2. Switch replication tuple decoding to binary format

• ➕ Avoids Postgres text array grammar entirely
• ➕ Potentially faster/less allocation for large payloads
• ➖ Larger protocol change; requires implementing binary decoding for more types
• ➖ Higher rollout risk vs a targeted fix

3. Delegate array parsing to Postgres via SQL (e.g., unnest/json)

• ➕ Moves parsing complexity to the database
• ➕ Can normalize outputs (e.g., JSON) consistently
• ➖ Not applicable to replication payloads without additional queries
• ➖ Adds latency and coupling to DB reads during replication

Recommendation: For the current design (text decoding in replication), a small dedicated parser is the most pragmatic fix and is well-contained behind load_value({:array, _}, ...). Longer term, consider moving the replication protocol to binary decoding (not required for this bug fix) to eliminate text-format parsing for arrays and similar types.

Files changed (4) +112 / -4

Bug fix (1) +78 / -3

replication.exParse Postgres array literals with quoting/escaping support +78/-3

Parse Postgres array literals with quoting/escaping support

• Replaces comma-splitting array decoding with a literal walker that respects Postgres text array rules. Correctly handles quoted elements, backslash escapes, unquoted NULL => nil, and preserves nested '{...}' substrings for recursive array decoding.

lib/live_sync/replication.ex

Tests (3) +34 / -1

replication_test.exsAdd replication coverage for quoted/escaped text[] elements +31/-0

Add replication coverage for quoted/escaped text[] elements

• Adds an end-to-end replication test that inserts/updates a 'text[]' column containing commas, quotes, backslashes, braces, empty string, NULL, and plain values. Asserts the received replication events match the original list exactly.

lib/live_sync/replication_test.exs

test_helper.exsExtend examples test table with tags text[] column +1/-0

Extend examples test table with tags text[] column

• Updates the test schema setup to include a 'tags text[]' column on the 'examples' table to support array decoding tests.

lib/test_helper.exs

example.exAdd tags array field to Example schema and changeset +2/-1

Add tags array field to Example schema and changeset

• Adds 'field :tags, {:array, :string}' to the Example schema and includes ':tags' in the changeset cast list so tests can persist and replicate the array column.

test/support/example.ex

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Quadratic binary concatenation ✓ Resolved 🐞 Bug ➹ Performance

Description

The parser builds element strings by repeatedly appending to binaries one UTF-8 codepoint at a time
(e.g., <<acc::binary, char::utf8>>), which can lead to O(n²) copying/allocation for long elements.
This runs in the replication commit path and can become a throughput/latency bottleneck for large
text array elements.

Code

lib/live_sync/replication.ex[R333-341]

+  defp parse_quoted_element(<<?\\, char::utf8, rest::binary>>, acc) do
+    parse_quoted_element(rest, <<acc::binary, char::utf8>>)
+  end
+
+  defp parse_quoted_element(<<?", rest::binary>>, acc), do: {acc, rest}
+
+  defp parse_quoted_element(<<char::utf8, rest::binary>>, acc) do
+    parse_quoted_element(rest, <<acc::binary, char::utf8>>)
+  end

Evidence

Multiple parser functions append to an ever-growing binary inside recursive loops, and array
decoding is invoked from the hot commit path for every decoded row/field.

lib/live_sync/replication.ex[205-217]
lib/live_sync/replication.ex[333-341]
lib/live_sync/replication.ex[350-352]
lib/live_sync/replication.ex[357-374]
lib/live_sync/replication.ex[376-384]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The array parsing helpers repeatedly do `<<acc::binary, ...>>` while walking each element. Binary concatenation copies `acc` each time, so long elements (or large arrays) can incur quadratic work and heavy allocations.

### Issue Context
This code runs when `handle_commit/2` decodes array-typed fields via `load_value/2`, so performance impacts can show up under replication load.

### Fix Focus Areas
- lib/live_sync/replication.ex[205-217]
- lib/live_sync/replication.ex[333-341]
- lib/live_sync/replication.ex[350-352]
- lib/live_sync/replication.ex[357-374]
- lib/live_sync/replication.ex[376-384]

### Suggested fix
- Change accumulators from binaries to iodata lists (e.g., `acc :: [iodata]`) and prepend chunks (`[char | acc]`) during recursion.
- At the end of each element parse, materialize once with `IO.iodata_to_binary(Enum.reverse(acc))`.
- Apply the same pattern in `parse_quoted_element/2`, `parse_unquoted_element/2`, `parse_nested_element/3`, and `skip_quoted_section/2`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Array parser no fallback 🐞 Bug ☼ Reliability

Description

parse_array/1 only matches values starting with "{", so any malformed/alternate array literal will
raise FunctionClauseError when load_value/2 decodes an array field during handle_commit, potentially
crashing replication processing for that transaction. The previous splitter implementation did not
crash on such inputs, making this a robustness regression.

Code

lib/live_sync/replication.ex[R316-317]

+  defp parse_array("{" <> rest), do: parse_array_elements(rest, [])
+

Evidence

The commit path decodes each column value using load_value/2, and the new array decoder
unconditionally calls parse_array/1, which now only accepts inputs starting with {; any other
binary will raise due to missing function clause and is not rescued in handle_commit/2.

lib/live_sync/replication.ex[205-217]
lib/live_sync/replication.ex[282-286]
lib/live_sync/replication.ex[316-317]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`parse_array/1` only has a single function head for `"{" <> rest`. If an array-typed column ever arrives in a non-braced/unsupported representation (or a malformed value), the pipeline `load_value({:array, _}, value) |> parse_array()` will raise `FunctionClauseError` during `handle_commit/2` and can take down replication processing for the transaction.

### Issue Context
- `handle_commit/2` maps every field through `load_value/2` without a rescue.
- `load_value({:array, type}, value)` unconditionally calls `parse_array(value)`.
- `parse_array/1` only matches braced literals.

### Fix Focus Areas
- lib/live_sync/replication.ex[205-217]
- lib/live_sync/replication.ex[282-286]
- lib/live_sync/replication.ex[316-317]

### Suggested fix
- Add a catch-all `parse_array/1` clause (or guard/`case`) that either:
 - normalizes/strips any supported prefix before the first `{` and then parses, or
 - returns a controlled error (e.g., `raise ArgumentError, "unsupported array literal: ..."`) with a clear message.
- If you choose to be resilient instead of crashing, log the bad value and return `nil`/`[]` consistently (but ensure downstream callers can handle it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---