Support for Aggregate Queries#355
Conversation
montymxb
left a comment
montymxb
left a comment
There was a problem hiding this comment.
I like it! Just some things here and there, and the tests look good at initial glance. We'll have to wait and see for when this feature is merged in to judge where the tests stand.
| */ | ||
| public function distinct($key) | ||
| { | ||
| $sessionToken = null; |
There was a problem hiding this comment.
You can do something like this to avoid calling ParseUser::getCurrentUser() twice, while still validating there is a current user.
if($user = ParseUser::getCurrentUser()) {
...
}$user would be set in the conditional block then to use.
| 'GET', | ||
| 'aggregate/'.$this->className.'?'.$queryString, | ||
| $sessionToken, | ||
| null, |
There was a problem hiding this comment.
I haven't looked over the PR in detail yet over on the server, but does this require the master key to work? If not we should be adding a method parameter to optionally use the master key, same as how the other query methods work.
There was a problem hiding this comment.
The server requires a masterKey for security purposes
| return $result['results']; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Just a little textual typo on a, Execute an aggregrate...
| */ | ||
| public function aggregate($pipeline) | ||
| { | ||
| $sessionToken = null; |
There was a problem hiding this comment.
Same thing as above regarding calling this twice.
| 'GET', | ||
| 'aggregate/'.$this->className.'?'.$queryString, | ||
| $sessionToken, | ||
| null, |
There was a problem hiding this comment.
Same thing here regarding the use of the master key. If it's not required to work it should be a method parameter with a default of false.
Codecov Report
@@ Coverage Diff @@
## master #355 +/- ##
==========================================
+ Coverage 99% 99.01% +0.01%
==========================================
Files 38 38
Lines 3522 3559 +37
==========================================
+ Hits 3487 3524 +37
Misses 35 35
Continue to review full report at Codecov.
|
|
@montymxb I noticed this issue as well parse-community/parse-server#4351 |
There was a problem hiding this comment.
Awesome! The code is good to go, but can I request that you add a small section in the README for this? You may have noticed that I've restructured it to start with a table of contents, everything is pretty easy to find from that. You can add a part in the Queries section. You can add it above or below the Relative Time sub-section, wherever works so long as it's in that group.
As part of the upcoming 1.4.0 release I'll be going over all changes since 1.3.0 and putting up a complete PR for the docs, so we're good on that end.
| $this->assertEquals(0, count($results)); | ||
| } | ||
|
|
||
| public function testDisinctOnUsers() |
There was a problem hiding this comment.
Whoop almost missed this, typo here on testDisinctOnUsers
|
Great feature to move on |
3 participants
Support for
distinct()andaggregate()queries.Pending: parse-community/parse-server#4207