From a17303166447173af60d8d4b3fbfee37f098a692 Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Thu, 3 Sep 2015 12:59:34 +0100
Subject: [PATCH] Issue #2561129 by plach, alexpott, catch: Composite indexes
are not correctly deleted/re-created when updating a field storage definition
---
.../Core/Database/Driver/pgsql/Schema.php | 23 ++-
.../Sql/SqlContentEntityStorageSchema.php | 181 +++++++++++++-----
.../Entity/EntityDefinitionUpdateTest.php | 12 ++
.../Driver/pgsql/PostgresqlSchemaTest.php | 81 ++++++++
.../Sql/SqlContentEntityStorageSchemaTest.php | 4 +-
5 files changed, 251 insertions(+), 50 deletions(-)
create mode 100644 core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlSchemaTest.php
diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
index 06b754035647..7e1e0b77ac94 100644
--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -588,13 +588,28 @@ public function indexExists($table, $name) {
/**
* Helper function: check if a constraint (PK, FK, UK) exists.
*
- * @param $table
+ * @param string $table
* The name of the table.
- * @param $name
- * The name of the constraint (typically 'pkey' or '[constraint]_key').
+ * @param string $name
+ * The name of the constraint (typically 'pkey' or '[constraint]__key').
+ *
+ * @return bool
+ * TRUE if the constraint exists, FALSE otherwise.
*/
public function constraintExists($table, $name) {
- $constraint_name = $this->ensureIdentifiersLength($table, $name);
+ // ::ensureIdentifiersLength() expects three parameters, although not
+ // explicitly stated in its signature, thus we split our constraint name in
+ // a proper name and a suffix.
+ if ($name == 'pkey') {
+ $suffix = $name;
+ $name = '';
+ }
+ else {
+ $pos = strrpos($name, '__');
+ $suffix = substr($name, $pos + 2);
+ $name = substr($name, 0, $pos);
+ }
+ $constraint_name = $this->ensureIdentifiersLength($table, $name, $suffix);
// Remove leading and trailing quotes because the index name is in a WHERE
// clause and not used as an identifier.
$constraint_name = str_replace('"', '', $constraint_name);
diff --git a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
index 9adffbd8182a..729de1a96933 100644
--- a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -328,42 +328,12 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
}
}
else {
- $schema_handler = $this->database->schema();
-
// Drop original indexes and unique keys.
- foreach ($this->loadEntitySchemaData($entity_type) as $table_name => $schema) {
- if (!empty($schema['indexes'])) {
- foreach ($schema['indexes'] as $name => $specifier) {
- $schema_handler->dropIndex($table_name, $name);
- }
- }
- if (!empty($schema['unique keys'])) {
- foreach ($schema['unique keys'] as $name => $specifier) {
- $schema_handler->dropUniqueKey($table_name, $name);
- }
- }
- }
+ $this->deleteEntitySchemaIndexes($this->loadEntitySchemaData($entity_type));
// Create new indexes and unique keys.
$entity_schema = $this->getEntitySchema($entity_type, TRUE);
- foreach ($this->getEntitySchemaData($entity_type, $entity_schema) as $table_name => $schema) {
- // Add fields schema because database driver may depend on this data to
- // perform index normalization.
- $schema['fields'] = $entity_schema[$table_name]['fields'];
-
- if (!empty($schema['indexes'])) {
- foreach ($schema['indexes'] as $name => $specifier) {
- // Check if the index exists because it might already have been
- // created as part of the earlier entity type update event.
- $this->addIndexIfNotExists($table_name, $name, $specifier, $schema);
- }
- }
- if (!empty($schema['unique keys'])) {
- foreach ($schema['unique keys'] as $name => $specifier) {
- $schema_handler->addUniqueKey($table_name, $name, $specifier);
- }
- }
- }
+ $this->createEntitySchemaIndexes($entity_schema);
// Store the updated entity schema.
$this->saveEntitySchemaData($entity_type, $entity_schema);
@@ -1161,7 +1131,7 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor
foreach ($schema[$table_name]['indexes'] as $name => $specifier) {
// Check if the index exists because it might already have been
// created as part of the earlier entity type update event.
- $this->addIndexIfNotExists($table_name, $name, $specifier, $schema[$table_name]);
+ $this->addIndex($table_name, $name, $specifier, $schema[$table_name]);
}
}
if (!empty($schema[$table_name]['unique keys'])) {
@@ -1175,7 +1145,14 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor
}
}
}
+
$this->saveFieldSchemaData($storage_definition, $schema);
+
+ if (!$only_save) {
+ // Make sure any entity index involving this field is re-created if
+ // needed.
+ $this->createEntitySchemaIndexes($this->getEntitySchema($this->entityType), $storage_definition);
+ }
}
/**
@@ -1206,6 +1183,9 @@ protected function deleteDedicatedTableSchema(FieldStorageDefinitionInterface $s
* The storage definition of the field being deleted.
*/
protected function deleteSharedTableSchema(FieldStorageDefinitionInterface $storage_definition) {
+ // Make sure any entity index involving this field is dropped.
+ $this->deleteEntitySchemaIndexes($this->loadEntitySchemaData($this->entityType), $storage_definition);
+
$deleted_field_name = $storage_definition->getName();
$table_mapping = $this->storage->getTableMapping(
$this->entityManager->getLastInstalledFieldStorageDefinitions($this->entityType->id())
@@ -1329,8 +1309,8 @@ protected function updateDedicatedTableSchema(FieldStorageDefinitionInterface $s
}
// Check if the index exists because it might already have been
// created as part of the earlier entity type update event.
- $this->addIndexIfNotExists($table, $real_name, $real_columns, $actual_schema[$table]);
- $this->addIndexIfNotExists($revision_table, $real_name, $real_columns, $actual_schema[$revision_table]);
+ $this->addIndex($table, $real_name, $real_columns, $actual_schema[$table]);
+ $this->addIndex($revision_table, $real_name, $real_columns, $actual_schema[$revision_table]);
}
}
$this->saveFieldSchemaData($storage_definition, $this->getDedicatedTableSchema($storage_definition));
@@ -1423,7 +1403,7 @@ protected function updateSharedTableSchema(FieldStorageDefinitionInterface $stor
foreach ($schema[$table_name]['indexes'] as $name => $specifier) {
// Check if the index exists because it might already have been
// created as part of the earlier entity type update event.
- $this->addIndexIfNotExists($table_name, $name, $specifier, $schema[$table_name]);
+ $this->addIndex($table_name, $name, $specifier, $schema[$table_name]);
}
}
@@ -1441,6 +1421,100 @@ protected function updateSharedTableSchema(FieldStorageDefinitionInterface $stor
}
}
+ /**
+ * Creates the specified entity schema indexes and keys.
+ *
+ * @param array $entity_schema
+ * The entity schema definition.
+ * @param \Drupal\Core\Field\FieldStorageDefinitionInterface|NULL $storage_definition
+ * (optional) If a field storage definition is specified, only indexes and
+ * keys involving its columns will be processed. Otherwise all defined
+ * entity indexes and keys will be processed.
+ */
+ protected function createEntitySchemaIndexes(array $entity_schema, FieldStorageDefinitionInterface $storage_definition = NULL) {
+ $schema_handler = $this->database->schema();
+
+ if ($storage_definition) {
+ $table_mapping = $this->storage->getTableMapping();
+ $column_names = $table_mapping->getColumnNames($storage_definition->getName());
+ }
+
+ $index_keys = [
+ 'indexes' => 'addIndex',
+ 'unique keys' => 'addUniqueKey',
+ ];
+
+ foreach ($this->getEntitySchemaData($this->entityType, $entity_schema) as $table_name => $schema) {
+ // Add fields schema because database driver may depend on this data to
+ // perform index normalization.
+ $schema['fields'] = $entity_schema[$table_name]['fields'];
+
+ foreach ($index_keys as $key => $add_method) {
+ if (!empty($schema[$key])) {
+ foreach ($schema[$key] as $name => $specifier) {
+ // If a set of field columns were specified we process only indexes
+ // involving them. Only indexes for which all columns exist are
+ // actually created.
+ $create = FALSE;
+ $specifier_columns = array_map(function($item) { return is_string($item) ? $item : reset($item); }, $specifier);
+ if (!isset($column_names) || array_intersect($specifier_columns, $column_names)) {
+ $create = TRUE;
+ foreach ($specifier_columns as $specifier_column_name) {
+ // This may happen when adding more than one field in the same
+ // update run. Eventually when all field columns have been
+ // created the index will be created too.
+ if (!$schema_handler->fieldExists($table_name, $specifier_column_name)) {
+ $create = FALSE;
+ break;
+ }
+ }
+ }
+ if ($create) {
+ $this->{$add_method}($table_name, $name, $specifier, $schema);
+ }
+ }
+ }
+ }
+ }
+ }
+
+ /**
+ * Deletes the specified entity schema indexes and keys.
+ *
+ * @param array $entity_schema_data
+ * The entity schema data definition.
+ * @param \Drupal\Core\Field\FieldStorageDefinitionInterface|NULL $storage_definition
+ * (optional) If a field storage definition is specified, only indexes and
+ * keys involving its columns will be processed. Otherwise all defined
+ * entity indexes and keys will be processed.
+ */
+ protected function deleteEntitySchemaIndexes(array $entity_schema_data, FieldStorageDefinitionInterface $storage_definition = NULL) {
+ $schema_handler = $this->database->schema();
+
+ if ($storage_definition) {
+ $table_mapping = $this->storage->getTableMapping();
+ $column_names = $table_mapping->getColumnNames($storage_definition->getName());
+ }
+
+ $index_keys = [
+ 'indexes' => 'dropIndex',
+ 'unique keys' => 'dropUniqueKey',
+ ];
+
+ foreach ($entity_schema_data as $table_name => $schema) {
+ foreach ($index_keys as $key => $drop_method) {
+ if (!empty($schema[$key])) {
+ foreach ($schema[$key] as $name => $specifier) {
+ $specifier_columns = array_map(function($item) { return is_string($item) ? $item : reset($item); }, $specifier);
+ if (!isset($column_names) || array_intersect($specifier_columns, $column_names)) {
+ $schema_handler->{$drop_method}($table_name, $name);
+ }
+ }
+ }
+ }
+ }
+ }
+
/**
* Checks whether a field property has NULL values.
*
@@ -1873,24 +1947,41 @@ protected function getColumnSchemaRelevantKeys() {
}
/**
- * Create an index if it doesn't already exist.
+ * Creates an index, dropping it if already existing.
*
* @param string $table
* The table name.
* @param string $name
* The index name.
- * @param array $fields
+ * @param array $specifier
* The fields to index.
- * @param array $spec
+ * @param array $schema
* The table specification.
*
- * For the full parameter descriptions see
- * \Drupal\Core\Database\Schema::addIndex().
+ * @see \Drupal\Core\Database\Schema::addIndex()
*/
- protected function addIndexIfNotExists($table, $name, array $fields, array $spec) {
- if (!$this->database->schema()->indexExists($table, $name)) {
- $this->database->schema()->addIndex($table, $name, $fields, $spec);
- }
+ protected function addIndex($table, $name, array $specifier, array $schema) {
+ $schema_handler = $this->database->schema();
+ $schema_handler->dropIndex($table, $name);
+ $schema_handler->addIndex($table, $name, $specifier, $schema);
+ }
+
+ /**
+ * Creates a unique key, dropping it if already existing.
+ *
+ * @param string $table
+ * The table name.
+ * @param string $name
+ * The index name.
+ * @param array $specifier
+ * The unique fields.
+ *
+ * @see \Drupal\Core\Database\Schema::addUniqueKey()
+ */
+ protected function addUniqueKey($table, $name, array $specifier) {
+ $schema_handler = $this->database->schema();
+ $schema_handler->dropUniqueKey($table, $name);
+ $schema_handler->addUniqueKey($table, $name, $specifier);
}
}
diff --git a/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
index a1980854f33f..0492b2f9be9d 100644
--- a/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
+++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
@@ -509,6 +509,18 @@ public function testEntityIndexCreateDeleteWithoutData() {
// Run the update and ensure the index is deleted.
$this->entityDefinitionUpdateManager->applyUpdates();
$this->assertFalse($this->database->schema()->indexExists('entity_test_update', 'entity_test_update__new_index'), 'Index deleted.');
+
+ // Test that composite indexes are handled correctly when dropping and
+ // re-creating one of their columns.
+ $this->addEntityIndex();
+ $this->entityDefinitionUpdateManager->applyUpdates();
+ $storage_definition = $this->entityDefinitionUpdateManager->getFieldStorageDefinition('name', 'entity_test_update');
+ $this->entityDefinitionUpdateManager->updateFieldStorageDefinition($storage_definition);
+ $this->assertTrue($this->database->schema()->indexExists('entity_test_update', 'entity_test_update__new_index'), 'Index created.');
+ $this->entityDefinitionUpdateManager->uninstallFieldStorageDefinition($storage_definition);
+ $this->assertFalse($this->database->schema()->indexExists('entity_test_update', 'entity_test_update__new_index'), 'Index deleted.');
+ $this->entityDefinitionUpdateManager->installFieldStorageDefinition('name', 'entity_test_update', 'entity_test', $storage_definition);
+ $this->assertTrue($this->database->schema()->indexExists('entity_test_update', 'entity_test_update__new_index'), 'Index created again.');
}
/**
diff --git a/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlSchemaTest.php b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlSchemaTest.php
new file mode 100644
index 000000000000..2d896466344b
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlSchemaTest.php
@@ -0,0 +1,81 @@
+<?php
+/**
+ * @file
+ * Contains \Drupal\Tests\Core\Database\Driver\pgsql\PostgresqlSchemaTest.
+ */
+
+namespace Drupal\Tests\Core\Database\Driver\pgsql;
+
+use Drupal\Core\Database\Driver\pgsql\Schema;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Database\Driver\pgsql\Schema
+ * @group Database
+ */
+class PostgresqlSchemaTest extends UnitTestCase {
+
+ /**
+ * The PostgreSql DB connection.
+ *
+ * @var \PHPUnit_Framework_MockObject_MockObject|\Drupal\Core\Database\Driver\pgsql\Connection
+ */
+ protected $connection;
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp() {
+ parent::setUp();
+
+ $this->connection = $this->getMockBuilder('\Drupal\Core\Database\Driver\pgsql\Connection')
+ ->disableOriginalConstructor()
+ ->getMock();
+ }
+
+ /**
+ * Tests whether the actual constraint name is correctly computed.
+ *
+ * @param string $table_name
+ * The table name the constrained column belongs to.
+ * @param string $name
+ * The constraint name.
+ * @param string $expected
+ * The expected computed constraint name.
+ *
+ * @covers ::constraintExists
+ * @dataProvider providerComputedConstraintName
+ */
+ public function testComputedConstraintName($table_name, $name, $expected) {
+ $max_identifier_length = 63;
+ $schema = new Schema($this->connection);
+
+ $statement = $this->getMock('\Drupal\Core\Database\StatementInterface');
+ $statement->expects($this->any())
+ ->method('fetchField')
+ ->willReturn($max_identifier_length);
+
+ $this->connection->expects($this->any())
+ ->method('query')
+ ->willReturn($statement);
+
+ $this->connection->expects($this->at(2))
+ ->method('query')
+ ->with("SELECT 1 FROM pg_constraint WHERE conname = '$expected'")
+ ->willReturn($this->getMock('\Drupal\Core\Database\StatementInterface'));
+
+ $schema->constraintExists($table_name, $name);
+ }
+
+ /**
+ * Data provider for ::testComputedConstraintName().
+ */
+ public function providerComputedConstraintName() {
+ return [
+ ['user_field_data', 'pkey', 'user_field_data____pkey'],
+ ['user_field_data', 'name__key', 'user_field_data__name__key'],
+ ['user_field_data', 'a_veeeery_veery_very_super_long_field_name__key', 'drupal_BGGYAXgbqlAF1rMOyFTdZGj9zIMXZtSvEjMAKZ9wGIk_key'],
+ ];
+ }
+
+}
diff --git a/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
index 83e9e50169a6..84ec3f22fb18 100644
--- a/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
@@ -1395,7 +1395,6 @@ public function setUpStorageDefinition($field_name, array $schema) {
* ::onEntityTypeUpdate
*/
public function testonEntityTypeUpdateWithNewIndex() {
- $entity_type_id = 'entity_test';
$this->entityType = $original_entity_type = new ContentEntityType(array(
'id' => 'entity_test',
'entity_keys' => array('id' => 'id'),
@@ -1465,6 +1464,9 @@ public function testonEntityTypeUpdateWithNewIndex() {
->method('dropIndex')
->with('entity_test', 'entity_test__removed_field');
+ $this->dbSchemaHandler->expects($this->atLeastOnce())
+ ->method('fieldExists')
+ ->willReturn(TRUE);
$this->dbSchemaHandler->expects($this->atLeastOnce())
->method('addIndex')
->with('entity_test', 'entity_test__b588603cb9', [['long_index_name', 10]], $this->callback(function($actual_value) use ($expected) {
--
GitLab