Problem/Motivation

In D7 and D8 there are three places in the __toString method of the select query class where table aliases are escaped by escapeTable instead of escapeAlias. In D9 only one of these remains.

This means that if a table alias is a MySQL reserved word it's possible for SELECT queries to be constructed which cause errors in MySQL 8 (and MySQL 5.7 in some cases) because the alias is not quoted / escaped properly.

Kernel tests have been written for D8/9 which exercise all three bugs, both with a table name which is a reserved word and an alias which is reserved word.

In D7 this is being addressed in #2978575: Mysql 8 Support on Drupal 7.

Proposed resolution

Ensure that table aliases are escaped for MySQL if they are reserved words.

Remaining tasks

Review patch which passes aliases through escapeAlias instead of escapeTable.

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

tbc

Original IS

We've come across this issue in #2978575-152: Mysql 8 Support on Drupal 7 where we're working on back-porting the MySQL 8 support implemented in D8 in #2966523: MySQL 8 Support. It seems the problem still exists in 8.9.x

Given a table with a name that's a reserved keyword in MySQL 5.7 or MySQL 8, certain SELECT queries can fail as the table name is automatically added as an alias in the SQL and is not escaped / quoted. A table alias which is a reserved word (and maybe different to the table name) causes a similar problem.

In D7 this surfaces in \DatabaseTemporaryQueryTestCase::testTemporaryQuery as that does a countQuery() on the system table (where "system" is reserved).

D8 doesn't have a system table, but the same query fails if we do create a table with a reserved name.

AFAICS this is no longer a problem in D9.x (perhaps because of #2986452: Database reserved keywords need to be quoted as per the ANSI standard ?).

It's also easy to reproduce manually:

mysql> CREATE TABLE `virtual` (`id` int);
$ drush ev "var_dump(db_select('virtual')->countQuery()->execute()->fetchField());"
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'virtual [error]
virtual) subquery' at line 5 in /path/to/drupal-8.x/core/lib/Drupal/Core/Database/Statement.php:59

[...snip...]

Next Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for
the right syntax to use near 'virtual virtual) subquery' at line 5: SELECT COUNT(*) AS expression
FROM
(SELECT 1 AS expression
FROM
{virtual} virtual) subquery; Array
(
)
 in /path/to/drupal-8.x/core/lib/Drupal/Core/Database/Connection.php:701

On closer investigation there are three different places in \Drupal\Core\Database\Query\Select::__toString where table aliases are being escaped by escapeTable instead of escapeAlias.

CommentFileSizeAuthor
#45 3155563-45_d9.patch5.6 KBmcdruid
#45 interdiff-3155563-37-45_d9.txt855 bytesmcdruid
#45 3155563-45_d8.patch6.33 KBmcdruid
#45 interdiff-3155563-37-45_d8.txt1.12 KBmcdruid
#37 3155563-37_d9.patch5.04 KBmcdruid
#37 3155563-37_d9_test_only.patch4.45 KBmcdruid
#37 interdiff-3155563-33-37_d9.txt1.31 KBmcdruid
#37 3155563-37_d8.patch6.19 KBmcdruid
#37 3155563-37_d8_test_only.patch4.47 KBmcdruid
#37 interdiff-3155563-14-37_d8.txt2.44 KBmcdruid
#35 interdiff-33-35.txt1.35 KBHardik_Patel_12
#35 3155563-35.patch5.13 KBHardik_Patel_12
#33 interdiff-3155563-25-33.txt857 bytesmrinalini9
#33 3155563-33_d9.patch5.15 KBmrinalini9
#32 interdiff-3155563-25-32.txt852 bytesmrinalini9
#32 3155563-32_d9.patch5.15 KBmrinalini9
#25 3155563-25_d9.patch5.24 KBmcdruid
#25 3155563-25_d9_test_only.patch4.65 KBmcdruid
#25 interdiff-3155563-21-25_d9.txt2.48 KBmcdruid
#21 3155563-21_d9.patch5.59 KBmcdruid
#21 3155563-21_d9_test_only.patch5 KBmcdruid
#19 3155563-19-d9.patch2.27 KBmcdruid
#19 3155563-19-d9_test_only.patch1.67 KBmcdruid
#18 3155563-18-d9.patch608 bytesmcdruid
#14 3155563-14.patch6.74 KBmcdruid
#14 3155563-14_test_only.patch5.01 KBmcdruid
#10 3155563-10.patch6.74 KBmcdruid
#10 3155563-10_test_only.patch5.01 KBmcdruid
#10 interdiff-3155563-3-10.txt6.53 KBmcdruid
#3 3155563-3.patch2.62 KBmcdruid
#2 3155563-2_test_only.patch1.94 KBmcdruid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcdruid created an issue. See original summary.

