TaxonomyVocabularyHierarchyUpdateTest::testTaxonomyUpdateParents() is prone to breaking on PostgreSQL

+++ b/core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
@@ -0,0 +1,92 @@
+  protected function getIndexColumnNames($table_name) {

This function doesn't need to be in a test trait. Maybe move it to some helpful location and call it in assert methods?

Let's see what core committer think. Do we need a test only patch?

+++ b/core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
@@ -0,0 +1,92 @@
+        $index_columns = $connection->query("SELECT	i.relname AS index_name, string_agg(a.attname, ',' ORDER BY a.attname ASC) AS index_columns FROM pg_class t, pg_class i, pg_index ix, pg_attribute a WHERE t.oid = ix.indrelid AND i.oid = ix.indexrelid AND a.attrelid = t.oid AND a.attnum = ANY(ix.indkey) AND t.relkind = 'r' AND t.relname = :table_name GROUP BY i.relname", [

Wired whitespace between SELECT and i.

+++ b/core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
@@ -0,0 +1,135 @@
+  protected function getIndexColumnNames($table_name, $index_type) {

Do we need tests for this method? :D

+++ b/core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
@@ -0,0 +1,135 @@
+  /**
+   * Returns the column names used by an index of a table.
+   *
+   * @param string $table_name
+   *   A table name.
+   * @param string $index_type
+   *   The type of the index. Can be one of 'index', 'unique' or 'primary'.
+   *
+   * @return array
+   *   An array keyed by the index name and values as strings in the form of
+   *   'column1,column2'.
+   */
+  protected function getIndexColumnNames($table_name, $index_type) {
+    assert(in_array($index_type, ['index', 'unique', 'primary'], TRUE));
+    $connection = \Drupal::database();
+

Couldn't this be a method on the Schema API? Like findPrimaryKeyColumns($table). That way also contrib drivers could add their own implementation.

Edit - see #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test

Given that we already have ::findPrimaryKeyColumns(), I guess a ::findIndexColumns() method would make sense.

Perfect! this looks much better now.

1. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -545,6 +545,28 @@ protected function findPrimaryKeyColumns($table) {
   +    throw new \RuntimeException("The '{$this->connection->driver()}' database driver does not implement " . __METHOD__);
   

   I was about to suggest to switch to BadMethodCallException, but given that the driver is configurable, a runtime exception makes sense to me.

2. +++ b/core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
   @@ -0,0 +1,83 @@
   +    foreach ($this->getIndexColumnNames($table_name, $index_type) as $index_name => $index_columns) {
   ...
   +    foreach ($this->getIndexColumnNames($table_name, $index_type) as $index_name => $index_columns) {
   

   I was initially confused by the $index_name variable, since it is unused. Can we remove it?

3. +++ b/core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
   @@ -0,0 +1,83 @@
   +      if ($column_names == $index_columns) {
   +        $this->assertTrue(TRUE);
   +      }
   +    }
   +    $this->assertFalse(FALSE);
   ...
   +      if ($column_names == $index_columns) {
   +        $this->assertFalse(FALSE);
   +      }
   +    }
   +    $this->assertTrue(TRUE);
   

   It seems to me that this logic is a bit messed up: all assertions here, especially the last lines, are no-ops. There's no way for that code to fail :)
   I guess the original goal was to fail as soon as a non-matching condition was found and only assert TRUE if all were met. Something like this:

       foreach ($this->getIndexColumnNames($table_name, $index_type) as $index_columns) {
         if ($column_names !== $index_columns) {
           $this->assertTrue(FALSE);
           return; // Not strictly needed, probably.
         }
       }
       $this->assertTrue(TRUE);
   

   Also, would it make sense to use strict comparison here?

4. +++ b/core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
   @@ -0,0 +1,83 @@
   +    $introspect_index_schema = new \ReflectionMethod(get_class($schema), 'introspectIndexSchema');
   

   What about making the method public? Being able to introspect the actual schema might turn out to be useful while doing fancy stuff with it ;)
   Moreover I'm not sure we normally "open" protected methods this way in tests, given that we are not testing the method return value itself.
   Which leads me to the next point :)

5. +++ b/core/tests/Drupal/Tests/Core/Database/SchemaIntrospectionTestTrait.php
   @@ -0,0 +1,83 @@
   +      $indexes = [$index_schema['primary key']];
   

   For consistency would it make sense to add a key name here as well? Otherwise the index name will be an integer in this case and that might cause issues with loose equality in the caller code.

   Regardless of that, it seems that we are not actually checking that all drivers are returning the exact same data structure (although it seems they are), given that index name keys are unused. Probably it would be a good idea to add a small test explicitly testing ::introspectIndexSchema() itself.

I guess the original goal was to fail as soon as a non-matching condition was found and only assert TRUE if all were met.

Now that I think about it, that wouldn't work if multiple indexes were returned. However the logic as it is now doesn't seem to work either.

Maybe something like this?

  protected function assertIndexOnColumns($table_name, array $column_names, $index_type = 'index') {
    foreach ($this->getIndexColumnNames($table_name, $index_type) as $index_columns) {
      if ($column_names === $index_columns) {
        $this->assertTrue(TRUE);
        return;
      }
    }
    $this->assertTrue(FALSE);
  }

  protected function assertNoIndexOnColumns($table_name, array $column_names, $index_type = 'index') {
    foreach ($this->getIndexColumnNames($table_name, $index_type) as $index_columns) {
      if ($column_names === $index_columns) {
        $this->assertTrue(FALSE);
        return;
      }
    }
    $this->assertTrue(TRUE);
  }

Thanks for the new test, it looks great!

1: Good point :)
2: 👍
3: That's what I was asking: would it make sense to be able to assert also the column order? I mean, since you are writing a test you can bother providing the correct order, and if the test starts failing maybe that means something got messed up with the schema. Not feeling strongly about this but I thought it would be good to point it out. Or are we implying that the some drivers may not provide a predictable order?
4:

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -274,6 +274,83 @@ public function testSchema() {
+    $introspect_index_schema = new \ReflectionMethod(get_class($this->schema), 'introspectIndexSchema');
+    $introspect_index_schema->setAccessible(TRUE);
+    $index_schema = $introspect_index_schema->invoke($this->schema, $table_name);

That's what I meant earlier: we normally make protected methods accessible in tests to assert their result, not to get access to internal APIs. The latter seems to suggest we need a public API instead. However, I'm fine with leaving the method protected, if the goal is to have a complete API instead.

5: Thanks!

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -631,7 +631,7 @@ protected function introspectIndexSchema($table) {
-      elseif ($row->Non_unique != 0) {
+      elseif ($row->Non_unique == 0) {

Tricky :)

Was this caught by the new test?

Now that we have a test for asserting its result, I hope you feel better with leaving it protected :)

Yep, this looks good to go to me :)

Saving credits

Committed 431b5b4 and pushed to 8.7.x. Thanks!

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -309,6 +274,83 @@ public function testSchema() {
+      'primary key' => ['id', 'test_field_1'],
+      'unique keys' => [
+        '2' => ['test_field_2'],
+        '3_4' => ['test_field_3', 'test_field_4'],
+      ],
+      'indexes' => [
+        '4' => ['test_field_4'],
+        '4_5' => ['test_field_4', 'test_field_5'],
+      ],

Uhm, numeric only index name like '2' can create issues in contrib as these can be interpreted as numbers rather than identifiers, e.g.

Drupal\KernelTests\Core\Database\SchemaTest::testIntrospectIndexSchema
Doctrine\DBAL\Exception\SyntaxErrorException: An exception occurred while executing 'CREATE UNIQUE INDEX 2 ON test97095778mhun9p3n (test_field_2)':

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 '2 ON test97095778mhun9p3n (test_field_2)' at line 1

Is it a big deal to change those to sth like

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -309,6 +274,83 @@ public function testSchema() {
+      'primary key' => ['id', 'test_field_1'],
+      'unique keys' => [
+        'uk_2' => ['test_field_2'],
+        'uk_3_4' => ['test_field_3', 'test_field_4'],
+      ],
+      'indexes' => [
+        'idx_4' => ['test_field_4'],
+        'idx_4_5' => ['test_field_4', 'test_field_5'],
+      ],

?

Great. Thanks :)

I was thinking about other ways a contrib driver could be affected by this: should we catch the not-implemented exception and prevent the test from failing if the implementation is missing?

#28 actually only tests would fail here - not normal runtime code. And only if you run the core testsuite under the contrib driver. So I find the exception raised as a good 'call for action' to contrib driver developers that would have to implement the missing pieces if they want to keep up with the evolving db abstraction layer. BTW this also applies to ::findPrimaryKeyColumns().

Maybe a change record would be good, anyway?

See https://www.drupal.org/node/2974956 for the change record that was done for #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test

Well, this is not a public API so a CR would only be helpful for DB driver developers, but why not? :)

#28 actually only tests would fail here - not normal runtime code.

Yep, I was suggesting to catch the failure in the test, however if core starts using that method in runtime code, it will break when using a driver not implementing that, so I agree it's fine to let the test fail.

Committed #26 as f5d6ace and pushed it to 8.7.x. Thanks!

Let's keep this fixed now ;)