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.

Sign up for GitHub

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

Don't throw on duplicate column for shared tables #435

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Don't throw on duplicate column for shared tables #435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions src/Database/Adapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

use Exception;
use Utopia\Database\Exception as DatabaseException;
use Utopia\Database\Exception\Duplicate as DuplicateException;
use Utopia\Database\Exception\Timeout as TimeoutException;

abstract class Adapter
{
Expand Down Expand Up @@ -337,6 +339,8 @@ abstract public function deleteCollection(string $id): bool;
* @param bool $signed
* @param bool $array
* @return bool
* @throws TimeoutException
* @throws DuplicateException
*/
abstract public function createAttribute(string $collection, string $id, string $type, int $size, bool $signed = true, bool $array = false): bool;

Expand Down Expand Up @@ -600,6 +604,13 @@ abstract public function getLimitForIndexes(): int;
*/
abstract public function getSupportForSchemas(): bool;

/**
* Are attributes supported?
*
* @return bool
*/
abstract public function getSupportForAttributes(): bool;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required for Mongo


/**
* Is index supported?
*
Expand Down
28 changes: 21 additions & 7 deletions src/Database/Adapter/MariaDB.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,14 @@ public function createAttribute(string $collection, string $id, string $type, in
$sql = "ALTER TABLE {$this->getSQLTable($name)} ADD COLUMN `{$id}` {$type};";
$sql = $this->trigger(Database::EVENT_ATTRIBUTE_CREATE, $sql);

return $this->getPDO()
->prepare($sql)
->execute();
try {
return $this->getPDO()
->prepare($sql)
->execute();
} catch (PDOException $e) {
$this->processException($e);
return false;
}
}

/**
Expand Down Expand Up @@ -2246,17 +2251,26 @@ public function setTimeout(int $milliseconds, string $event = Database::EVENT_AL
/**
* @param PDOException $e
* @throws TimeoutException
* @throws DuplicateException
*/
protected function processException(PDOException $e): void
{
// Regular PDO
/**
* PDO and Swoole PDOProxy swap error codes and errorInfo
*/

// Timeout
if ($e->getCode() === '70100' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1969) {
throw new TimeoutException($e->getMessage(), $e->getCode(), $e);
} elseif ($e->getCode() === 1969 && isset($e->errorInfo[0]) && $e->errorInfo[0] === '70100') {
throw new TimeoutException($e->getMessage(), $e->getCode(), $e);
}

// PDOProxy switches errorInfo PDOProxy.php line 64
if ($e->getCode() === 1969 && isset($e->errorInfo[0]) && $e->errorInfo[0] === '70100') {
throw new TimeoutException($e->getMessage(), $e->getCode(), $e);
// Duplicate column
if ($e->getCode() === '42S21' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1060) {
throw new DuplicateException($e->getMessage(), $e->getCode(), $e);
} elseif ($e->getCode() === 1060 && isset($e->errorInfo[0]) && $e->errorInfo[0] === '42S21') {
throw new DuplicateException($e->getMessage(), $e->getCode(), $e);
}

throw $e;
Expand Down
12 changes: 11 additions & 1 deletion src/Database/Adapter/Mongo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ public function createCollection(string $name, array $attributes = [], array $in
try {
$this->getClient()->createCollection($id);
} catch (MongoException $e) {
throw new DatabaseException($e->getMessage(), $e->getCode(), $e);
throw new Duplicate($e->getMessage(), $e->getCode(), $e);
}

$indexesCreated = $this->client->createIndexes($id, [[
Expand Down Expand Up @@ -1561,6 +1561,16 @@ public function getSupportForSchemas(): bool
return true;
}

/**
* Are attributes supported?
*
* @return bool
*/
public function getSupportForAttributes(): bool
{
return false;
}

/**
* Is index supported?
*
Expand Down
15 changes: 12 additions & 3 deletions src/Database/Adapter/MySQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PDOException;
use Utopia\Database\Database;
use Utopia\Database\Exception as DatabaseException;
use Utopia\Database\Exception\Duplicate as DuplicateException;
use Utopia\Database\Exception\Timeout as TimeoutException;

class MySQL extends MariaDB
Expand Down Expand Up @@ -37,16 +38,24 @@ public function setTimeout(int $milliseconds, string $event = Database::EVENT_AL
/**
* @param PDOException $e
* @throws TimeoutException
* @throws DuplicateException
*/
protected function processException(PDOException $e): void
{
/**
* PDO and Swoole PDOProxy swap error codes and errorInfo
*/

if ($e->getCode() === 'HY000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 3024) {
throw new TimeoutException($e->getMessage(), $e->getCode(), $e);
} elseif ($e->getCode() === 3024 && isset($e->errorInfo[0]) && $e->errorInfo[0] === "HY000") {
throw new TimeoutException($e->getMessage(), $e->getCode(), $e);
}

// PDOProxy which who switches errorInfo
if ($e->getCode() === 3024 && isset($e->errorInfo[0]) && $e->errorInfo[0] === "HY000") {
throw new TimeoutException($e->getMessage(), $e->getCode(), $e);
if ($e->getCode() === '42S21' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1060) {
throw new DuplicateException($e->getMessage(), $e->getCode(), $e);
} elseif ($e->getCode() === 1060 && isset($e->errorInfo[0]) && $e->errorInfo[0] === '42S21') {
throw new DuplicateException($e->getMessage(), $e->getCode(), $e);
Comment on lines +55 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future, lets have a better way to identify the exception

}

throw $e;
Expand Down
26 changes: 19 additions & 7 deletions src/Database/Adapter/Postgres.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,14 @@ public function createAttribute(string $collection, string $id, string $type, in

$sql = $this->trigger(Database::EVENT_ATTRIBUTE_CREATE, $sql);

return $this->getPDO()
->prepare($sql)
->execute();
try {
return $this->getPDO()
->prepare($sql)
->execute();
} catch (PDOException $e) {
$this->processException($e);
return false;
}
}

/**
Expand Down Expand Up @@ -2197,17 +2202,24 @@ public function setTimeout(int $milliseconds, string $event = Database::EVENT_AL
/**
* @param PDOException $e
* @throws Timeout
* @throws Duplicate
*/
protected function processException(PDOException $e): void
{
// Regular PDO
/**
* PDO and Swoole PDOProxy swap error codes and errorInfo
*/

if ($e->getCode() === '57014' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 7) {
throw new Timeout($e->getMessage(), $e->getCode(), $e);
} elseif ($e->getCode() === 7 && isset($e->errorInfo[0]) && $e->errorInfo[0] === '57014') {
throw new Timeout($e->getMessage(), $e->getCode(), $e);
}

// PDOProxy switches errorInfo PDOProxy.php line 64
if ($e->getCode() === 7 && isset($e->errorInfo[0]) && $e->errorInfo[0] === '57014') {
throw new Timeout($e->getMessage(), $e->getCode(), $e);
if ($e->getCode() === '42701' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 7) {
throw new Duplicate($e->getMessage(), $e->getCode(), $e);
} elseif ($e->getCode() === 7 && isset($e->errorInfo[0]) && $e->errorInfo[0] === '42701') {
throw new Duplicate($e->getMessage(), $e->getCode(), $e);
}

throw $e;
Expand Down
10 changes: 10 additions & 0 deletions src/Database/Adapter/SQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,16 @@ public function getSupportForIndex(): bool
return true;
}

/**
* Are attributes supported?
*
* @return bool
*/
public function getSupportForAttributes(): bool
{
return true;
}

/**
* Is unique index supported?
*
Expand Down
28 changes: 28 additions & 0 deletions src/Database/Adapter/SQLite.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
use Utopia\Database\Document;
use Utopia\Database\Exception as DatabaseException;
use Utopia\Database\Exception\Duplicate;
use Utopia\Database\Exception\Duplicate as DuplicateException;
use Utopia\Database\Exception\Timeout as TimeoutException;
use Utopia\Database\Helpers\ID;

/**
Expand Down Expand Up @@ -1380,4 +1382,30 @@ public function getKeywords(): array
'WITHOUT',
];
}

/**
* @param PDOException $e
* @throws TimeoutException
* @throws DuplicateException
*/
protected function processException(PDOException $e): void
{
/**
* PDO and Swoole PDOProxy swap error codes and errorInfo
*/

if ($e->getCode() === 'HY000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 3024) {
throw new TimeoutException($e->getMessage(), $e->getCode(), $e);
} elseif ($e->getCode() === 3024 && isset($e->errorInfo[0]) && $e->errorInfo[0] === "HY000") {
throw new TimeoutException($e->getMessage(), $e->getCode(), $e);
}

if ($e->getCode() === 'HY000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1) {
throw new DuplicateException($e->getMessage(), $e->getCode(), $e);
} elseif ($e->getCode() === 1 && isset($e->errorInfo[0]) && $e->errorInfo[0] === 'HY000') {
throw new DuplicateException($e->getMessage(), $e->getCode(), $e);
}

throw $e;
}
}
15 changes: 11 additions & 4 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,7 +1269,7 @@ public function createAttribute(string $collection, string $id, string $type, in
throw new DatabaseException('Collection not found');
}

// attribute IDs are case insensitive
// Attribute IDs are case insensitive
$attributes = $collection->getAttribute('attributes', []);
/** @var array<Document> $attributes */
foreach ($attributes as $attribute) {
Expand Down Expand Up @@ -1351,10 +1351,17 @@ public function createAttribute(string $collection, string $id, string $type, in
$this->validateDefaultTypes($type, $default);
}

$created = $this->adapter->createAttribute($collection->getId(), $id, $type, $size, $signed, $array);
try {
$created = $this->adapter->createAttribute($collection->getId(), $id, $type, $size, $signed, $array);

if (!$created) {
throw new DatabaseException('Failed to create attribute');
if (!$created) {
throw new DatabaseException('Failed to create attribute');
}
} catch (DuplicateException $e) {
// HACK: Metadata should still be updated, can be removed when null tenant collections are supported.
if (!$this->adapter->getSharedTables()) {
throw $e;
}
}

if ($collection->getId() !== self::METADATA) {
Expand Down
57 changes: 54 additions & 3 deletions tests/e2e/Adapter/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1220,8 +1220,14 @@ public function testCreateDeleteAttribute(): void
$this->assertEquals(true, static::getDatabase()->createAttribute('attributes', 'socialAccountForYoutubeSubscribersss', Database::VAR_BOOLEAN, 0, true));
$this->assertEquals(true, static::getDatabase()->createAttribute('attributes', '5f058a89258075f058a89258075f058t9214', Database::VAR_BOOLEAN, 0, true));

// Using this collection to test invalid default values
// static::getDatabase()->deleteCollection('attributes');
// Test shared tables duplicates throw duplicate
static::getDatabase()->createAttribute('attributes', 'duplicate', Database::VAR_STRING, 128, true);
try {
static::getDatabase()->createAttribute('attributes', 'duplicate', Database::VAR_STRING, 128, true);
$this->fail('Failed to throw exception');
} catch (Exception $e) {
$this->assertInstanceOf(DuplicateException::class, $e);
}
}

/**
Expand Down Expand Up @@ -15011,7 +15017,7 @@ public function testEmptyOperatorValues(): void
* @throws StructureException
* @throws TimeoutException
*/
public function testIsolationModes(): void
public function testSharedTables(): void
{
/**
* Default mode already tested, we'll test 'schema' and 'table' isolation here
Expand Down Expand Up @@ -15190,6 +15196,51 @@ public function testIsolationModes(): void
$database->setDatabase($this->testDatabase);
}

public function testSharedTablesDuplicateAttributesDontThrow(): void
{
$database = static::getDatabase();

if (!$database->getAdapter()->getSupportForAttributes()) {
$this->expectNotToPerformAssertions();
return;
}

if ($database->exists('sharedTables')) {
$database->setDatabase('sharedTables')->delete();
}

$database
->setDatabase('sharedTables')
->setNamespace('')
->setSharedTables(true)
->setTenant(1)
->create();

// Create collection
$database->createCollection('duplicates', documentSecurity: false);
$database->createAttribute('duplicates', 'name', Database::VAR_STRING, 10, false);

$database->setTenant(2);
try {
$database->createCollection('duplicates', documentSecurity: false);
} catch (DuplicateException) {
// Ignore
}
$database->createAttribute('duplicates', 'name', Database::VAR_STRING, 10, false);

$collection = $database->getCollection('duplicates');
$this->assertEquals(1, \count($collection->getAttribute('attributes')));

$database->setTenant(1);

$collection = $database->getCollection('duplicates');
$this->assertEquals(1, \count($collection->getAttribute('attributes')));

$database->setSharedTables(false);
$database->setNamespace(static::$namespace);
$database->setDatabase($this->testDatabase);
}

public function testTransformations(): void
{
static::getDatabase()->createCollection('docs', attributes: [
Expand Down
Loading