Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queries with empty arrays are broken #7

Closed
mirnovov opened this issue Oct 23, 2023 · 9 comments
Closed

Queries with empty arrays are broken #7

mirnovov opened this issue Oct 23, 2023 · 9 comments

Comments

@mirnovov
Copy link

mirnovov commented Oct 23, 2023

The array interpolation feature usually works great, but when given an empty array, it fails.

const a = new SQLiteTag(':memory:', { readonly: true, persistent: true });
const b = await a.all`SELECT * FROM (VALUES (1), (2)) WHERE column1 NOT IN (${[-1]})`;
console.log(b);

correctly returns [ { column1: 1 }, { column1: 2 } ] but

const a = new SQLiteTag(':memory:', { readonly: true, persistent: true });
const b = await a.all`SELECT * FROM (VALUES (1), (2)) WHERE column1 NOT IN (${[]})`;
console.log(b);

either produces an error or returns nothing.

This is because your dependency static-params doesn't return a value for empty arrays when converting JS format strings to SQL parameters. Your code in utils.js assumes that the amount of statement fragments is always v+1, where v is the amount of substituted values. But since the SQL statement is still split at an empty array but no value is provided, this assumption is incorrect.

A possible solution would be to specifically account for empty arrays by returning a value of '', as I have done in my project that bundles sqlite-tag-spawned. I'm unsure if this would break other things that rely on static-params though.

@WebReflection
Copy link
Owner

WebReflection commented Oct 23, 2023

I think you are not fully understanding how template literals work (apologies this was indeed a meant feature) ... you never pass an array, you just pass interpolations:

const b = await a.all`SELECT * FROM (VALUES (1), (2)) WHERE column1 NOT IN (${-1})`;

accordingly, your second query is invalid SQL syntax because you are not passing a value.

@WebReflection
Copy link
Owner

WebReflection commented Oct 23, 2023

to clarify, with template literal tags the template is always v+1 length as defined by specs ... please read again the README and the examples, in no circumstances you pass a complex object as interpolation.

@WebReflection
Copy link
Owner

actually ... never mind, I indeed added the ability to spread lists ... but still, your SQL looks invalid, apologies for the confusion ... how would you write that statement in raw SQL?

@mirnovov
Copy link
Author

mirnovov commented Oct 23, 2023

SELECT * FROM (VALUES (1), (2)) WHERE column1 NOT IN (-1)
SELECT * FROM (VALUES (1), (2)) WHERE column1 NOT IN ()

I have tested all of these SQL statements by directly typing the expected values into SQLite and it works fine; it is only when using your library that empty arrays fail.

@mirnovov
Copy link
Author

mirnovov commented Oct 23, 2023

https://www.sqlite.org/lang_expr.html#booleanexpr

Note that SQLite allows the parenthesized list of scalar values on the right-hand side of an IN or NOT IN operator to be an empty list but most other SQL database engines and the SQL92 standard require the list to contain at least one element.

Given that is an SQLite-specific library I did expect empty arrays to work, and it'd be nice to have this feature. But given the static-params dependency is for SQL in general (where such statements are usually not allowed) I understand if you don't want to add it, it's only a one-line code fix on my end after all.

@WebReflection
Copy link
Owner

I think I want that to work in here too ... why not filing a PR? 😄

@mirnovov
Copy link
Author

I've opened one in your static-params repo since that's where the query parameters are parsed; it'd be hard to include a fix in this repo given how you've designed things.

@WebReflection
Copy link
Owner

sure and ... thanks! I'll do the update dance right after that gets merged

@mirnovov
Copy link
Author

Thanks!

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

No branches or pull requests

2 participants