Fix like escape for sqlite #3104

nickvergessen · 2018-04-10T22:38:17Z

Follow up to #2720 from @mondrake approved by @deeky666 ("I simply trust you guys on this one" 😿)

Not sure if that is also the case for other DBs, but at least for SQLite ESCAPE "…" must only contain one char. So that is ESCAPE "\\" in php. The unit test accidently used ESCAPE "\\\\" and therefor didn't match the actual syntax and broke for SQLite.

In case ESCAPE "\\\\" is needed for another DB, we should just add both unit tests.

So it must be a single backslash, 2 to escape it as a php string. Double escape is not necessary. Signed-off-by: Joas Schilling <[email protected]>

Signed-off-by: Joas Schilling <[email protected]>

MorrisJobke · 2018-04-10T22:48:33Z

Maybe @Ocramius can have a look at this as well 🤔

morozov · 2018-04-11T03:29:20Z

tests/Doctrine/Tests/DBAL/SQLParserUtilsTest.php

+            array('SELECT data.age AS age, data.id AS id, data.name AS name, data.id AS id FROM test_data data WHERE (data.description LIKE :condition_0 ESCAPE "\\") AND (data.description LIKE :condition_1 ESCAPE "\\") ORDER BY id ASC', false, array(121 => 'condition_0', 173 => 'condition_1')),
+            array('SELECT data.age AS age, data.id AS id, data.name AS name, data.id AS id FROM test_data data WHERE (data.description LIKE :condition_0 ESCAPE "\\") AND (data.description LIKE :condition_1 ESCAPE \'\\\') ORDER BY id ASC', false, array(121 => 'condition_0', 173 => 'condition_1')),
+            array('SELECT data.age AS age, data.id AS id, data.name AS name, data.id AS id FROM test_data data WHERE (data.description LIKE :condition_0 ESCAPE `\\`) AND (data.description LIKE :condition_1 ESCAPE `\\`) ORDER BY id ASC', false, array(121 => 'condition_0', 173 => 'condition_1')),
+            array('SELECT data.age AS age, data.id AS id, data.name AS name, data.id AS id FROM test_data data WHERE (data.description LIKE :condition_0 ESCAPE \'\\\') AND (data.description LIKE :condition_1 ESCAPE `\\`) ORDER BY id ASC', false, array(121 => 'condition_0', 173 => 'condition_1')),


Not sure if that is also the case for other DBs […]

Is it possible to convert this (at least the relevant parts) to a functional test to make sure it does work on a real DB. Otherwise, we may end up changing tests after implementation w/o any evidence that the query is actually valid.

Can you tell me a file where i should put them?

Please look at https://github.com/doctrine/dbal/tree/master/tests/Doctrine/Tests/DBAL/Functional.

Added to tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

nickvergessen · 2018-04-11T14:32:42Z

tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

+    public function ticketGitHub3104Provider()
+    {
+        $escapes = [
+            "ESCAPE '\\'",


So SQLite needs \\ and all others need \\\\…
How to best adjust the test for this?

The question is, how to write a portable query which will work on SQLite and others using the DBAL? If there's no proper solution now, then there's nothing to be fixed or tested: no matter how it's implemented, it's not portable.

Okay found a way, please have a look again.

And well, the databases require a different query, that is exactly what this PR is fixing. SQLite is broken at the moment, all others work. With this PR SQLite and all others work.

@nickvergessen now Postgres is broken. Please take a look at the build.

morozov · 2018-04-11T17:20:10Z

tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

+            ['param_0' => 'bar', 'param_1' => 'foo', 'find' => '%a%'],
+            ['param_0' => 2, 'param_1' => 2, 'find' => 2]
+        );
+        $result = $stmt->fetchAll(FetchMode::ASSOCIATIVE);


If FetchMode::COLUMN is used instead, you could avoid using the query_matched column alias and array_change_key_case() conversion.

morozov · 2018-04-11T17:21:26Z

tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

+        self::assertEquals($expected, $result);
+    }
+
+    public function ticketGitHub3104Provider()


Please avoid test names attached to ticket numbers. Please describe them in terms of features/requirements instead.

morozov · 2018-04-11T17:23:42Z

tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

