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

Feat 5376 improve the select query #287

Closed
wants to merge 20 commits into from
Closed

Feat 5376 improve the select query #287

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0777736
fixed query and attribute types
faisalill Jun 28, 2023
1ca3476
Merge pull request #3 from faisalill/work-feat-5376
faisalill Jun 28, 2023
fedafbf
cleaned composer.json
faisalill Jun 28, 2023
0278adf
clean code
faisalill Jun 28, 2023
cde877c
changed few assertions for testFindSelect
faisalill Jun 28, 2023
b93544f
return by default
faisalill Jul 1, 2023
3a3b51f
permissions can be queried now
faisalill Jul 8, 2023
e39efe7
added assertions to test that other default keys are not returned
faisalill Jul 9, 2023
60bae57
fixed errors from testFindSelect
faisalill Jul 9, 2023
c4c6e2a
removed mytest from composer.json
faisalill Jul 9, 2023
f901305
added more assertions
faisalill Jul 9, 2023
e61eb13
fixed mongodb and postgres issue
faisalill Jul 12, 2023
0313f8c
fixed whitespace related issue
faisalill Jul 12, 2023
5fd6373
fixed lint issue
faisalill Jul 12, 2023
27364ea
Merge branch 'utopia-php:main' into feat-5376-improve-the-select-query
faisalill Jul 18, 2023
85afec2
Made changes as per review
faisalill Jul 20, 2023
4d20bc3
Merge branch 'utopia-php:main' into feat-5376-improve-the-select-query
faisalill Jul 22, 2023
a2d25c4
Fix code as per review
faisalill Jul 24, 2023
e480ad8
Make changes as per review
faisalill Jul 26, 2023
56dedb3
Merge branch 'utopia-php:main' into feat-5376-improve-the-select-query
faisalill Jul 27, 2023
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
59 changes: 45 additions & 14 deletions src/Database/Adapter/MariaDB.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,17 +1046,26 @@ public function find(string $collection, array $queries = [], ?int $limit = 25,
$results = $stmt->fetchAll();

foreach ($results as $key => $value) {
$results[$key]['$id'] = $value['_uid'];
$results[$key]['$internalId'] = $value['_id'];
$results[$key]['$createdAt'] = $value['_createdAt'];
$results[$key]['$updatedAt'] = $value['_updatedAt'];
$results[$key]['$permissions'] = json_decode($value['_permissions'] ?? '[]', true);

unset($results[$key]['_uid']);
unset($results[$key]['_id']);
unset($results[$key]['_createdAt']);
unset($results[$key]['_updatedAt']);
unset($results[$key]['_permissions']);
if (array_key_exists('_uid', $value)) {
$results[$key]['$id'] = $value['_uid'];
unset($results[$key]['_uid']);
}
if (array_key_exists('_id', $value)) {
$results[$key]['$internalId'] = $value['_id'];
unset($results[$key]['_id']);
}
if (array_key_exists('_createdAt', $value)) {
$results[$key]['$createdAt'] = $value['_createdAt'];
unset($results[$key]['_createdAt']);
}
if (array_key_exists('_updatedAt', $value)) {
$results[$key]['$updatedAt'] = $value['_updatedAt'];
unset($results[$key]['_updatedAt']);
}
if (array_key_exists('_permissions', $value)) {
$results[$key]['$permissions'] = json_decode($value['_permissions'] ?? '[]', true);
unset($results[$key]['_permissions']);
}

$results[$key] = new Document($results[$key]);
}
Expand Down Expand Up @@ -1189,10 +1198,32 @@ protected function getAttributeProjection(array $selections, string $prefix = ''
return '*';
}

if (in_array('$id', $selections)) {
$index = array_search('$id', $selections);
$selections[$index] = '_uid';
faisalill marked this conversation as resolved.
Show resolved Hide resolved
}
if (in_array('$internalId', $selections)) {
$index = array_search('$internalId', $selections);
$selections[$index] = '_id';
}
if (in_array('$createdAt', $selections)) {
$index = array_search('$createdAt', $selections);
$selections[$index] = '_createdAt';
}
if (in_array('$updatedAt', $selections)) {
$index = array_search('$updatedAt', $selections);
$selections[$index] = '_updatedAt';
}
if (in_array('$permissions', $selections)) {
faisalill marked this conversation as resolved.
Show resolved Hide resolved
$index = array_search('$permissions', $selections);
$selections[$index] = '_permissions';
}
if (in_array('$collection', $selections)) {
$index = array_search('$collection', $selections);
unset($selections[$index]);
}

$selections[] = '_uid';
$selections[] = '_id';
$selections[] = '_createdAt';
$selections[] = '_updatedAt';
$selections[] = '_permissions';

if (!empty($prefix)) {
Expand Down
20 changes: 20 additions & 0 deletions src/Database/Adapter/Mongo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1335,6 +1335,26 @@ protected function getAttributeProjection(array $selections, string $prefix = ''
$projection['_createdAt'] = 1;
$projection['_updatedAt'] = 1;
Comment on lines 1335 to 1336
Copy link
Member

Choose a reason for hiding this comment

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

We need to make sure these are not set by default the same as maraidb

Copy link
Contributor Author

@faisalill faisalill Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Except _collection and _id rest have to be set because.

Copy link
Member

Choose a reason for hiding this comment

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

Can we make it so that they are not required at this point? I think we should be able to if we can do it for MariaDB

Copy link
Contributor Author

@faisalill faisalill Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For all adapters _uid and _permissions are a must.

MariaDB/MySQL/SQLite: https://github.com/faisalill/database/blob/feat-5376-improve-the-select-query/src/Database/Adapter/MariaDB.php#L1226
Postgres: https://github.com/faisalill/database/blob/feat-5376-improve-the-select-query/src/Database/Adapter/Postgres.php#L1211

Because of this:

Copy link
Contributor Author

@faisalill faisalill Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For Postgres I am querying all the internal attributes and if any internal attribute is in the query I am removing it from selection then before the document is returned I check which internal attributes were in the query and only keep those internal attributes in the document.
But for all the adapters internal attributes won't be returned. Unless queried specificly.
https://github.com/faisalill/database/blob/feat-5376-improve-the-select-query/tests/Database/Base.php#L1042
All these tests work fine.
It's a mess ik.
I will close this PR start from scratch and update u on the same.

$projection['_permissions'] = 1;
$projection['_collection'] = 1;

if (isset($projection['$id'])) {
unset($projection['$id']);
}
if (isset($projection['$internalId'])) {
unset($projection['$internalId']);
}
if (isset($projection['$permissions'])) {
unset($projection['$permissions']);
}
if (isset($projection['$createdAt'])) {
unset($projection['$createdAt']);
}
if (isset($projection['$updatedAt'])) {
unset($projection['$updatedAt']);
}
if (isset($projection['$collection'])) {
unset($projection['$collection']);
}

return $projection;
}
Expand Down
42 changes: 31 additions & 11 deletions src/Database/Adapter/Postgres.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,17 +1056,26 @@ public function find(string $collection, array $queries = [], ?int $limit = 25,
$results = $stmt->fetchAll();

foreach ($results as $key => $value) {
$results[$key]['$id'] = $value['_uid'];
$results[$key]['$internalId'] = $value['_id'];
$results[$key]['$createdAt'] = $value['_createdAt'];
$results[$key]['$updatedAt'] = $value['_updatedAt'];
$results[$key]['$permissions'] = json_decode($value['_permissions'] ?? '[]', true);

unset($results[$key]['_uid']);
unset($results[$key]['_id']);
unset($results[$key]['_createdAt']);
unset($results[$key]['_updatedAt']);
unset($results[$key]['_permissions']);
if (array_key_exists('_uid', $value)) {
$results[$key]['$id'] = $value['_uid'];
unset($results[$key]['_uid']);
}
if (array_key_exists('_id', $value)) {
$results[$key]['$internalId'] = $value['_id'];
unset($results[$key]['_id']);
}
if (array_key_exists('_createdAt', $value)) {
$results[$key]['$createdAt'] = $value['_createdAt'];
unset($results[$key]['_createdAt']);
}
if (array_key_exists('_updatedAt', $value)) {
$results[$key]['$updatedAt'] = $value['_updatedAt'];
unset($results[$key]['_updatedAt']);
}
if (array_key_exists('_permissions', $value)) {
$results[$key]['$permissions'] = json_decode($value['_permissions'] ?? '[]', true);
unset($results[$key]['_permissions']);
}

$results[$key] = new Document($results[$key]);
}
Expand Down Expand Up @@ -1215,6 +1224,17 @@ protected function getAttributeProjection(array $selections, string $prefix = ''
}
}

$parameterNames = \array_map(function ($value) {
return \ltrim($value, '$');
}, Database::INTERNAL_PARAMETERS);

foreach ($selections as $key => $value) {
$valueUnquoted = \trim(\stripslashes($value), "\"");
if (\in_array($valueUnquoted, $parameterNames)) {
unset($selections[$key]);
}
}
abnegate marked this conversation as resolved.
Show resolved Hide resolved

return \implode(', ', $selections);
}

Expand Down
31 changes: 20 additions & 11 deletions src/Database/Adapter/SQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,17 +127,26 @@ public function getDocument(string $collection, string $id, array $queries = [])
return new Document([]);
}

