From 7e6c8093679887b8cf9f7eaac8ee2ae064c7b2de Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Thu, 16 Jan 2025 10:10:22 +1300 Subject: [PATCH 1/3] NEW Provide clear UX validation errors when violating unique index (#11558) --- src/ORM/Connect/DBConnector.php | 19 ++- src/ORM/Connect/DuplicateEntryException.php | 50 ++++++++ src/ORM/Connect/MySQLiConnector.php | 55 ++++++++- src/ORM/DataObject.php | 53 ++++++-- tests/php/ORM/DataObjectTest.php | 58 +++++++++ .../ORM/DataObjectTest/UniqueIndexObject.php | 33 +++++ tests/php/ORM/MySQLiConnectorTest.php | 113 ++++++++++++++++-- 7 files changed, 360 insertions(+), 21 deletions(-) create mode 100644 src/ORM/Connect/DuplicateEntryException.php create mode 100644 tests/php/ORM/DataObjectTest/UniqueIndexObject.php diff --git a/src/ORM/Connect/DBConnector.php b/src/ORM/Connect/DBConnector.php index 5e0413f3f6e..6b82b7e596b 100644 --- a/src/ORM/Connect/DBConnector.php +++ b/src/ORM/Connect/DBConnector.php @@ -52,7 +52,7 @@ protected function databaseError($msg, $errorLevel = E_USER_ERROR, $sql = null, // Format query if given if (!empty($sql)) { - $formatter = new SQLFormatter(); + $formatter = SQLFormatter::create(); $formattedSQL = $formatter->formatPlain($sql); $msg = "Couldn't run query:\n\n{$formattedSQL}\n\n{$msg}"; } @@ -66,6 +66,23 @@ protected function databaseError($msg, $errorLevel = E_USER_ERROR, $sql = null, } } + protected function duplicateEntryError( + string $msg, + ?string $keyName, + ?string $duplicatedValue, + ?string $sql = null, + array $parameters = [] + ): void { + // Format query if given + if (!empty($sql)) { + $formatter = SQLFormatter::create(); + $formattedSQL = $formatter->formatPlain($sql); + $msg = "Couldn't run query:\n\n{$formattedSQL}\n\n{$msg}"; + } + + throw new DuplicateEntryException($msg, $keyName, $duplicatedValue, $sql, $parameters); + } + /** * Determine if this SQL statement is a destructive operation (write or ddl) * diff --git a/src/ORM/Connect/DuplicateEntryException.php b/src/ORM/Connect/DuplicateEntryException.php new file mode 100644 index 00000000000..91985d9ee79 --- /dev/null +++ b/src/ORM/Connect/DuplicateEntryException.php @@ -0,0 +1,50 @@ +keyName = $keyName; + $this->duplicatedValue = $duplicatedValue; + } + + /** + * Get the name of the key which the error is for (e.g. index name) + */ + public function getKeyName(): ?string + { + return $this->keyName; + } + + /** + * Get the value which was duplicated (e.g. combined value of multiple columns in index) + */ + public function getDuplicatedValue(): ?string + { + return $this->duplicatedValue; + } +} diff --git a/src/ORM/Connect/MySQLiConnector.php b/src/ORM/Connect/MySQLiConnector.php index 2c60d57ca1f..4b3a98910ca 100644 --- a/src/ORM/Connect/MySQLiConnector.php +++ b/src/ORM/Connect/MySQLiConnector.php @@ -2,6 +2,7 @@ namespace SilverStripe\ORM\Connect; +use Exception; use mysqli; use mysqli_sql_exception; use mysqli_stmt; @@ -65,12 +66,18 @@ public function prepareStatement($sql, &$success) // Record last statement for error reporting $statement = $this->dbConn->stmt_init(); $this->setLastStatement($statement); + try { $success = $statement->prepare($sql); } catch (mysqli_sql_exception $e) { $success = false; - $this->databaseError($e->getMessage(), E_USER_ERROR, $sql); + $this->throwRelevantError($e->getMessage(), $e->getCode(), E_USER_ERROR, $sql, []); } + + if (!$success || $statement->error) { + $this->throwRelevantError($this->getLastError(), $this->getLastErrorCode(), E_USER_ERROR, $sql, []); + } + return $statement; } @@ -190,17 +197,19 @@ public function query($sql, $errorLevel = E_USER_ERROR) { $this->beforeQuery($sql); - $error = null; + $exception = null; $handle = null; try { // Benchmark query $handle = $this->dbConn->query($sql ?? '', MYSQLI_STORE_RESULT); } catch (mysqli_sql_exception $e) { - $error = $e->getMessage(); + $exception = $e; } finally { if (!$handle || $this->dbConn->error) { - $this->databaseError($error ?? $this->getLastError(), $errorLevel, $sql); + $errorMsg = $exception ? $exception->getMessage() : $this->getLastError(); + $errorCode = $exception ? $exception->getCode() : $this->getLastErrorCode(); + $this->throwRelevantError($errorMsg, $errorCode, $errorLevel, $sql, []); return null; } } @@ -319,13 +328,13 @@ public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) $statement->execute(); } catch (mysqli_sql_exception $e) { $success = false; - $this->databaseError($e->getMessage(), E_USER_ERROR, $sql, $parameters); + $this->throwRelevantError($e->getMessage(), $e->getCode(), $errorLevel, $sql, $parameters); } } if (!$success || $statement->error) { $values = $this->parameterValues($parameters); - $this->databaseError($this->getLastError(), $errorLevel, $sql, $values); + $this->throwRelevantError($this->getLastError(), $this->getLastErrorCode(), $errorLevel, $sql, $values); return null; } @@ -382,4 +391,38 @@ public function getLastError() } return $this->dbConn->error; } + + public function getLastErrorCode(): int + { + // Check if a statement was used for the most recent query + if ($this->lastStatement && $this->lastStatement->errno) { + return $this->lastStatement->errno; + } + return $this->dbConn->errno; + } + + /** + * Throw the correct DatabaseException for this error + * + * @throws DatabaseException + */ + private function throwRelevantError(string $message, int $code, int $errorLevel, ?string $sql, array $parameters): void + { + if ($errorLevel === E_USER_ERROR && ($code === 1062 || $code === 1586)) { + // error 1062 is for a duplicate entry + // see https://dev.mysql.com/doc/mysql-errors/8.4/en/server-error-reference.html#error_er_dup_entry + // error 1586 is ALSO for a duplicate entry and uses the same error message + // see https://dev.mysql.com/doc/mysql-errors/8.4/en/server-error-reference.html#error_er_dup_entry_with_key_name + preg_match('/Duplicate entry \'(?P[^\']+)\' for key \'?(?P[^\']+)\'?/', $message, $matches); + // MySQL includes the table name in the key, but MariaDB doesn't. + $key = $matches['key']; + if (str_contains($key ?? '', '.')) { + $parts = explode('.', $key); + $key = array_pop($parts); + } + $this->duplicateEntryError($message, $key, $matches['val'], $sql, $parameters); + } else { + $this->databaseError($message, $errorLevel, $sql, $parameters); + } + } } diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 85245b155c2..2b8394615ee 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -24,6 +24,7 @@ use SilverStripe\Forms\SearchableDropdownField; use SilverStripe\i18n\i18n; use SilverStripe\i18n\i18nEntityProvider; +use SilverStripe\ORM\Connect\DuplicateEntryException; use SilverStripe\ORM\Connect\MySQLSchemaManager; use SilverStripe\ORM\FieldType\DBComposite; use SilverStripe\ORM\FieldType\DBDatetime; @@ -1642,13 +1643,17 @@ public function write($showDebug = false, $forceInsert = false, $forceWrite = fa } $this->record['LastEdited'] = $now; - // New records have their insert into the base data table done first, so that they can pass the - // generated primary key on to the rest of the manipulation - $baseTable = $this->baseTable(); - $this->writeBaseRecord($baseTable, $now); - - // Write the DB manipulation for all changed fields - $this->writeManipulation($baseTable, $now, $isNewRecord); + // Try write the changes - but throw a validation exception if we violate a unique index + try { + // New records have their insert into the base data table done first, so that they can pass the + // generated primary key on to the rest of the manipulation + $baseTable = $this->baseTable(); + $this->writeBaseRecord($baseTable, $now); + // Write the DB manipulation for all changed fields + $this->writeManipulation($baseTable, $now, $isNewRecord); + } catch (DuplicateEntryException $e) { + throw new ValidationException($this->buildValidationResultForDuplicateEntry($e)); + } // If there's any relations that couldn't be saved before, save them now (we have an ID here) $this->writeRelations(); @@ -4638,4 +4643,38 @@ public function findAllRelatedData(array $excludedClasses = []): SS_List $service = Injector::inst()->get(RelatedDataService::class); return $service->findAll($this, $excludedClasses); } + + private function buildValidationResultForDuplicateEntry(DuplicateEntryException $exception): ValidationResult + { + $key = $exception->getKeyName(); + $singleName = static::i18n_singular_name(); + $indexes = DataObject::getSchema()->databaseIndexes(static::class); + $columns = $indexes[$key]['columns'] ?? []; + $validationResult = ValidationResult::create(); + if (empty($columns)) { + $validationResult->addError(_t( + __CLASS__ . '.NO_DUPLICATE', + 'Cannot create duplicate {type}', + ['type' => $singleName] + )); + } elseif (count($columns) === 1) { + $duplicateField = $columns[0]; + $validationResult->addFieldError( + $duplicateField, + _t( + __CLASS__ . '.NO_DUPLICATE_SINGLE_FIELD', + 'Cannot create duplicate {type} with "{field}" set to "{value}"', + ['type' => $singleName, 'field' => $this->fieldLabel($duplicateField), 'value' => $exception->getDuplicatedValue()] + ) + ); + } else { + $duplicateFieldNames = array_map(fn ($column) => $this->fieldLabel($column), $columns); + $validationResult->addError(_t( + __CLASS__ . '.NO_DUPLICATE_MULTI_FIELD', + 'Cannot create duplicate {type} - at least one of the following fields need to be changed: {fields}', + ['type' => $singleName, 'fields' => implode(', ', $duplicateFieldNames)] + )); + } + return $validationResult; + } } diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index fec9aa4c164..191dc1a5f9d 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -68,6 +68,7 @@ class DataObjectTest extends SapphireTest DataObjectTest\OverriddenDataObject::class, DataObjectTest\InjectedDataObject::class, DataObjectTest\SettersAndGetters::class, + DataObjectTest\UniqueIndexObject::class, ]; protected function setUp(): void @@ -2782,4 +2783,61 @@ public function testGetDatabaseBackedField(string $fieldPath, $expected) $databaseBackedField = $method->invokeArgs($class, [$fieldPath]); $this->assertSame($expected, $databaseBackedField); } + + public function provideExceptionForUniqueIndexViolation() + { + return [ + 'violate SingleFieldIndex only' => [ + 'fieldsRecordOne' => [ + 'SingleField' => 'Same Value', + 'Name' => 'Value1', + 'Code' => 'Value1', + ], + 'fieldsRecordTwo' => [ + 'SingleField' => 'Same Value', + 'Name' => 'Value2', + 'Code' => 'Value2', + ], + 'expectedMessage' => 'Cannot create duplicate Unique Index Object with "Single field" set to "Same Value"', + ], + 'violate MultiFieldIndex only' => [ + 'fieldsRecordOne' => [ + 'SingleField' => 'Value1', + 'Name' => 'Name Value', + 'Code' => 'Code Value', + ], + 'fieldsRecordTwo' => [ + 'SingleField' => 'Value2', + 'Name' => 'Name Value', + 'Code' => 'Code Value', + ], + 'expectedMessage' => 'Cannot create duplicate Unique Index Object - at least one of the following fields need to be changed: Name, Code', + ], + 'violate both indexes' => [ + 'fieldsRecordOne' => [ + 'SingleField' => 'Same Value', + 'Name' => 'Name Value', + 'Code' => 'Code Value', + ], + 'fieldsRecordTwo' => [ + 'SingleField' => 'Same Value', + 'Name' => 'Name Value', + 'Code' => 'Code Value', + ], + 'expectedMessage' => 'Cannot create duplicate Unique Index Object with "Single field" set to "Same Value"', + ], + ]; + } + + /** + * @dataProvider provideExceptionForUniqueIndexViolation + */ + public function testExceptionForUniqueIndexViolation(array $fieldsRecordOne, array $fieldsRecordTwo, string $expectedMessage): void + { + DataObjectTest\UniqueIndexObject::create($fieldsRecordOne)->write(); + $record2 = DataObjectTest\UniqueIndexObject::create($fieldsRecordTwo); + $this->expectException(ValidationException::class); + $this->expectExceptionMessage($expectedMessage); + $record2->write(); + } } diff --git a/tests/php/ORM/DataObjectTest/UniqueIndexObject.php b/tests/php/ORM/DataObjectTest/UniqueIndexObject.php new file mode 100644 index 00000000000..e1b380b5a8e --- /dev/null +++ b/tests/php/ORM/DataObjectTest/UniqueIndexObject.php @@ -0,0 +1,33 @@ + 'Varchar', + 'Name' => 'Varchar', + 'Code' => 'Varchar', + ]; + + private static array $indexes = [ + 'SingleFieldIndex' => [ + 'type' => 'unique', + 'columns' => [ + 'SingleField', + ], + ], + 'MultiFieldIndex' => [ + 'type' => 'unique', + 'columns' => [ + 'Name', + 'Code', + ], + ], + ]; +} diff --git a/tests/php/ORM/MySQLiConnectorTest.php b/tests/php/ORM/MySQLiConnectorTest.php index 8da8b65dd1b..a982f37b457 100644 --- a/tests/php/ORM/MySQLiConnectorTest.php +++ b/tests/php/ORM/MySQLiConnectorTest.php @@ -6,6 +6,8 @@ use SilverStripe\Dev\SapphireTest; use SilverStripe\Dev\TestOnly; use SilverStripe\ORM\Connect\DatabaseException; +use SilverStripe\ORM\Connect\DuplicateEntryException; +use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DB; use SilverStripe\ORM\Tests\MySQLiConnectorTest\MySQLiConnector; use SilverStripe\Tests\ORM\Utf8\Utf8TestHelper; @@ -43,13 +45,43 @@ public function setUp(): void $config = DB::getConfig(); + // NOTE this also runs for MariaDB. if (strtolower(substr($config['type'] ?? '', 0, 5)) !== 'mysql') { $this->markTestSkipped("The test only relevant for MySQL - but $config[type] is in use"); } + // Build a table for the duplicate entry tests + DB::get_schema()->schemaUpdate(function () { + DB::quiet(true); + DB::require_table( + 'duplicate_entry_table', + [ + 'ID' => 'PrimaryKey', + 'Title' => 'Varchar', + 'Name' => 'Varchar', + ], + [ + 'MyIndex' => [ + 'type' => 'unique', + 'columns' => ['Title', 'Name'], + ], + ], + options: DataObject::config()->get('create_table_options') + ); + }); + $this->config = $config; } + protected function tearDown(): void + { + DB::get_schema()->schemaUpdate(function () { + DB::quiet(true); + DB::get_schema()->dontRequireTable('duplicate_entry_table'); + }); + parent::tearDown(); + } + /** * @dataProvider charsetProvider */ @@ -143,24 +175,91 @@ public function testUtf8mb4UnicodeCollation() $this->assertEquals('rst', $result[1][0]); } - public function testQueryThrowsDatabaseErrorOnMySQLiError() + public function provideQueryThrowsException() + { + return [ + [ + // Uses errors, not exceptions + 'reportMode' => MYSQLI_REPORT_OFF, + ], + [ + // Uses exceptions. This is the default since PHP 8.1 + // See https://www.php.net/manual/en/mysqli-driver.report-mode.php + 'reportMode' => MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT, + ], + ]; + } + + /** + * @dataProvider provideQueryThrowsException + */ + public function testQueryThrowsDatabaseError(int $reportMode): void { $connector = $this->getConnector(); $driver = new mysqli_driver(); // The default with PHP >= 8.0 - $driver->report_mode = MYSQLI_REPORT_OFF; - $this->expectException(DatabaseException::class); + $driver->report_mode = $reportMode; $connector = $this->getConnector(null, null, true); + $this->expectException(DatabaseException::class); $connector->query('force an error with invalid SQL'); } - public function testQueryThrowsDatabaseErrorOnMySQLiException() + /** + * @dataProvider provideQueryThrowsException + */ + public function testQueryThrowsDuplicateEntryException(int $reportMode): void + { + $connector = $this->getConnector(); + $driver = new mysqli_driver(); + $driver->report_mode = $reportMode; + $connector = DB::get_conn(); + // Create the first item + $connector->query('INSERT INTO duplicate_entry_table (Title, Name) VALUES (\'My Title\', \'My Name\');'); + $this->expectException(DuplicateEntryException::class); + // Create the duplicate item + $connector->query('INSERT INTO duplicate_entry_table (Title, Name) VALUES (\'My Title\', \'My Name\');'); + } + + /** + * @dataProvider provideQueryThrowsException + */ + public function testPreparedQueryThrowsDatabaseError(int $reportMode): void { + $connector = $this->getConnector(); $driver = new mysqli_driver(); - // The default since PHP 8.1 - https://www.php.net/manual/en/mysqli-driver.report-mode.php - $driver->report_mode = MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT; + $driver->report_mode = $reportMode; + $connector = $this->getConnector(null, null, true); $this->expectException(DatabaseException::class); + $connector->preparedQuery('force an error with invalid SQL', []); + } + + /** + * @dataProvider provideQueryThrowsException + */ + public function testPreparedQueryThrowsDuplicateEntryException(int $reportMode): void + { + $connector = $this->getConnector(); + $driver = new mysqli_driver(); + $driver->report_mode = $reportMode; + $connector = DB::get_conn(); + // Create the first item + $connector->preparedQuery('INSERT INTO duplicate_entry_table (Title, Name) VALUES (?, ?);', ['My Title', 'My Name']); + $this->expectException(DuplicateEntryException::class); + // Create the duplicate item + $connector->preparedQuery('INSERT INTO duplicate_entry_table (Title, Name) VALUES (?, ?);', ['My Title', 'My Name']); + } + + /** + * @dataProvider provideQueryThrowsException + */ + public function testPrepareStatementThrowsDatabaseError(int $reportMode): void + { + $connector = $this->getConnector(); + $driver = new mysqli_driver(); + $driver->report_mode = $reportMode; $connector = $this->getConnector(null, null, true); - $connector->query('force an error with invalid SQL'); + $success = false; + $this->expectException(DatabaseException::class); + $connector->prepareStatement('force an error with invalid SQL', $success); } } From 16ef0944df426e6df68f31ac6f147216a802a279 Mon Sep 17 00:00:00 2001 From: Thomas Portelange Date: Wed, 15 Jan 2025 22:41:33 +0100 Subject: [PATCH 2/3] FIX Class manifest is not caching enums (#11532) --- src/Core/Manifest/ClassManifest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Core/Manifest/ClassManifest.php b/src/Core/Manifest/ClassManifest.php index 0e2887635ab..e3898598c72 100644 --- a/src/Core/Manifest/ClassManifest.php +++ b/src/Core/Manifest/ClassManifest.php @@ -696,6 +696,7 @@ public function handleFile($basename, $pathname, $includeTests) 'classes' => $classes, 'interfaces' => $interfaces, 'traits' => $traits, + 'enums' => $enums, ]; $this->cache->set($key, $cache); } From 227e1788ba52de719b8cd6ab052fa04b1ada0da1 Mon Sep 17 00:00:00 2001 From: Thomas Portelange Date: Thu, 16 Jan 2025 20:43:37 +0100 Subject: [PATCH 3/3] FIX Less cache fragmentation in ClassManifest (#11533) --- src/Core/Manifest/ClassManifest.php | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/Core/Manifest/ClassManifest.php b/src/Core/Manifest/ClassManifest.php index e3898598c72..c1189f433d7 100644 --- a/src/Core/Manifest/ClassManifest.php +++ b/src/Core/Manifest/ClassManifest.php @@ -53,6 +53,16 @@ class ClassManifest */ protected $cacheKey; + /** + * In memory cache array for individually parsed files + */ + protected ?array $filesCache = null; + + /** + * Key to use for files cache + */ + protected string $filesCacheKey; + /** * Array of properties to cache * @@ -203,6 +213,7 @@ public function __construct($base, CacheFactory $cacheFactory = null) $this->base = $base; $this->cacheFactory = $cacheFactory; $this->cacheKey = 'manifest'; + $this->filesCacheKey = 'manifestFiles'; } private function buildCache($includeTests = false) @@ -563,6 +574,7 @@ public function regenerate($includeTests) if ($this->cache) { $data = $this->getState(); + $this->cache->set($this->filesCacheKey, $this->filesCache); $this->cache->set($this->cacheKey, $data); $this->cache->set('generated_at', time()); $this->cache->delete('regenerate'); @@ -587,11 +599,15 @@ public function handleFile($basename, $pathname, $includeTests) // since just using the datetime lead to problems with upgrading. $key = preg_replace('/[^a-zA-Z0-9_]/', '_', $basename ?? '') . '_' . md5_file($pathname ?? ''); + if ($this->cache && $this->filesCache === null) { + $this->filesCache = $this->cache->get($this->filesCacheKey); + } + // Attempt to load from cache // Note: $classes, $interfaces and $traits arrays have correct-case keys, not lowercase $changed = false; if ($this->cache - && ($data = $this->cache->get($key)) + && ($data = ($this->filesCache[$key] ?? null)) && $this->validateItemCache($data) ) { $classes = $data['classes']; @@ -698,7 +714,8 @@ public function handleFile($basename, $pathname, $includeTests) 'traits' => $traits, 'enums' => $enums, ]; - $this->cache->set($key, $cache); + + $this->filesCache[$key] = $cache; } }