+        $escapes = [
+            "ESCAPE '\\'",
+            'ESCAPE "\\"',
+            'ESCAPE \'\\\'',


This line is identical to the one 2 lines above. By testing two different "spellings", we only test the PHP engine which is not the purpose of this test.

Signed-off-by: Joas Schilling <[email protected]>

morozov · 2018-04-14T18:14:33Z

tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

+     * @param string $query
+     * @param bool $ignorePostgres
+     */
+    public function testNamedParametersAfterEscapeFirstMatches($query, $ignorePostgres)


The functional test should only only cover the literals quoted according to the platform rules: AbstractPlatform::quoteStringLiteral(). The fact that the parser supports other quote characters than the current platform supports will be covered in the unit test for the parser.

morozov · 2018-04-14T18:18:00Z

tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

+    public function namedParametersAfterEscapeProvider()
+    {
+        return [
+            ["SELECT 1 FROM ddc1372_foobar WHERE (:param_0 LIKE :find ESCAPE '{{escape}}') OR (:param_1 LIKE :find ESCAPE '{{escape}}') LIMIT 1", false],


Would it make sense to remove the duplicating code from the data set (SELECT 1 FROM...) and focus on the fragments which make the difference? We may end up having just one data set if AbstractPlatform::quoteStringLiteral() is used instead of the combinations.

morozov · 2018-04-14T18:20:25Z

tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

+            $this->markTestSkipped('PostgreSql uses double quotes for column names, therefor it is not a valid query');
+        }
+
+        $escapeChar = $this->_conn->getDatabasePlatform() instanceof SqlitePlatform || $this->_conn->getDatabasePlatform() instanceof PostgreSqlPlatform ? '\\' : '\\\\';


instanceof is a bad sign here. The code should be portable by means of using the DBAL APIs, not by introspecting its implementation details.

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen · 2018-04-14T18:38:26Z

tests/Doctrine/Tests/DBAL/Functional/NamedParametersTest.php

+     */
+    public function testNamedParametersAfterEscape(array $params)
+    {
+        $escapeChar = $this->_conn->getDatabasePlatform()->quoteStringLiteral('\\');


@morozov the problem is, PostgreSqlPlatform escapes \, but the string in ESCAPE '…' must not be escaped for PostgreSql

@nickvergessen please see if you can use the API introduced in #3013.

nickvergessen added 2 commits April 11, 2018 00:31

On SQLite the given escape char is not escaped

1be3946

So it must be a single backslash, 2 to escape it as a php string. Double escape is not necessary. Signed-off-by: Joas Schilling <[email protected]>

Fix the regex to not break on "ESCAPE '\\'" strings

8b704a4

Signed-off-by: Joas Schilling <[email protected]>

morozov reviewed Apr 11, 2018

View reviewed changes

nickvergessen commented Apr 11, 2018

View reviewed changes

morozov reviewed Apr 11, 2018

View reviewed changes

Add functional test for named placeholders with ESCAPE

d9e60bd

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen force-pushed the fix-like-escape-for-sqlite branch from c285d3a to d9e60bd Compare April 12, 2018 07:52

marcelklehr mentioned this pull request Apr 12, 2018

SQL LIKE comparisons in SQLite: Escaping doesn't work nextcloud/server#8090

Closed

Adjust PostgreSql as well

47c1864

Signed-off-by: Joas Schilling <[email protected]>

morozov reviewed Apr 14, 2018

View reviewed changes

Reduce integration test

e016b51

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen force-pushed the fix-like-escape-for-sqlite branch from 33f9555 to e016b51 Compare April 14, 2018 18:25

nickvergessen commented Apr 14, 2018

View reviewed changes

nickvergessen mentioned this pull request Dec 6, 2018

Fix named parameter detection on SQLite nextcloud/3rdparty#187

Merged

nickvergessen mentioned this pull request Jan 30, 2019

Bump doctrine/dbal to 2.7.2 nextcloud/3rdparty#224

Merged

Base automatically changed from master to 4.0.x January 22, 2021 07:44

morozov closed this Jul 26, 2021

nickvergessen deleted the fix-like-escape-for-sqlite branch July 26, 2021 13:06

CarlSchwan mentioned this pull request Nov 15, 2021

[stable22]: Add composer patch nextcloud/3rdparty#890

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022

Uh oh!

Fix like escape for sqlite #3104

Fix like escape for sqlite #3104

Uh oh!

Conversation

nickvergessen commented Apr 10, 2018

Uh oh!

MorrisJobke commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!