$document['$id'] = $document['_uid'];
$document['$internalId'] = $document['_id'];
$document['$createdAt'] = $document['_createdAt'];
$document['$updatedAt'] = $document['_updatedAt'];
$document['$permissions'] = json_decode($document['_permissions'] ?? '[]', true);

unset($document['_id']);
unset($document['_uid']);
unset($document['_createdAt']);
unset($document['_updatedAt']);
unset($document['_permissions']);
if (isset($document['_uid'])) {
$document['$id'] = $document['_uid'];
unset($document['_uid']);
}
if (isset($document['_id'])) {
$document['$internalId'] = $document['_id'];
unset($document['_id']);
}
if (isset($document['_createdAt'])) {
$document['$createdAt'] = $document['_createdAt'];
unset($document['_createdAt']);
}
if (isset($document['_updatedAt'])) {
$document['$updatedAt'] = $document['_updatedAt'];
unset($document['_updatedAt']);
}
if (isset($document['_permissions'])) {
$document['$permissions'] = json_decode($document['_permissions'] ?? '[]', true);
unset($document['_permissions']);
}

return new Document($document);
}
Expand Down
52 changes: 50 additions & 2 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,16 @@ class Database
public const EVENT_INDEX_CREATE = 'index_create';
public const EVENT_INDEX_DELETE = 'index_delete';

