fix(realtime): parse Postgres array literals instead of guessing - #2586
fix(realtime): parse Postgres array literals instead of guessing#2586Sy-D wants to merge 1 commit into
Conversation
`toArray` tried `JSON.parse` on the literal and fell back to splitting on
commas. Neither is the array literal grammar, and the file said as much:
a `TODO` on the first and a `WARNING` on the second.
Postgres quotes an element whenever it is empty, spells `NULL`, or
contains a delimiter, brace, quote, backslash or whitespace, so one
literal routinely mixes quoted and unquoted elements. `JSON.parse` then
fails on the unquoted ones and the comma split tears the quoted ones
apart mid-element:
{"a,b",c} -> ['"a', 'b"', 'c']
{"hello world",plain} -> ['"hello world"', 'plain']
{a,NULL,b} -> ['a', 'NULL', 'b']
Replace both with a scanner over the documented grammar: quoted elements
honour `\\` and `\"`, an unquoted `NULL` becomes the null element, and a
quoted `"NULL"` stays the four-character string.
Multidimensional arrays are still unsupported — they were before this
change too, and handling them needs a decision about the element type
that is out of scope here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Sequence Diagram(s)sequenceDiagram
participant toArray
participant parseArrayElements
participant ScalarTypeParser
toArray->>parseArrayElements: Parse PostgreSQL array literal
parseArrayElements-->>toArray: Return parsed elements
toArray->>ScalarTypeParser: Convert non-null elements
ScalarTypeParser-->>toArray: Return converted array
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Description
toArrayturned a Postgres array literal into a JS array by tryingJSON.parseon it and, when that threw, splitting on commas. Neither is the array literal grammar, and the file already said so — aTODOabove the first attempt and aWARNINGabove the fallback.Postgres quotes an element whenever it is empty, spells
NULL, or contains a delimiter, brace, quote, backslash or whitespace. A single literal therefore routinely mixes quoted and unquoted elements — and that is exactly where both strategies fail:JSON.parserejects the unquoted ones, then the comma split tears the quoted ones apart mid-element.toArrayonmaster['a,b', 'c']{"a,b",c}['"a', 'b"', 'c']['hello world', 'plain']{"hello world",plain}['"hello world"', 'plain']['x"y', 'z']{"x\"y",z}['"x\"y"', 'z']['a', null, 'b']{a,NULL,b}['a', 'NULL', 'b']I generated the middle column from a real Postgres 16 rather than reasoning about it, so these are the strings the server actually puts on the wire.
This is on the live
postgres_changespath —RealtimeChannel→convertChangeData→convertCell→toArray— so any subscription to a table with atext[]column whose values contain a space or a comma receives corrupted data, silently.NULLelements arrive as the string'NULL'.The existing tests pass because every case they cover happens to be one where one of the two strategies works:
{a,b,c}and{1,2,3,4}are entirely unquoted, and the daterange case is entirely quoted.What changed
parseArrayElementsscans the literal per the documented grammar: quoted elements honour\\and\", an unquotedNULLbecomes the null element, and a quoted"NULL"stays the four-character string.nullelements skipconvertCellrather than being fed totoNumber/toBoolean.Multidimensional arrays remain unsupported.
{{1,2},{3,4}}was already mangled before this change (JSON.parsefails, the split produces['{1', '2}', '{3', '4}']) and still is. Handling them properly needs a decision about what the element type means one level down, which felt out of scope for a fix; happy to follow up if you want it.Testing
New
packages/core/realtime-js/test/array-literal.test.ts— 9 tests covering comma, whitespace, embedded quote, embedded backslash, brace, empty string, both orderings of quoted/unquoted, unquotedNULLand quoted"NULL".8 of the 9 fail on
master. The one that passes there is{"NULL"}, whichJSON.parsehappens to get right.Full
realtime-jssuite:masterBoth fully green; the delta is exactly this file and its 9 tests. No existing test changed status.
nx lint realtime-jsreports 536 errors both here and onmaster— all pre-existing.nx format:checkandnx build realtime-jspass.Type of Change
Checklist
nx format)vitest run, fully green)nx build realtime-js)