-
Notifications
You must be signed in to change notification settings - Fork 48
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
Feat 5376 improve the select query #287
Feat 5376 improve the select query #287
Conversation
fixed query and attribute types
@fogelito can you review this PR please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
starting CI now as well 👍🏻
Combine $createdAt and $updatedAt to in_array Use static internal parameteres array rather than $stringToRemove
$projection['_createdAt'] = 1; | ||
$projection['_updatedAt'] = 1; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
- no _permissions returns a null document (https://github.com/utopia-php/database/blob/main/src/Database/Database.php#L2238)
- no _uid will not convert the data types of the document
- no _createdAt and _updatedAt are required here (https://github.com/utopia-php/database/blob/main/src/Database/Adapter/Mongo.php#L971)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- no _permissions returns a null document (https://github.com/utopia-php/database/blob/main/src/Database/Database.php#L2238)
- no _uid will not convert the data types of the document
There was a problem hiding this comment.
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.
src/Database/Database.php
Outdated
foreach ($queries as $query) { | ||
if ($query->getMethod() === Query::TYPE_SELECT) { | ||
foreach ($results as $result) { | ||
foreach (Database::INTERNAL_PARAMETERS as $parameter) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests here check whether only the queried internal attribute is returned.
$this->assertArrayNotHasKey('$collection', $document); | ||
$this->assertArrayNotHasKey('$createdAt', $document); | ||
$this->assertArrayNotHasKey('$updatedAt', $document); | ||
$this->assertArrayNotHasKey('$permissions', $document); | ||
} | ||
} |
There was a problem hiding this comment.
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
I will close this PR start from scratch and update u guys on the same. |
No description provided.