// Internal Parameters
public const INTERNAL_PARAMETERS = [
faisalill marked this conversation as resolved.
Show resolved Hide resolved
'$id',
'$internalId',
'$createdAt',
'$updatedAt',
'$permissions',
'$collection'
];

protected Adapter $adapter;

protected Cache $cache;
Expand Down Expand Up @@ -2286,6 +2296,26 @@ public function getDocument(string $collection, string $id, array $queries = [])

$this->trigger(self::EVENT_DOCUMENT_READ, $document);

foreach ($queries as $query) {
if ($query->getMethod() == Query::TYPE_SELECT) {
$queriedValues = $query->getValues();
$defaultKeys = Database::INTERNAL_PARAMETERS;

foreach ($queriedValues as $queriedValue) {
if (in_array($queriedValue, $defaultKeys)) {
$index = array_search($queriedValue, $defaultKeys);
unset($defaultKeys[$index]);
}
}

foreach ($defaultKeys as $defaultKey) {
if ($document->isSet($defaultKey)) {
$document->removeAttribute($defaultKey);
}
}
}
}

faisalill marked this conversation as resolved.
Show resolved Hide resolved
return $document;
}

Expand Down Expand Up @@ -4001,6 +4031,18 @@ public function find(string $collection, array $queries = [], ?int $timeout = nu

$this->trigger(self::EVENT_DOCUMENT_FIND, $results);

// remove default keys $id and $permissions from the results

foreach ($queries as $query) {
if ($query->getMethod() === Query::TYPE_SELECT) {
foreach ($results as $result) {
faisalill marked this conversation as resolved.
Show resolved Hide resolved
foreach (Database::INTERNAL_PARAMETERS as $parameter) {
Copy link
Member

Choose a reason for hiding this comment

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

This will remove internal attributes even if they were selected, we need to make sure they are only removed when not selected

Let's add a test to cover this case as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/faisalill/database/blob/feat-5376-improve-the-select-query/tests/Database/Base.php#L1042

Tests here check whether only the queried internal attribute is returned.

$result->removeAttribute($parameter);
}
}
}
}

faisalill marked this conversation as resolved.
Show resolved Hide resolved
return $results;
}

Expand Down Expand Up @@ -4335,8 +4377,12 @@ public function decode(Document $collection, Document $document, array $selectio
}
}

