Database: SqlBuilder::addWhere() consider NOT [Closes #667]

[hrach]

My solution of fixing #667 and #869

[hrach]

Well, I just realized maybe not more efficent, but more understandable solution :D

[tomaswindsor]

Does it work even for more complex expressions than id IN ? How reliable is the detection of beginning of expr in expr IN () ?

[tomaswindsor]

typo in NULL

[hrach]

Rebased with brand new implementation :)

• no double brackets
• maybe even more reliable
• and hopefully, much more easier to understand

[milo]

Nice :) I must test it with fresh head.

[tomaswindsor]

Good work, but I've found some issues when whole condition is surrounded by brackets when using OR:

$selection->where('(id NOT ? OR id = 3)', array());

... leads to:

WHERE ((`id` IS NULL OR `TRUEid` = 3))

and:

$selection->where('(id = 3 OR id NOT ?)', array());

... leads to:

WHERE ((`id` = 3 OR `id` IS NULL OR TRUE)

[hrach]

Thanks for testing :) rebased & fixed. See the change - only one line moved outside condition :D

[tomaswindsor]

One more:

$selection->where('id = 3 OR id = 2 AND id NOT ?', array());

leads to:

WHERE ((`id` = 3 OR `id` = 2) AND (`id` IS NULL OR TRUE))

Added brackets change the meaning of the expression...

[hrach]

Fixed :) now it should be finally ready :D

[hrach]

cc @dg it's ready :)

[dg]

@hrach I believe you, but let's wait another 24 hours, ok? ;)

[hrach]

@dg of course :) cc means review it, not merge :)

[tomaswindsor]

Few more issues I am afraid:

$selection->where('NOT id NOT ?', array());

leads to:

WHERE (NOT `id` IS NULL OR TRUE)
-- should be:
WHERE (NOT (`id` IS NULL OR TRUE))
-- or:
WHERE (`id` IS NULL AND FALSE)

---

$selection->where('NOT (id ? OR title ?)', array(), 'foo');

leads to:

WHERE (((`id` IS NULL OR TRUE) OR (`title` = 'foo')))
-- should be:
WHERE (NOT ((`id` IS NULL AND FALSE) OR (`title` = 'foo')))

---

$selection->where('NOT (id NOT ? OR title ?)', array(), 'foo');

leads to:

WHERE ((NOT (`id` IS NULL OR TRUE) OR (`title` = 'foo')))
-- should be:
WHERE (NOT ((`id` IS NULL OR TRUE) OR (`title` = 'foo')))

[hrach]

@tomaswindsor 1st example is bullshit, I'm not going to solve it. Other examples are fixed.
This would work

$selection->where('NOT (id NOT ?)', array());

But I'm not going to support sth like

SELECT * FROM `users` WHERE NOT NOT NOT NOT `id` = 3;

which is also valid.

rebased & fixed.

[tomaswindsor]

->where('(id))', array()); // works
->where('(id) ?)', array()); // works

->where('(id) NOT', array()); // triggers exception
->where('(id) NOT ?', array()); // triggers exception

->where('NOT (id)', array()); // doesn't work properly
->where('NOT (id) ?', array()); // doesn't work properly

->where('(id NOT ? OR id ?)', array(), 3); // works with 2 extra brackets
->where('(id = ? OR id NOT ?)', 3, array()); // works with 2 extra brackets

->where('(id NOT ? AND id ?)', array(), 3); // works with 2 extra brackets
->where('(id = ? AND id NOT ?)', 3, array()); // works with 2 extra brackets

->where('NOT (title ? AND id NOT ? AND id > ?)', 'foo', array(), 3); // works with 1 extra bracket
->where('NOT (id NOT ? OR id ?)', array(), 3); // works with 1 extra bracket

->where('NOT id NOT ?', array()); // bullshit or not, doesn't work properly
->where('NOT (id) NOT ?)', array()); // bullshit or not, triggers exception

I haven't tested the NOT NOT NOT case, but I think those shouldn't be that hard to resolve (replace NOT NOT with empty string comes to mind or at least NOT NOT NOT with NOT).
Extra brackets can be filtered out separately afterwards.

[hrach]

@tomaswindsor As said, none of these will be fixed. Do something useful.

[tomaswindsor]

@hrach I am only trying to help. I understand some of the currently problematic conditions don't appear as particulary useful in real use, but if we want to be sure it will work in all useful cases, it's best to make sure it works in all valid ones. I appreciate the work and time you have put into this so don't interpret my reports as something trying to belittle your work.

An example of the problematic query in more realistic use:

->where("SUBSTRING_INDEX(url, '.', -2) NOT ?", array());

... incorrectly leads to:

WHERE (SUBSTRING_INDEX((`url`, '.', -2)IS NULL OR TRUE ?)

... while:

->where("SUBSTRING_INDEX(url, '.', -2) NOT ?", array('mysql.com'));

... correctly leads to:

WHERE (SUBSTRING_INDEX(`url`, '.', -2) NOT IN ('mysql.com'))

Same problem appears even if the NOT is used before the expression rather than after.
This also means that for MySql this is problematic:

$blockedDomains = $db->table('domains')->select('url')->where('blocked', TRUE);
$anotherSelection->where("SUBSTRING_INDEX(url, '.', -2) NOT ?", $blockedDomains);

And I know this could be resolved in other ways, where this problem wouldn't occur, but that's not the point.

[tomaswindsor]

I think I've found a better solution. It involves using a special function to determine beginning of the expression just to the left of the IN operator (which is the only problematic part IMHO). You may check the implementation of that function here: https://gist.github.com/tomaswindsor/05ab413c5e2fe995b14b

It should allow support of all the problematic queries mentioned above, and also strings with arbitrary content as part of the expression.

What do you think?

[hrach]

Your comment with substring must be definitelly fixed. The way how is the question. I'd like to avoid things such as parsing sql, you posted in the last query. Much more propper solution seems to me just note in the documentation,
you have to make parentheses yourself. We can easily add the check that they are presentent. (just check closing bracket after placeholder`) (this included BC break or parsing NOT..., so, obviously without checking that)

In the meantime I'm thinking this should be backed out.

[tomaswindsor]

I think some sort of parser (like the one I referenced) is necessary to avoid most possible conflicts. And the one I referenced seems to work good enough IMHO (prove me wrong and come with condition it fails to parse correctly if you can). There may be needed multiple versions of this function for different database engines (different escaping of strings, possible differences in operators, etc.), and perhaps one fallback version with basic support.

The usage in addWhere method would consist merely of a replacement (based on immediate presence of NOT before ?) and insertion of 2 brackets (here you will need to call that function to figure out where to insert the first bracket), nothing more.

Only other issue could be speed of parsing, but even if that turns out to be too slow (which is unlikely IMHO), we can always cache the results of that function for given strings.