Corrected handling of list parameters with sparse types

[mateuszsip]

Meant to fix #2897

[morozov]

@kejwmen thanks for fixing this. I believe this problem also exists for named parameters but reveals in a slightly different way. Consider the following test:

array(
    "SELECT * FROM Foo WHERE :foo IN (:bar)",
    array('foo' => 1, 'bar' => array(1, 2)),
    array('bar' => Connection::PARAM_INT_ARRAY),
    "SELECT * FROM Foo WHERE ? IN (?, ?)",
    array(1, 1, 2),
    array(null, \PDO::PARAM_INT, \PDO::PARAM_INT)
),

On your branch, it produces:

1) Doctrine\Tests\DBAL\SQLParserUtilsTest::testExpandListParameters with data set #10 ('SELECT * FROM Foo WHERE :foo IN (:bar)', array(1, array(1, 2)), array(101), 'SELECT * FROM Foo WHERE ? IN (?, ?)', array(1, 1, 2), array(null, 1, 1))
Types dont match
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => null
+    0 => 2

Basically, despite the first parameter has an integer value and doesn't have the type specified, it will be bound as a PDO::PARAM_STR.

[morozov]

If you decide to fix that as well, please make sure it works in the case when parameters have leading colons but types don't and vice versa like in these cases:

dbal/tests/Doctrine/Tests/DBAL/SQLParserUtilsTest.php

Lines 324 to 339 in 09e072d

	array(
	"SELECT * FROM Foo WHERE foo IN (:foo) OR bar = :bar",
	array(':foo' => array(1, 2), ':bar' => 'bar'),
	array('foo' => Connection::PARAM_INT_ARRAY),
	'SELECT * FROM Foo WHERE foo IN (?, ?) OR bar = ?',
	array(1, 2, 'bar'),
	array(\PDO::PARAM_INT, \PDO::PARAM_INT, \PDO::PARAM_STR)
	),
	array(
	"SELECT * FROM Foo WHERE foo IN (:foo) OR bar = :bar",
	array('foo' => array(1, 2), 'bar' => 'bar'),
	array(':foo' => Connection::PARAM_INT_ARRAY),
	'SELECT * FROM Foo WHERE foo IN (?, ?) OR bar = ?',
	array(1, 2, 'bar'),
	array(\PDO::PARAM_INT, \PDO::PARAM_INT, \PDO::PARAM_STR)
	),

[mateuszsip]

Thanks for test cases, digging!

[mateuszsip]

I started focusing on the problem, so fillMissingNamedTypes can be not optimal performance-wise. If you see some obvious issues I would love to correct it.

Please tak a look at modified tests, do I understand you correctly?

By the way, it would be nice to make expandListParameters readable, but first I need to understand it.

[morozov]

I started focusing on the problem, so fillMissingNamedTypes can be not optimal performance-wise. If you see some obvious issues I would love to correct it.

Given that the existing code is performant enough and handles most of the cases, the fix (ideally) should be applied to the existing algorithm instead of being put aside. Otherwise, it fixes a very rare edge case at the cost of overal performance degradation. For instance, type/param normalization seems not required in any of the covered cases most probably it's already implemented somewhere inside.

I cannot give any concrete advice on the approach since it requires detailed understanding of the algorithm.

Please take a look at modified tests, do I understand you correctly?

Not sure why the tests had to be modified. I think it would be acceptable if instead of a missing type a NULL type was returned (as it was done previously), but all explicitly specified types should remain in place.

By the way, it would be nice to make expandListParameters readable, but first I need to understand it.

I think it's enough to have the algorithm documented somewhere in the ticket. Refactoring (if I read "make readable" correctly) for sake of readability may be too dangerous given how mission critical this piece of code is.

[mateuszsip]

Otherwise, it fixes a very rare edge case at the cost of overal performance degradation. For instance, type/param normalization seems not required in any of the covered cases most probably it's already implemented somewhere inside.

I will take a closer look to check what could be done.

Not sure why the tests had to be modified. I think it would be acceptable if instead of a missing type a NULL type was returned (as it was done previously), but all explicitly specified types should remain in place.

Example:

There were test cases like:

array(
    "SELECT * FROM Foo WHERE foo IN (:foo) OR bar = :bar",
    array('foo' => array(1, 2), 'bar' => 'bar'),
    array(':foo' => Connection::PARAM_INT_ARRAY),
    'SELECT * FROM Foo WHERE foo IN (?, ?) OR bar = ?',
    array(1, 2, 'bar'),
    array(\PDO::PARAM_INT, \PDO::PARAM_INT, \PDO::PARAM_STR)
)

without explicit type declaration, where \PDO::PARAM_STR was expected for not defined type.
So I changed expectations there, because it returns NULL after my changes.

I think it would be acceptable if instead of a missing type a NULL type was returned (as it was done previously), but all explicitly specified types should remain in place.

Sounds like what I did, thanks for clariication.

I think it's enough to have the algorithm documented somewhere in the ticket.

👌

Refactoring (if I read "make readable" correctly) for sake of readability may be too dangerous given how mission critical this piece of code is.

So I won't even try, thanks for advice.