mcdruid’s picture

mcdruid’s picture

I think this may be as simple as:

core/lib/Drupal/Core/Database/Query/Select.php
@@ -852,7 +852,7 @@ public function __toString() {
 
       // Don't use the AS keyword for table aliases, as some
       // databases don't support it (e.g., Oracle).
-      $query .= $table_string . ' ' . $this->connection->escapeTable($table['alias']);
+      $query .= $table_string . ' ' . $this->connection->escapeAlias($table['alias']);

It looks like the same change fixes this in D7 as well AFAICS.

I am little confused because locally I'm seeing a difference between manual testing of the query with drush vs. running the phpunit test; manual testing is suggesting that the table name ($table_string above) also needs to be quoted for the query to work, but adding an escapeTable method to the mysql driver which runs table names through \Drupal\Core\Database\Driver\mysql\Connection::quoteIdentifier seems to make the query work via drush but make this test fail... Let's try this patch out before worrying about that.

I'm not adding a test only patch as it would be identical to #2.

mcdruid’s picture

So the new test now passes with MySQL8. Good.

Running more tests to ensure the change doesn't break other dbs.

jungle’s picture

+++ b/core/modules/system/tests/src/Functional/Database/ReservedNameTest.php
@@ -0,0 +1,39 @@
+    $this->assertEqual($this->countTableRows('virtual'), 0, 'Count query on a table with a reserved name was successful.');

One nitpick: $this->assertEqual() -> $this->assertEquals()

TR’s picture

Re: #5. If you're going to use assertEquals(), remember that the parameters are in a different order than assertEqual():
So
+ $this->assertEqual($this->countTableRows('virtual'), 0, 'Count query on a table with a reserved name was successful.');
should be changed to:
+ $this->assertEquals(0, $this->countTableRows('virtual'), 'Count query on a table with a reserved name was successful.');

daffie’s picture

Status: Needs review » Needs work

I am missing how the fix in the current patch fixed the problem given in the IS.

  1. +++ b/core/modules/system/tests/modules/database_test/database_test.install
    --- /dev/null
    +++ b/core/modules/system/tests/src/Functional/Database/ReservedNameTest.php
    

    This test does not need to be a functionaltest. Lets change it to be a kerneltest.

  2. Could you post the patch with only the added test. The patch should fail and by doing so it will prove that the fix does what it is supposed to do.

If you all want support for reserved keywords in Drupal 8.9, then my suggestion would be to backport #2986452: Database reserved keywords need to be quoted as per the ANSI standard.

mcdruid’s picture

Thanks for the review.

#2986452-129: Database reserved keywords need to be quoted as per the ANSI standard (and 130) confirmed that the work on quoting reserved keywords (actually it ended up being quoting all identifiers IIUC) will not be backported to 8.9.x

The test only patch in #2 does fail without the change in #3 and passes when accompanying the change in #3. Fair enough it could be a kerneltest instead; it was pretty much copied from testTemporaryQuery which fails in D7 and which brought this bug to light.

The reason the change in #3 fixes the problem in the IS is that the count query goes from:

SELECT COUNT(*) AS expression
FROM
(SELECT 1 AS expression
FROM
{virtual} virtual) subquery;

...to:

SELECT COUNT(*) AS expression
FROM
(SELECT 1 AS expression
FROM
{virtual} "virtual") subquery;

...whereby the alias in the 2nd query is quoted as it's a reserved word. Therefore MySQL 8 runs the query instead of throwing an error / exception.

This looks like a fairly straightforward bug where the alias was being passed through escapeTable instead of escapeAlias and therefore was not being quoted by the mysql driver if it's a reserved word.

NW to change the test to a kerneltest.

mcdruid’s picture

Title: select queries can fail in MySQL 8 for tables with reserved names » select query should quote aliases which are reserved words in MySQL
Issue summary: View changes
Issue tags: +MySQL 8

Updated IS / title.

mcdruid’s picture

I think there are actually three different places in \Drupal\Core\Database\Query\Select::__toString where table aliases are being escaped by escapeTable instead of escapeAlias.

Here are some kernel tests which hopefully illustrate some of the problems this can cause in MySQL 8 when reserved words are used in select queries.

I'm not sure how useful an interdiff is against the last patch, but I'll include it.

The last submitted patch, 10: 3155563-10_test_only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 10: 3155563-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mcdruid’s picture

Oh, looks like MySQL 5.7 doesn't like some of the reserved words either IIUC; note the test pass for MySQL 5.6

Will take a closer look at the failing tests.

mcdruid’s picture

I'm a bit confused by some of the test results now; I've run the new tests locally on the appropriate environments (e.g. PHP 7.4 + MySQL 5.7, and MySQL 8) and the results are what I expected; i.e. test only patch fails and the full patch passes.

That seems to be reflected in the newer round of drupalci tests that I (re-)ran here, but there's still at least one lingering test showing as a fail (when the same PHP / MySQL combo passed on retest).

I think I'll upload the same pair of patches again to try and get a clean set of tests... just to avoid confusion.

These are the same patches as #10 so no interdiff.

The last submitted patch, 14: 3155563-14_test_only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mcdruid’s picture

Ok, that was the expected result this time. Must have been solar flares in #10 :)

I've updated the IS to hopefully make it all a bit clearer.

I'm hoping someone will now be happy to RTBC this.

The 3 changes are going into the D7 MySQL 8 patch as things stand.

mcdruid’s picture

Looks like this one is still in 9.x

    // FIELDS and EXPRESSIONS                                                      
    $fields = [];                                                                  
    foreach ($this->tables as $alias => $table) {                                  
      if (!empty($table['all_fields'])) {                                          
        $fields[] = $this->connection->escapeTable($alias) . '.*';                 
      }                                                                            
    } 

...but the other two are not.

mcdruid’s picture

Simple patch for 9.1.x to ensure this change doesn't break any existing tests.

Can look at bringing the tests from #14 into D9 too, but I don't expect most of them to fail even without the patch - the all_fields one might do though.

mcdruid’s picture

Looks like this is still a problem for all_fields select queries in 9.1.x which I believe this test only patch should illustrate.

We've already added a very similar test table to the database_test module (one with a table and column name which are both reserved words) in D9 so adding the table from the D8 patches means a bit of duplication. We could backport the select table that's already in D9 to D8 and get rid of the virtual table... or just leave them both in D9.

Let's see if the tests come out the way I'm expecting first...

mcdruid’s picture

Issue summary: View changes

Updated and simplified IS.

mcdruid’s picture

Oops - managed to miss out the actual test(s) as they're in a new file. Let's try that again.

The last submitted patch, 21: 3155563-21_d9_test_only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Database/ReservedWordTest.php
@@ -0,0 +1,87 @@
+    $this->assertEqual($num_records, 1, 'Returned the correct number of rows.');
...
+    $this->assertEqual($rows[0], 'Function value 1', 'Returned the correct value.');
...
+    $this->assertEqual($result->function, 'Function value 1', 'Returned the correct value from an all_fields query.');
...
+    $this->assertEqual($num_records, 4, 'Returned the correct number of rows.');
...
+    $this->assertEqual($record->name, 'George', 'Name field has the correct value.');
+    $this->assertEqual($record->age, 27, 'Age field has the correct value.');
...
+    $this->assertEqual($record->id, 2, 'ID field has the correct value.');
+    $this->assertEqual($record->name, 'George', 'Name field has the correct value.');
+    $this->assertEqual($record->age, 27, 'Age field has the correct value.');
+    $this->assertEqual($record->job, 'Singer', 'Job field has the correct value.');

appreciate this is meant to go all the way down to D7, but assertEqual should not be used since D8 and is formally deprecated in D9. We should use assertEquals($expected, $actual) or even better, with scalars, assertSame($expected, $actual). Also, in this cases I do not think the $message is necessary, re. #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages.

mcdruid’s picture

Thanks for the review; changed them all to assertSame (@jungle had mentioned this earlier).

I wasn't sure about casting some of the results to int in the assertion, but without doing so I got a couple of e.g.

Failed asserting that 27 is identical to '27'.

I've not changed the 8.9 patch, but I think the ReservedWordTest.php is otherwise identical so the D9 version should work with D8 also.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

I wasn't sure about casting some of the results to int

yes, fetch methods from the Statement always return strings, even if the returned db fields are numeric. Maybe one day we'll have to look into that. Using assertEquals would do the casting for us, but especially in database tests I think it's fine to be stricter and do the explicit casting.

The last submitted patch, 25: 3155563-25_d9_test_only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mcdruid’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @mondrake

Oops, I left two casts in this one:

  public function testSelectReservedWordAliasCount() {
    $query = $this->connection->select('test', 'character');
    $num_records = (int) $query->countQuery()->execute()->fetchField();

    $this->assertSame(4, (int) $num_records);

Can do a new patch or the (int) before $query could be removed on commit.

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community

struggling to computer today, apparently

mondrake’s picture

Status: Reviewed & tested by the community » Needs work

Actually, sirry did not notice before, there are a few code style issues reported by the test run.

mrinalini9’s picture

Assigned: Unassigned » mrinalini9
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
5.15 KB
852 bytes

Fixed coding standard issues reported by the test run in #25 and also removed a cast (int) left before $query as mentioned in #28, please review.

mrinalini9’s picture

FileSize
5.15 KB
857 bytes

Please ignore patch #32, here is the updated patch with the fixed coding standard issues reported by the test run in #25 and removal of a cast (int) left before $query as mentioned in #28, please review.

Hardik_Patel_12’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.13 KB
1.35 KB

It's good remove type hinting from assertSame instead of this we can use assertEquals like as follow

-    $this->assertSame(4, (int) $num_records);
+    $this->assertEquals(4, $num_records);

Kindly review a patch.

mondrake’s picture

IMHO, as I said in #26, we should be stricter in database tests. That's because we are testing what comes out of the database API layer, and we should be clear ourselves on what we are expecting.

In the pgsql and sqlite Connection objects, we have in the open method

      // Convert numeric values to strings when fetching.
      \PDO::ATTR_STRINGIFY_FETCHES => TRUE,

So we expect numeric values to be strings (don't know why in MySql it's not necessary to make that explicit, alas, but the effect is the same). Therefore I think we need (again, at least in the Database group of tests) check that the actual matches the expectation. So if we do not want casting, we should have

-    $this->assertSame(4, (int) $num_records);
+    $this->assertSame('4', $num_records);

If at any point in the future we will remove stringification, then it would be natural to adjust the tests to assertSame against an int. But that should be part of that future issue.

mcdruid’s picture

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks

The last submitted patch, 37: 3155563-37_d9_test_only.patch, failed testing. View results

The last submitted patch, 37: 3155563-37_d8_test_only.patch, failed testing. View results

daffie’s picture

Status: Reviewed & tested by the community » Needs work

The patch for D9 is for me RTBC.
For the patch for D8 I have one remark:

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -814,13 +814,13 @@ public function __toString() {
+      $fields[] = (isset($field['table']) ? $this->connection->escapeAlias($field['table']) . '.' : '') . $this->connection->escapeField($field['field']) . ' AS ' . $this->connection->escapeAlias($field['alias']);

The value of $field['table'] should be escaped with the method escapeTable() and not with escapeAlias().

mcdruid’s picture

Status: Needs work » Needs review

The value of $field['table'] should be escaped with the method escapeTable()

I thought the same thing initially - see #2978575-201: Mysql 8 Support on Drupal 7.

However, I now think that it's actually the table alias that we have here in $field['table'] but it's common for the value of that to be the same as the name of the table.

In terms of the SQL syntax, it's the table alias not the table name that we're dealing with.

For example:

$ drush ev '$query = \Drupal::database()->select("node", "table_alias"); $query->addField("table_alias", "nid"); print (string) $query;'
SELECT table_alias.nid AS nid
FROM
{node} table_alias

The code in question in the \Drupal\Core\Database\Query\Select::__toString method is building the table_alias.nid AS nid part of the query.

We can verify this crudely by tinkering with the string that's being built up, e.g.:

$fields[] = (isset($field['table']) ? $this->connection->escapeTable($field['table']) . '.' : '') . $this->connection->escapeField($field['field']) . ' A-hello-S ' . $this->connection->escapeAlias($field['alias']);

Now check the SQL string again:

$ drush ev '$query = \Drupal::database()->select("node", "table_alias"); $query->addField("table_alias", "nid"); print (string) $query;'
SELECT table_alias.nid A-hello-S nid
FROM
{node} table_alias

If that's the case, we could try and address what looks like a misleading / inaccurate naming of that array key/element, but I'd argue that's out of scope of this issue and should be looked at in a follow-up.

In short, even if the value will very often be the same as the table name, in this context it should be treated as a table alias.

I believe the tests testSelectReservedWordTableSpecificField and testSelectReservedWordAliasSpecificFields also illustrate this (i.e. they'll break if we do escapeTable($field['table'])).

daffie’s picture

Status: Needs review » Reviewed & tested by the community

I took another look at the method Drupal\Core\Database\Query\Select::addField() and the value of $field['table'] in

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -814,13 +814,13 @@ public function __toString() {
+      $fields[] = (isset($field['table']) ? $this->connection->escapeAlias($field['table']) . '.' : '') . $this->connection->escapeField($field['field']) . ' AS ' . $this->connection->escapeAlias($field['alias']);

does not hold a table name, but a table alias name. Therefore the change is correct.

Both the patch for D8 and D9 are for me now RTBC.

@mcdruid: Thank you for explaining it to me!

daffie’s picture

Status: Reviewed & tested by the community » Needs work

After talking to @mcdruid on slack, we have decided that it would be best to add a comment to the change:

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -814,13 +814,13 @@ public function __toString() {
-      $fields[] = (isset($field['table']) ? $this->connection->escapeTable($field['table']) . '.' : '') . $this->connection->escapeField($field['field']) . ' AS ' . $this->connection->escapeAlias($field['alias']);
+      $fields[] = (isset($field['table']) ? $this->connection->escapeAlias($field['table']) . '.' : '') . $this->connection->escapeField($field['field']) . ' AS ' . $this->connection->escapeAlias($field['alias']);

Changing the status to needs work for adding the comment.

mcdruid’s picture

Thanks @daffie - yes, agreed that it's worth a comment as it's not obvious that $field['table'] is the table alias.

In the D8 patch here, I've moved the handling of $field['table'] to a separate line, which is more like D9 and I think makes the (code and) comments clearer.

I'm also including a D9 patch which adds the same comment, but you could argue it's not necessary as $table is not being incorrectly escaped anyway. The D9 patch from #37 is fine if we think this new comment is superfluous.

There would be no change to the test-only patches, so they're omitted.

daffie’s picture

The added comment looks good to me.
For me it is RTBC.

@mcdruid: Thank you for adding the comment.

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 959b5e1f11 to 9.1.x and 74c55a5517 to 9.0.x. Thanks!
Committed 9b9e1f0 and pushed to 8.9.x. Thanks!

@mcdruid thanks for coming to D8 & 9 land to fix this!

  • alexpott committed 959b5e1 on 9.1.x
    Issue #3155563 by mcdruid, mrinalini9, Hardik_Patel_12, mondrake, daffie...

  • alexpott committed 74c55a5 on 9.0.x
    Issue #3155563 by mcdruid, mrinalini9, Hardik_Patel_12, mondrake, daffie...

  • alexpott committed 9b9e1f0 on 8.9.x
    Issue #3155563 by mcdruid, mrinalini9, Hardik_Patel_12, mondrake, daffie...
mcdruid’s picture

Yay!

@mcdruid thanks for coming to D8 & 9 land to fix this!

Thanks for having me :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.