Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW Provide clear UX validation errors when violating unique index #11558

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/ORM/Connect/DBConnector.php
Original file line number Diff line number Diff line change
Expand Up @@ -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}";
}
Expand All @@ -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)
*
Expand Down
50 changes: 50 additions & 0 deletions src/ORM/Connect/DuplicateEntryException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<?php

namespace SilverStripe\ORM\Connect;

/**
* Exception for errors related to duplicate entries (e.g. violating a unique index)
*/
class DuplicateEntryException extends DatabaseException
{
private ?string $keyName = null;

private ?string $duplicatedValue = null;

/**
* Constructs the database exception
*
* @param string $message The Exception message to throw.
* @param string|null $keyName The name of the key which the error is for (e.g. index name)
* @param string|null $duplicatedValue The value which was duplicated (e.g. combined value of multiple columns in index)
* @param string|null $sql The SQL executed for this query
* @param array $parameters The parameters given for this query, if any
*/
public function __construct(
string $message = '',
?string $keyName = null,
?string $duplicatedValue = null,
?string $sql = null,
array $parameters = []
) {
parent::__construct($message, sql: $sql, parameters: $parameters);
$this->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;
}
}
55 changes: 49 additions & 6 deletions src/ORM/Connect/MySQLiConnector.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace SilverStripe\ORM\Connect;

use Exception;
use mysqli;
use mysqli_sql_exception;
use mysqli_stmt;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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<val>[^\']+)\' for key \'?(?P<key>[^\']+)\'?/', $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);
}
}
}
53 changes: 46 additions & 7 deletions src/ORM/DataObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
}
58 changes: 58 additions & 0 deletions tests/php/ORM/DataObjectTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class DataObjectTest extends SapphireTest
DataObjectTest\OverriddenDataObject::class,
DataObjectTest\InjectedDataObject::class,
DataObjectTest\SettersAndGetters::class,
DataObjectTest\UniqueIndexObject::class,
];

protected function setUp(): void
Expand Down Expand Up @@ -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();
}
}
33 changes: 33 additions & 0 deletions tests/php/ORM/DataObjectTest/UniqueIndexObject.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

namespace SilverStripe\ORM\Tests\DataObjectTest;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;

class UniqueIndexObject extends DataObject implements TestOnly
{
private static string $table_name = 'DataObjectTest_UniqueIndexObject';

private static array $db = [
'SingleField' => 'Varchar',
'Name' => 'Varchar',
'Code' => 'Varchar',
];

private static array $indexes = [
'SingleFieldIndex' => [
'type' => 'unique',
'columns' => [
'SingleField',
],
],
'MultiFieldIndex' => [
'type' => 'unique',
'columns' => [
'Name',
'Code',
],
],
];
}
Loading
Loading