Empty array in whereEquals() causes SQL syntax error

[paigeruten]

Hello, thank you for your work on this excellent library! There's just this one annoying case that keeps coming up ever since we started using it. Here's an example of where it might happen:

$user_ids = getUserIdsByAccountId($account_id);
$settings = getSettingsByUserIds($user_ids);

function getSettingsByUserIds(array $user_ids): array {
    return Select::new($conn)
        ->columns('*')
        ->from('settings')
        ->whereEquals(['user_id' => $user_ids])
        ->fetchAll();
}

This causes an SQL syntax error if there happens to be no users in that account (i.e. $user_ids is an empty array). It produces the query SELECT * FROM settings WHERE user_id IN ().

So we have to keep writing lines like if ($user_ids === []) { return []; } everywhere to handle this case.

I'm wondering if it would make sense for Atlas to handle this case by mapping it to a false condition: SELECT * FROM settings WHERE false? Then no rows are returned, which to me is the expected result of an empty array condition. You could also see it as making up for a shortcoming in SQL's syntax.

What do you think? (Sorry if this has already been considered before.)

[froschdesign]

There's just this one annoying case that keeps coming up ever since we started using it.
…
It produces the query SELECT * FROM settings WHERE user_id IN ().

This is the documented behaviour:

Given an array value, the condition will be IN ().

• in version 4: http://atlasphp.io/dymaxion/statement/select.html#1-2-4-1-4-1
• in version 3: http://atlasphp.io/cassini/query/select.html#2-4-3-1-4-1

So it should not be annoying or surprising. 😉

[paigeruten]

I don't think the docs mean to literally use the invalid SQL syntax IN () as an example. I think they're implying that something will go inside the parentheses. But they don't warn you about what happens with an empty array, when nothing goes inside the parentheses. That case surprised me, especially since I didn't know IN () was invalid SQL.

Here's a more clear example of how handling this case can be annoying. Instead of writing:

$settings = Select::new($conn)
    ->columns('*')
    ->from('settings')
    ->whereEquals(['user_id' => $user_ids])
    ->fetchAll();

We have to write:

if ($user_ids === []) {
    $settings = [];
} else {
    $settings = Select::new($conn)
        ->columns('*')
        ->from('settings')
        ->whereEquals(['user_id' => $user_ids])
        ->fetchAll();
}

This is often an edge case that people don't think about, so we forget to handle it sometimes and we get the syntax error when the array happens to be empty. I've just seen so many people run into this over the years that I thought it might be worth considering handling it in the library, for convenience.

[pmjones]

I thought it might be worth considering handling it in the library, for convenience.

/me nods along

I can see where this would be trouble. You suggested ...

handle this case by mapping it to a false condition: SELECT * FROM settings WHERE false

... but we don't necessarily want to scrap the whole query; instead, we would need a valid IN() that just has nothing in it. @paigeruten can you think of something that would work there?

[paigeruten]

I was thinking of mapping just that one condition to false, and leaving the rest of the query alone. Something like this in 1.x: paigeruten@399fd28

diff --git a/src/Clause/Where.php b/src/Clause/Where.php
index d296e07..99bb588 100644
--- a/src/Clause/Where.php
+++ b/src/Clause/Where.php
@@ -69,6 +69,8 @@ public function whereEquals(array $columnsValues)
                 $this->where($val);
             } elseif ($val === null) {
                 $this->where("{$key} IS NULL");
+            } elseif ($val === []) {
+                $this->where("false");
             } elseif (is_array($val)) {
                 $this->where("{$key} IN ", $val);
             } else {

Another option is to use IN (NULL). A slight difference with this is it will evaluate to null instead of false (https://stackoverflow.com/a/37419910), but I don't think that would affect whereEquals() at all.

[pmjones]

I was thinking of mapping just that one condition to false,

Ah, yes, I see now -- thanks for the clarification.

I don't think WHERE col IN (NULL) will work as expected when NULL is a valid value for that column, but WHERE FALSE might be a solution. @paigeruten add a test to your fork, and we can see how well it works.

@froschdesign thoughts?

EDIT: @paigeruten Looks like the FALSE keyword is not supported by SQLite. Instead of WHERE FALSE maybe try WHERE 0 = 1.

[froschdesign]

@pmjones

Looks like the FALSE keyword is not supported by SQLite.

A short test:

$connection = Atlas\Pdo\Connection::new('sqlite::memory:');

$connection->query(
    'CREATE TABLE pages (
        page_id INTEGER PRIMARY KEY AUTOINCREMENT,
        title   VARCHAR(255),
        body    TEXT
    )'
);

$stmt = 'INSERT INTO pages (page_id, title, body) VALUES (?, ?, ?)';
for ($page_id = 1; $page_id <= 10; $page_id ++) {
    $title = "Page title {$page_id}";
    $body = "Page body {$page_id}";
    $connection->perform($stmt, [$page_id, $title, $body]);
}

$rows = $connection->fetchAll('SELECT * FROM pages WHERE FALSE');
var_dump(count($rows)); // int(0)

$rows = $connection->fetchAll('SELECT * FROM pages WHERE TRUE');
var_dump(count($rows)); // int(10)

[paigeruten]

I've added it to testWhereEquals(), and all tests pass: paigeruten/Atlas.Query@1.x...where-equals-empty-array

I realize it doesn't test actually executing the query, but based on @froschdesign's experiment can we assume that FALSE will be supported in all database backends? I can replace with 0 or 0 = 1 if we think it's safer.

[pmjones]

@paigeruten Very good; you can leave it as FALSE. Submit a PR (so you get the proper credit) and I'll deal with merging. Thanks for your work here!

[pmjones]

Fixed by #14