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

long size varchar with long index size mysql #210

Closed
wants to merge 21 commits into from
Closed

long size varchar with long index size mysql #210

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
534fb6b
long size varchar with long index size mysql
fogelito Oct 20, 2022
6dfc741
mongo revert changes
fogelito Oct 20, 2022
fef0691
make pr more clear
fogelito Oct 23, 2022
fa88dfb
remove test comments
fogelito Oct 23, 2022
ffe17f8
some
fogelito Oct 23, 2022
ae14887
remove comment
fogelito Oct 23, 2022
8485a3c
check is collection exist
fogelito Oct 23, 2022
8eb5a44
put findAttributeInList in adapter
fogelito Oct 23, 2022
fda4df6
put findAttributeInList in Database.php
fogelito Oct 23, 2022
f77cbbd
back
fogelito Oct 23, 2022
c2a3354
changes
fogelito Oct 23, 2022
6a7e558
changing some logic
fogelito Oct 24, 2022
a3ad9ca
remove comments
fogelito Oct 24, 2022
577751d
remove comments
fogelito Oct 24, 2022
92054f4
update hints
fogelito Oct 24, 2022
c96d008
Text fix
fogelito Oct 24, 2022
f1ea80a
description
fogelito Oct 24, 2022
a25f7d6
some fixes
fogelito Oct 25, 2022
c58ab07
some fixes
fogelito Oct 25, 2022
44b4eff
some fixes
fogelito Oct 26, 2022
a65d9e0
some fixes
fogelito Oct 26, 2022
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
20 changes: 7 additions & 13 deletions composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
convertNoticesToExceptions="true"
convertWarningsToExceptions="true"
processIsolation="false"
stopOnFailure="false">
stopOnFailure="true">
<testsuites>
<testsuite name="Application Test Suite">
<directory>./tests/</directory>
Expand Down
40 changes: 32 additions & 8 deletions src/Database/Adapter/MariaDB.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,34 +148,48 @@ public function createCollection(string $name, array $attributes = [], array $in
$database = $this->getDefaultDatabase();
$namespace = $this->getNamespace();
$id = $this->filter($name);
$collectionAttributes = [];
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what this variable name should indicate. The story of this new logic is not very clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it for functions I think it is much clear now.


foreach ($attributes as $key => $attribute) {
$attrId = $this->filter($attribute->getId());
$attrType = $this->getSQLType($attribute->getAttribute('type'), $attribute->getAttribute('size', 0), $attribute->getAttribute('signed', true));
$size = $attribute->getAttribute('size', 0);
$type = $attribute->getAttribute('type');
$signed = $attribute->getAttribute('signed', false);
$attrType = $this->getSQLType($type, $size, $signed);

if ($attribute->getAttribute('array')) {
$attrType = 'LONGTEXT';
}

$attributes[$key] = "`{$attrId}` {$attrType}, ";
$collectionAttributes[$attrId] = $attribute;
}

foreach ($indexes as $key => $index) {
$indexId = $this->filter($index->getId());
$indexType = $index->getAttribute('type');

$indexAttributes = $index->getAttribute('attributes');

foreach ($indexAttributes as $nested => $attribute) {
$indexLength = $index->getAttribute('lengths')[$key] ?? '';
$indexLength = (empty($indexLength)) ? '' : '(' . (int)$indexLength . ')';
$indexOrder = $index->getAttribute('orders')[$key] ?? '';
$indexAttribute = $this->filter($attribute);

//$collectionAttributes[$indexAttribute]['size'] = $collectionAttributes[$indexAttribute]['size'] ?? 0;
$size = isset($index['lengths'][$key]) ? $index['lengths'][$key]:0 ;

if($collectionAttributes[$indexAttribute]['type'] === Database::VAR_STRING){
if($collectionAttributes[$indexAttribute]['size'] > 700){
$size = $size === 0 || $size > 700 ? 700 : $size;
}
}

$length = $size === 0 ? '' : '(' . $size . ')';
$indexOrder = $index->getAttribute('orders')[$key] ?? '';

if ($indexType === Database::INDEX_FULLTEXT) {
$indexOrder = '';
}

$indexAttributes[$nested] = "`{$indexAttribute}`{$indexLength} {$indexOrder}";
$indexAttributes[$nested] = "`{$indexAttribute}`{$length} {$indexOrder}";
}

$indexes[$key] = "{$indexType} `{$indexId}` (" . \implode(", ", $indexAttributes) . " ),";
Expand Down Expand Up @@ -368,7 +382,6 @@ public function deleteAttribute(string $collection, string $id, bool $array = fa
* @param array $orders
* @return bool
* @throws Exception
* @throws PDOException
*/
public function createIndex(string $collection, string $id, string $type, array $attributes, array $lengths, array $orders): bool
{
Expand All @@ -383,6 +396,18 @@ public function createIndex(string $collection, string $id, string $type, array
}, $attributes);

foreach ($attributes as $key => $attribute) {

// $size = (int)($lengths[$key] ?? 0);
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need this part?
Should we throw an exception when index size is not set properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the Index size, not the attribute size, it is not mandatory.
I commend out this part since I have questions. This is another place we have to fix regardless of the current bug.

// foreach ($collectionAttributes as $ca){
// if($attribute === $ca['key']){
// if($ca['type'] == Database::VAR_STRING){
// if($ca['size'] > 700){
// $size = $size === 0 || $size > 700 ? 700 : $size;
// }
// }
// }
// }

$length = $lengths[$key] ?? '';
$length = (empty($length)) ? '' : '(' . (int)$length . ')';
$order = $orders[$key] ?? '';
Expand Down Expand Up @@ -1790,7 +1815,6 @@ protected function getSQLIndex(string $collection, string $id, string $type, ar

default:
throw new Exception('Unknown Index Type:' . $type);
break;
}

return "CREATE {$type} `{$id}` ON {$this->getSQLTable($collection)} ( " . implode(', ', $attributes) . " )";
Expand Down
2 changes: 1 addition & 1 deletion src/Database/Adapter/SQLite.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,10 +254,10 @@ public function renameIndex(string $collection, string $old, string $new): bool
* @param array $orders
* @return bool
* @throws Exception
* @throws PDOException
*/
public function createIndex(string $collection, string $id, string $type, array $attributes, array $lengths, array $orders): bool
{
// $name = $this->filter($collection->getId());
$name = $this->filter($collection);
$id = $this->filter($id);

Expand Down
7 changes: 5 additions & 2 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1176,6 +1176,7 @@ public function renameIndex(string $collection, string $old, string $new): bool
* @param array $orders
*
* @return bool
* @throws Exception
*/
public function createIndex(string $collection, string $id, string $type, array $attributes, array $lengths = [], array $orders = []): bool
{
Expand All @@ -1184,7 +1185,10 @@ public function createIndex(string $collection, string $id, string $type, array
}

$collection = $this->silent(fn() => $this->getCollection($collection));

if($collection->isEmpty()){
throw new Exception('Not found');
Copy link
Member

Choose a reason for hiding this comment

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

Are we missing this check from other methods? We should make sure we keep a consistent behavior across the library. Please add this check in other places when absolutely needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I added some more checks like this where the collection was called.

Copy link
Member

Choose a reason for hiding this comment

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

Not found is very generic and not helpful at all for other devs who might be using the library. Our errors should be as verbose, useful, and consistent as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to
throw new Exception('Collection ' .$collectionName . ' Not found');
I added In a few more places we are called the collection object.

}

// index IDs are case insensitive
$indexes = $collection->getAttribute('indexes', []);
/** @var Document[] $indexes */
Expand Down Expand Up @@ -1219,7 +1223,6 @@ public function createIndex(string $collection, string $id, string $type, array

default:
throw new Exception('Unknown index type: ' . $type);
break;
}

$index = $this->adapter->createIndex($collection->getId(), $id, $type, $attributes, $lengths, $orders);
Expand Down
Binary file modified tests/Database/Adapter/database.sql
View file
Open in desktop
Binary file not shown.
8 changes: 6 additions & 2 deletions tests/Database/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ public function testCreateDeleteIndex()
$this->assertEquals(true, static::getDatabase()->createAttribute('indexes', 'integer', Database::VAR_INTEGER, 0, true));
$this->assertEquals(true, static::getDatabase()->createAttribute('indexes', 'float', Database::VAR_FLOAT, 0, true));
$this->assertEquals(true, static::getDatabase()->createAttribute('indexes', 'boolean', Database::VAR_BOOLEAN, 0, true));
$this->assertEquals(true, static::getDatabase()->createAttribute('indexes', 'long_varchar', Database::VAR_STRING, 2048, true));

// Indexes
$this->assertEquals(true, static::getDatabase()->createIndex('indexes', 'index1', Database::INDEX_KEY, ['string', 'integer'], [128], [Database::ORDER_ASC]));
Expand All @@ -291,6 +292,8 @@ public function testCreateDeleteIndex()
$this->assertEquals(true, static::getDatabase()->createIndex('indexes', 'index4', Database::INDEX_UNIQUE, ['string'], [128], [Database::ORDER_ASC]));
$this->assertEquals(true, static::getDatabase()->createIndex('indexes', 'index5', Database::INDEX_UNIQUE, ['$id', 'string'], [128], [Database::ORDER_ASC]));
$this->assertEquals(true, static::getDatabase()->createIndex('indexes', 'order', Database::INDEX_UNIQUE, ['order'], [128], [Database::ORDER_ASC]));
//$this->assertEquals(true, static::getDatabase()->createIndex('indexes', 'long_varchar', Database::INDEX_UNIQUE, ['long_varchar'], [], [Database::ORDER_ASC]));


$collection = static::getDatabase()->getCollection('indexes');
$this->assertCount(6, $collection->getAttribute('indexes'));
Expand All @@ -302,6 +305,7 @@ public function testCreateDeleteIndex()
$this->assertEquals(true, static::getDatabase()->deleteIndex('indexes', 'index4'));
$this->assertEquals(true, static::getDatabase()->deleteIndex('indexes', 'index5'));
$this->assertEquals(true, static::getDatabase()->deleteIndex('indexes', 'order'));
//$this->assertEquals(true, static::getDatabase()->deleteIndex('indexes', 'long_varchar'));

$collection = static::getDatabase()->getCollection('indexes');
$this->assertCount(0, $collection->getAttribute('indexes'));
Expand All @@ -315,7 +319,7 @@ public function testCreateCollectionWithSchema()
new Document([
'$id' => ID::custom('attribute1'),
'type' => Database::VAR_STRING,
'size' => 256,
'size' => 2500,
'required' => false,
'signed' => true,
'array' => false,
Expand Down Expand Up @@ -346,7 +350,7 @@ public function testCreateCollectionWithSchema()
'$id' => ID::custom('index1'),
'type' => Database::INDEX_KEY,
'attributes' => ['attribute1'],
'lengths' => [256],
'lengths' => [],
'orders' => ['ASC'],
]),
new Document([
Expand Down