Repository navigation
Conversation
@supabase/auth-js
@supabase/functions-js
@supabase/postgrest-js
@supabase/realtime-js
@supabase/storage-js
@supabase/supabase-js
commit: |
mandarini
left a comment
There was a problem hiding this comment.
Hi @spokodev, thank you so much for contributing to Supabase!
Good catch, this is a real bug. A couple of changes before merging: the regex still misses a couple of cases Postgres treats specially, an empty string element or the bare word null both get emitted unquoted and read back as SQL NULL instead of the intended value. Rather than expanding the regex, could you just always double quote string elements? That's what PostgREST's own pgBuildArrayLiteral does and it's simpler than what we have now.
Also, likeAllOf, likeAnyOf, ilikeAllOf, and ilikeAnyOf build array literals the same unquoted way, so the same fix should apply there too.
Thanks again for digging into this, looking forward to the update!
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
|
Thanks! Switched |
|
Checking in on this one. The update you asked for went in on 30 July: Anything else you would like changed before this can go in? |
mandarini
left a comment
There was a problem hiding this comment.
Hi @spokodev, sorry for taking so long to answer. Thanks for making the update.
One thing before this can merge: the new tests in array-literal.test.ts only check the constructed URL string, they don't run against a real PostgREST server. Every neighboring test for these same methods in test/filters.test.ts (e.g. contains with array, overlaps with array) is an integration test that hits the local Dockerized PostgREST instance and snapshots the actual response. Could you add a case there too, with a comma-containing array element, so we have proof the server parses the fixed literal correctly, not just that our own code produces the string we expect?
Also, can you please rebase with master just in case?
Thank you so much for sticking with this!
…containedBy/overlaps
Mirror PostgREST's pgBuildArrayLiteral: quote every string element instead of only those with reserved characters. An empty string and the literal word `null` were emitted unquoted and read back as SQL NULL; quoting them preserves the intended value. Apply the same helper to likeAllOf/likeAnyOf/ilikeAllOf/ ilikeAnyOf, which built their array literals the same unquoted way.
…tgREST The array-literal tests only assert the constructed URL. Add an integration case alongside the other contains/containedBy/overlaps tests so the server is shown to read the quoted literal as one element, with a seed row whose element contains a comma. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
497952f to
21f4b72
Compare
|
Done — rebased on master and added the integration case.
(4, ARRAY['Doe, John', 'admin']);The three existing You were right that the URL assertion is only half the chain. I could not bring the Dockerized stack up locally (the daemon on this machine is wedged), so I verified the server half directly against PostgreSQL 14 with the same table and seed. The two literals are read differently:
and on the actual operator: So the unquoted form did not error, it silently matched a different set — which is what makes the integration case worth having. The client side of the same chain is already pinned in I can add the same case for |
|
@mandarini I merged |
The array branches of
.contains(),.containedBy()and.overlaps()build thePostgres array literal with a raw
value.join(','), so an array element thatcontains a comma is split into several elements and the server evaluates a
different filter than the one requested.
For example
.contains('tags', ['Doe, John', 'admin'])emittedtags=cs.{Doe, John,admin}, which Postgres reads as three elements(
Doe,John,admin) instead of the intended two. The correct literal istags=cs.{"Doe, John",admin}. The same defect applied tocontainedByandoverlaps.A Postgres array literal requires elements that contain the delimiter, braces,
quotes, backslash or surrounding whitespace to be double quoted. The
.in()and.notIn()filters already quote reserved characters, and the realtime filterbuilder does full quoting, so these three were the outliers.
The fix adds a small helper that double quotes and escapes only string elements
with reserved characters or surrounding whitespace. Numbers, booleans, plain
strings, and the range and JSON branches are unchanged. Added a test asserting
the emitted request URL.