fix: Data API arrays in where clauses #4241
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ok so this one is a bit of a doozy. Bear with me.
tl;dr RDS Data API handling of arrays is broken anywhere which relies on the
SQL
class logic. This PR fixes it.The problem
When using the RDS Data API, Array types are not currently supported in where clauses (and maybe some other places). There's special-case handling for insert and update queries, but only for params passed in the insert
values
and updateset
collections. If you try to use an Array type in a where clause it will fail (and I suspect it will fail anywhere else which relies on theSQL
class's logic).What happens when you include an Array in a where clause, by the time the parameter reaches the Data API Drizzle driver the
SQL
object has already converted the Array value into a string (via the encoder,PgArray
in this case) e.g. a JavaScript array["abc"]
becomes the PostgreSQL string'{"abc"}'
. When the Data API driver creates the Data API query, it iterates all the params and converts them into the Data API's request structure, but it now has lost any context that the string'{"abc"}'
actually represents an Array, so it sends it as astringValue
to the Data API rather than anarrayValue
.Making a query like
pg.select().from(usersTable).where(eq(usersTable.bestTexts, ["abc"]))
would result in generated SQL like:select * from users where best_texts = $1
with$1
being the string literal'{"abc"}'
. This query fails with an error likeoperator does not exist: text[] = text
because PostgreSQL is trying to use the=
operator on an array column with a literal string. You get similar errors using the helper functions likearrayContains
etc.The not solution
At first, my plan was to add proper support for sending the params as proper ArrayValues by including including array in the type hints. This seemed like the obvious approach. The array type was being lost so the Data API driver was sending the value as a string, so obviously the fix is to send the value as an Array. Until I had a working proof of concept built which showed… the Data API doesn’t actually support the ArrayValues structure for sending params, only returning data.
The solution
So what you see in this PR is my second attempt, which works by adding an explicit cast to the escaped parameter in the query. So now using the above example again, the generated SQL now looks like:
select * from users where best_texts = $1::text[]
, with$1
being the same as before (a literal string) but now we’re forcing PostgreSQL to convert the type to the same type as the column we’re matching against.I tried to make this change as low impact to the rest of the drivers as possible, which was a bit difficult as a lot of this code is generic and shared across drivers in the
SQL
class. So the best thing I could think to do was include the encoder (aka PgArray in this case) in the call to theescapeParam
, so only the Data API driver can override that function and add the extra cast if needed.I'm not in love with this solution but it does work, and it keeps the ick mostly isolated to the Data API driver. Feedback absolutely welcome, and if anyone has ideas how better to approach this I'm all ears.
All the current integration tests pass, and I’ve added a couple more (which failed before my patch). I also had to update
sst
because it was very out-of-date and not working.