Silverstripe-framework: Test failure in MySQL 5.7

[sminnee]

I'm getting the following error with MySQL 5.7.11 on my local build. It doesn't happen in Travis.

My assumption is that there is a config setting that makes MySQL a bit more strict (only_full_group_by) that is on by default from MySQL 5.7.5 onwards. We probably want to address this. I can't imagine it's too much of an issue, as PostgreSQL has had this kind of strictness for a long time.

1) SQLSelectTest::testCount
SilverStripe\Model\Connect\DatabaseException: Couldn't run query:

SELECT DISTINCT "SQLSelectTest_DO"."ClassName", "SQLSelectTest_DO"."LastEdited", "SQLSelectTest_DO"."Created", "SQLSelectTest_DO"."Name", "SQLSelectTest_DO"."Meta", "SQLSelectTest_DO"."Common", "SQLSelectTest_DO"."Date", "SQLSelectTest_DO"."ID", 
            CASE WHEN "SQLSelectTest_DO"."ClassName" IS NOT NULL THEN "SQLSelectTest_DO"."ClassName"
            ELSE 'SQLSelectTest_DO' END AS "RecordClassName"

FROM "SQLSelectTest_DO"

GROUP BY Common
 HAVING ("Date" > 2012-02-01)

Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'ss_tmpdb5709632.SQLSelectTest_DO.ClassName' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by

/Users/sminnee/Sites/ss4/framework/model/connect/DBConnector.php:63
/Users/sminnee/Sites/ss4/framework/model/connect/MySQLiConnector.php:141
/Users/sminnee/Sites/ss4/framework/model/connect/MySQLiConnector.php:229
/Users/sminnee/Sites/ss4/framework/model/connect/Database.php:150
/Users/sminnee/Sites/ss4/framework/model/connect/Database.php:201
/Users/sminnee/Sites/ss4/framework/model/connect/Database.php:153
/Users/sminnee/Sites/ss4/framework/model/DB.php:329
/Users/sminnee/Sites/ss4/framework/model/queries/SQLExpression.php:123
/Users/sminnee/Sites/ss4/framework/model/queries/SQLSelect.php:581
/Users/sminnee/Sites/ss4/framework/tests/model/SQLSelectTest.php:44
/Users/sminnee/Sites/ss4/vendor/phpunit/phpunit/phpunit:47

[tractorcow]

+1 this is a bug; We should not be generating invalid SQL, even if prior mysql versions were forgiving of such syntax.

The code that converts grouping queries -> counts could be quite a challenge. Could we accept a less performant (but more robust) solution for certain queries that can't be automatically converted, such as querying the full result, but then discarding after counting rows?

[tractorcow]

It looks as though this is what is already being attempted; (3.x head)

// we can't clear the select if we're relying on its output by a HAVING clause
if(!empty($this->having)) {
    $records = $this->execute();
    return $records->numRecords();
}
// Choose a default column
elseif($column == null) {
    if($this->groupby) {
        $column = 'DISTINCT ' . implode(", ", $this->groupby);
    } else {
        $column = '*';
    }
}

[tractorcow]

Hm, I take that back... the problem seems to be with the initial query constructions.

$qry = SQLQueryTest_DO::get()->dataQuery()->getFinalisedQuery();
$qry->setGroupBy('"Common"');

This seems like a mis-use, to simply apply a grouping to a dataquery, without specifying the aggregation method? The problem is nothing to do with the count at all, as the $qry object is set to an inconsistent state as soon as the above lines are invoked.

We could do one of;

• Update the test to set the SELECT component at the same time as the GROUP BY to aggregate columns. We can document this behaviour as a necessary part of constructing SQL queries for execution (onus is on developer not the framework).
• Provide some kind of default aggregate to apply to columns when running ->groupBy (or some other manipulation of SELECT fragment). Default aggregation could be applied just before query, or as soon as ->setGroupBy() is called.
• Alternatively, instead of aggregating automatically, throw an error where aggregration would be necessary (but isn't done). Essentially causing a different error at the same time, but with more specific information (.e.g. you grouped by column X but didn't aggregate columns Y and Z).

[sminnee]

From what I can see, setGroupBy() isn't used by the core ORM, so this is only for custom query creation.

Perhaps go with the 3rd case, but throw a warning? Since it works on some version of MySQL, that'll tell people that their code is dumb without breaking things that currently work?

[tractorcow]

I've fixed the poor sql usage at https://github.com/silverstripe/silverstripe-framework/pull/6608. Not sure how to best manage future warnings, yet.

[sminnee]

Future warnings?

[tractorcow]

Warnings in case of improper usage. In this case, we just avoid improper usage, but we aren't protecting other users against it.

[oddnoc]

This is really down to MySQL changing the meaning of sql_mode ANSI in version 5.7.5. A possible fix that will protect users and modules from creating queries that run under 5.6 but crash under 5.7 would be for framework to explicitly set modes REAL_AS_FLOAT, PIPES_AS_CONCAT, ANSI_QUOTES, & IGNORE_SPACE (the previous definition of "ANSI"), rather than use the ANSI alias, which is not stable.

[dhensby]

Related:#6632