if (empty($selections) || \in_array($key, $selections) || \in_array('*', $selections)) {
$document->setAttribute($key, ($array) ? $value : $value[0]);
if (empty($selections) || \in_array($key, $selections) || \in_array('*', $selections) || \in_array($key, ['$createdAt', '$updatedAt'])) {
faisalill marked this conversation as resolved.
Show resolved Hide resolved
if (($key === '$createdAt' || $key === '$updatedAt') && $value[0] === null) {
faisalill marked this conversation as resolved.
Show resolved Hide resolved
continue;
} else {
$document->setAttribute($key, ($array) ? $value : $value[0]);
}
}
}

Expand Down Expand Up @@ -4494,6 +4540,8 @@ private function validateSelections(Document $collection, array $queries): array
}
}

$keys = \array_merge($keys, Database::INTERNAL_PARAMETERS);

$invalid = \array_diff($selections, $keys);
if (!empty($invalid) && !\in_array('*', $invalid)) {
throw new DatabaseException('Cannot select attributes: ' . \implode(', ', $invalid));
Expand Down
41 changes: 30 additions & 11 deletions tests/Database/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,9 @@ public function testGetDocument(Document $document): Document
*/
public function testGetDocumentSelect(Document $document): Document
{
$document = static::getDatabase()->getDocument('documents', $document->getId(), [
$documentId = $document->getId();

$document = static::getDatabase()->getDocument('documents', $documentId, [
Query::select(['string', 'integer']),
]);

Expand All @@ -1029,6 +1031,23 @@ public function testGetDocumentSelect(Document $document): Document
$this->assertArrayNotHasKey('boolean', $document->getAttributes());
$this->assertArrayNotHasKey('colors', $document->getAttributes());
$this->assertArrayNotHasKey('with-dash', $document->getAttributes());
$this->assertArrayNotHasKey('$id', $document);
$this->assertArrayNotHasKey('$internalId', $document);
$this->assertArrayNotHasKey('$collection', $document);
$this->assertArrayNotHasKey('$permissions', $document);
$this->assertArrayNotHasKey('$createdAt', $document);
$this->assertArrayNotHasKey('$updatedAt', $document);

$document = static::getDatabase()->getDocument('documents', $documentId, [
Query::select(['string', 'integer', '$id', '$internalId', '$permissions', '$createdAt', '$updatedAt', '$collection']),
]);

$this->assertArrayHasKey('$id', $document);
$this->assertArrayHasKey('$internalId', $document);
$this->assertArrayHasKey('$permissions', $document);
$this->assertArrayHasKey('$createdAt', $document);
$this->assertArrayHasKey('$updatedAt', $document);
$this->assertArrayHasKey('$collection', $document);

return $document;
}
Expand Down Expand Up @@ -2539,18 +2558,19 @@ public function testFindSelect(): void
$documents = static::getDatabase()->find('movies', [
Query::select(['name', 'year'])
]);

foreach ($documents as $document) {
$this->assertArrayHasKey('$id', $document);
$this->assertArrayHasKey('$internalId', $document);
$this->assertArrayHasKey('$collection', $document);
$this->assertArrayHasKey('$createdAt', $document);
$this->assertArrayHasKey('$updatedAt', $document);
$this->assertArrayHasKey('$permissions', $document);
$this->assertArrayHasKey('name', $document);
$this->assertArrayHasKey('year', $document);
$this->assertArrayNotHasKey('director', $document);
$this->assertArrayNotHasKey('price', $document);
$this->assertArrayNotHasKey('active', $document);
$this->assertArrayNotHasKey('director', $document);
$this->assertArrayNotHasKey('$id', $document);
$this->assertArrayNotHasKey('$internalId', $document);
$this->assertArrayNotHasKey('$collection', $document);
$this->assertArrayNotHasKey('$createdAt', $document);
$this->assertArrayNotHasKey('$updatedAt', $document);
$this->assertArrayNotHasKey('$permissions', $document);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a test for the find method when selecting internal attributes


Expand Down Expand Up @@ -4156,14 +4176,13 @@ public function testOneToOneOneWayRelationship(): void
$this->assertArrayNotHasKey('area', $person->getAttribute('library'));

$person = static::getDatabase()->getDocument('person', 'person1', [
Query::select(['*', 'library.name'])
Query::select(['*', 'library.name', '$id'])
]);

$this->assertArrayHasKey('$id', $person);
$this->assertEquals('Library 1', $person->getAttribute('library')->getAttribute('name'));
faisalill marked this conversation as resolved.
Show resolved Hide resolved
$this->assertArrayNotHasKey('area', $person->getAttribute('library'));



$document = static::getDatabase()->getDocument('person', $person->getId(), [
Query::select(['name']),
]);
Expand Down