Problem/Motivation

This module is issueing queries with duplicate placeholders, which are not allowed per the PDO specification.

Per the Drupal database placeholder documentation, "A query may have any number of placeholders, but all must have unique names even if they have the same value."

The query is also building aggregates incorrectly. There is proposal to enforce proper aggregate building at the database level in MySQL for D8.

#567148: Use ONLY_FULL_GROUP_BY for MySQL

Proposed resolution

Pending.

Remaining tasks

Pending.

User interface changes

None.

API changes

None.

Original report

I had to make serverl adjustments to make multi word searches compatible with SQL Server.

Problems Summary:

[1] When constructing createKeysQuery the OR statement is reused. This poses a problem for the SQL Server PDO driver because it re-uses parameter replacement

(SELECT * FROM A WHERE A.field1 = :param1 AND A.field2 = :param1) -> where param1 = 'SAMPLETEXT'

it should be

(SELECT * FROM A WHERE A.field1 = :param1 AND A.field2 = :param2) -> where param1 = param2 = 'SAMPLETEXT'

I believe this might be a bug in the the driver itself but the proposed changes in the module make it compatible without breaking anything.

[2] As usual, SQL Server complains when selecting a field not contained in aggreate function:

Column 'history.id' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.

Patch attached. Sorry, no GIT.

Comments

drunken monkey’s picture

Status: Active » Needs review
StatusFileSize
new1.48 KB

Thanks a lot for reporting this and providing a patch right away!
It's no big problem that you didn't use Git – however, it seems you used an older version, and also made some changes (in the last part of the patch) in the "original" version from which the patch is derived. This made it impossible to apply, I had to edit the changes in manually.
So please, next time be sure to incorporate all changes in the patch, and use the latest dev version for preparing it!

Other than that, your patch looks good. I tested it, and it at least doesn't break anything obvious on MySQL. (From the code changes, I also don't think it would.) Two question, however:

+++ D:/service.inc	Sat Jun 01 14:04:40 2013
@@ -863,9 +859,13 @@
-          $query->fields('t');
+          $query->fields('t', array('item_id', 'score','word'));

Why is this change necessary? Doesn't SQL Server support SELECT t.* … syntax?

+++ D:/service.inc	Sat Jun 01 14:04:40 2013
@@ -1084,6 +1084,9 @@
+	if (!$db_query->getGroupBy()) {
+        $db_query->groupBy('t.item_id');

This would be a severe error (even though regrettably not reported) in MySQL and other SQL servers, too. (At least, as long as there is an aggregation present.)
Do you know when such a query is constructed without the patch?
Or does SQL Server just always require a GROUP BY, even if no aggregation is present?

Apart from that, please just test/review the attached patch (against a clean checkout of the module's latest dev version) and see if it, too fixes your problems on SQL Server.

david_garcia’s picture

Hi, I just started really working with drupal about 3 months ago so I'm just learning on contributions.

Answer your questions:

[1] Why is this change necessary? Doesn't SQL Server support SELECT t.* … syntax?

Yes it does, but because an aggregate is being used in the query you cannot select all fields unless you include all of them in the aggreate function.

[2] This would be a severe error (even though regrettably not reported) in MySQL and other SQL servers, too. (At least, as long as there is an aggregation present.)

You are right, I missplaced the addition of the aggreate. I have to find out where the offending query is built and add it there.

drunken monkey’s picture

Hi, I just started really working with drupal about 3 months ago so I'm just learning on contributions.

Oh, OK, then thanks for posting a patch at all, and please just keep my notes in mind for the future. (And if you plan to contribute more often, or managing your own code, definitely get used to Git, it makes things really much easier.)

Yes it does, but because an aggregate is being used in the query you cannot select all fields unless you include all of them in the aggreate function.

But there are only three fields in the table, which are the three you added. So there should be no problem then, right? (Also, if all three fields are selected, I'm pretty sure there will never be an aggregate added. Otherwise, there'd be two score columns.)

You are right, I missplaced the addition of the aggreate. I have to find out where the offending query is built and add it there.

Yes, it would be great if you could find out the proper place this should be corrected. Thanks!

drunken monkey’s picture

Status: Needs review » Needs work
david_garcia’s picture

Title: Sql Server Compatibility » Bad usage of database abstraction layer
Issue summary: View changes