PHPORM-33: Add `Query::toMql()` and tests on query syntax #4

GromNaN · 2023-07-07T19:09:41Z

Fix PHPORM-33

This is the preliminary work before writing all the tests. It presents 2 ways of writing the tests:

The "integration" way: currently, the query features are tested by checking the query results. Each test have to initialise some collection data. The tests require a running server and can't run in parallel.
We could use the driver events to track the query, but it too deep and may change with server version and maybe the connection string.
The "unit" way: assertion are written on the query. I created the toMql() method for that.

In this PR

Add the method Mongodb\Query\Builder::toMql() to mimic the Illuminate\Database\Query::toSql(). No need for a toRawSql() equivalent because there is no data binding.
Import tests from Illuminate\Tests\Database\DatabaseQueryBuilderTest

The MQL format (my proposition)

MQL is a javascript-like format.

The collection name is not in the dumped format.

db.users.find().skip(5).take(10)

[
    'find' => [[], ['skip' => 5, 'limit' => 10]],
]

Or in split version:

[
    'find' => [],
    'skip' => [5],
    'take' => [10],
]

When chaining, add several calls to the array. This does not accept calls to the same method. But the query builder never use method chaining.

db.users.find({"name.family": "Smith"}).count()

[
    'find' => [['name.family' => 'Smith']],
    'count' => [],
]

GromNaN · 2023-07-10T08:48:21Z

src/Query/Builder.php

     */
-    public function getFresh($columns = [], $returnLazy = false)
+    public function toMql($columns = []): array


If we want toMql to return a MQL string like db.users.find({}), I can move this code in an intermediary private method.

If we want toMql to return a MQL string like db.users.find({})

What is an "MQL string" in that case? db.users.find({}) returns a cursor, which the shell's REPL iterates automatically to display a batch of documents. I'm not sure what that has to do with MQL.

Also, I cannot for the life of me find an actual definition of MQL in the server documentation. "MongoDB Query Language (MQL)" only seems to be mentioned in some Atlas docs and blog posts.

I used to think it referred to the query document syntax but given some of the usage in the above I'm not so sure that's the case anymore.

I certainly wouldn't have expected this method's return value (operation name followed by its arguments) to be considered MQL. That said, I can see how the match syntax alone might be insufficient to convey the query behavior, since it omits options like skip and limit.

The term MQL is a vague concept, it represent the calls to the high-level library. Ideally we could have an "export query for mongosh" feature in a profiler panel, but we don't have a 100% match in PHP and mongosh method signatures. That would work for the find, countDocuments, distinct, aggregate.

That said, I can see how the match syntax alone might be insufficient to convey the query behavior, since it omits options like skip and limit.

They can be formatted as find options or chained method calls.

Options:

[ 'find' => [[], ['skip' => 5, 'limit' => 10]], ]

Chained method calls:

[ 'find' => [], 'skip' => [5], 'take' => [10], ]

src/Query/Builder.php

alcaeus

We could use the driver events to track the query, but it too deep and may change with server version and maybe the connection string.

In this case, we'd track outgoing events (so CommandStartedEvent) to verify the command being sent to the server. While it is possible that it changes based on the connection string, such instances are limited (e.g. adding $readPreference when sending a query to mongos). With that in mind, it would indeed be feasible to use the event system to inspect queries, but that would still require a live database server to run the tests. IMO checking query generation without a MongoDB server has some advantages, but we shouldn't limit our testing to it.

alcaeus · 2023-07-10T08:26:32Z

composer.json

@@ -23,7 +23,8 @@
        "illuminate/container": "^10.0",
        "illuminate/database": "^10.0",
        "illuminate/events": "^10.0",
-        "mongodb/mongodb": "^1.15"
+        "mongodb/mongodb": "^1.15",
+        "ext-mongodb": "*"


👍, but I would move this to the top of the list.

src/Query/Builder.php

alcaeus · 2023-07-10T10:35:49Z

src/Query/Builder.php

@@ -279,21 +278,12 @@ public function getFresh($columns = [], $returnLazy = false)
                    $aggregations = blank($this->aggregate['columns']) ? [] : $this->aggregate['columns'];

                    if (in_array('*', $aggregations) && $function == 'count') {
-                        // When ORM is paginating, count doesnt need a aggregation, just a cursor operation
+                        // When ORM is paginating, count doesn't need an aggregation, just a cursor operation


Correction here: it isn't a cursor operation.

For context, the legacy driver used to have a Cursor::count method, but that would send off a separate count command which can yield different results. That is the reason why this was omitted from the Cursor class in the new driver.

It's not clear to me, is there something to change?

I think @alcaeus was commenting on the "just a cursor operation" phrase in this comment. The link to https://docs.mongodb.com/manual/reference/method/cursor.count/ a few lines below is related I'm sure.

Since Collection::count() is deprecated, I wonder if this should instead be using countDocuments() or estimatedDocumentCount() depending on whether a query filter is used or not, respectively.

I'll change to use countDocuments. The name estimatedDocumentCount make me feel that we shouldn't use it silently.

GromNaN

Imported a lot of tests from Eloquent, and fixed som bugs spotted by this tests.

Should be added as part of a new feature:

whereDate, whereDay, whereMonth, whereYear, whereTime
Operations accepting "raw" fields.
whereFulltext using $text indexes
union and unionAll using aggregations
subselect using aggregations
inRandomOrder
having in conjonction with groups using aggregations
whereExists
Jointures using aggregations

To be tested

Aggregations
Like

GromNaN · 2023-07-10T15:51:33Z

src/Query/Builder.php

+                    $direction = -1;
+                    break;
+                default:
+                    throw new \InvalidArgumentException('Order direction must be "asc" or "desc".');


This is a bugfix to throw an exception when using an incorrect value for $direction. Covered by tests imported from Illuminate\Tests\Database\DatabaseQueryBuilderTest::testOrderByInvalidDirectionParam()

I'm not sure if we have CS setup for this project yet, but should there be a use statement for InvalidArgumentException to avoid the global reference here?

On a separate note, I think we can omit the period after an exception message if there's only one sentence/phrase. That's what is done in PHPC/PHPLIB.

There is php-cs-fixer, the top-level classes doesn't require to be in use statements. We can decide to change the CS. https://jira.mongodb.org/browse/PHPORM-52

GromNaN · 2023-07-10T15:54:16Z

src/Query/Builder.php

-                        $item = new UTCDateTime($item->format('Uv'));
-                    }
-                });
+                if (is_array($where['values'])) {


This is a bugfix to not convert Carbon\CarbonPeriod properties $startDate and $endDate into MongoDB\BSON\UTCDateTime. CarbonCarbonPeriod::getStartDate() calls $this->startDate->avoidMutation() which does not exist on the MongoDB class.

AFAICT, the originally array_walk_recursive() call was just wrapped in an is_array($where['values']) condition. How does that fix the bug?

It's not clear to me how the original code would have converted anything other than a DateTimeInterface into UTCDateTime.

I see that Carbon/Carbon extends DateTime (and thus implements the interface), but it's not clear to me where Carbon/CarbonPeriod comes into play or why its internal logic would be relevant during this query compilation.

I'm sure that fixed the bug as described in the comment. I'll digg why the CarbonPeriod properties were updated.

GromNaN · 2023-07-10T15:54:58Z

src/Query/Builder.php

@@ -997,6 +1028,10 @@ protected function compileWheres(): array
            $method = "compileWhere{$where['type']}";
            $result = $this->{$method}($where);

+            if ($not) {
+                $result = ['$not' => $result];


This is a bugfix to support the $not operations! whereNot and orWhereNot.

GromNaN · 2023-07-10T15:57:38Z

src/Query/Builder.php

@@ -1101,7 +1136,7 @@ protected function compileWhereIn(array $where): array
    {
        extract($where);

-        return [$column => ['$in' => array_values($values)]];
+        return [$column => ['$in' => Arr::flatten($values)]];


This is a bugfix to support nested arrays. Test imported from Illuminate\Tests\Database\DatabaseQueryBuilderTest::testBasicWhereIns

src/Query/Builder.php

GromNaN · 2023-07-10T16:03:59Z

src/Query/Builder.php

+        if ($values instanceof CarbonPeriod) {
+            $values = [new UTCDateTime($values->start->format('Uv')), new UTCDateTime($values->end->format('Uv'))];
+        } else {
+            $values = Arr::flatten($values);


Bugfix to accept nested arrays in whereBetween. Test imported from Illuminate\Tests\Database\DatabaseQueryBuilderTest::testWhereBetweens

GromNaN · 2023-07-10T16:10:41Z

src/Query/Builder.php

+    }
+
+    /** @internal This method is not supported by MongoDB. */
+    public function whereColumn($first, $operator = null, $second = null, $boolean = 'and')


All this methods from the base Eloquent builder silently do nothing. By overloading I can add the @internal annotation so that they are not documented and appears strikethrough in good IDEs.

I noted that these are tested in testEloquentMethodsNotSupported(). Good job.

jmikola

Sharing some feedback from an initial, incomplete pass through the PR. Some of this query builder syntax is concerning, to say the least.

composer.json

src/Query/Builder.php

jmikola · 2023-07-11T00:25:49Z

src/Query/Builder.php

     */
-    public function getFresh($columns = [], $returnLazy = false)
+    public function toMql($columns = []): array


If we want toMql to return a MQL string like db.users.find({})

What is an "MQL string" in that case? db.users.find({}) returns a cursor, which the shell's REPL iterates automatically to display a batch of documents. I'm not sure what that has to do with MQL.

Also, I cannot for the life of me find an actual definition of MQL in the server documentation. "MongoDB Query Language (MQL)" only seems to be mentioned in some Atlas docs and blog posts.

I used to think it referred to the query document syntax but given some of the usage in the above I'm not so sure that's the case anymore.

I certainly wouldn't have expected this method's return value (operation name followed by its arguments) to be considered MQL. That said, I can see how the match syntax alone might be insufficient to convey the query behavior, since it omits options like skip and limit.

jmikola · 2023-07-11T00:31:07Z

src/Query/Builder.php

@@ -279,21 +278,12 @@ public function getFresh($columns = [], $returnLazy = false)
                    $aggregations = blank($this->aggregate['columns']) ? [] : $this->aggregate['columns'];

                    if (in_array('*', $aggregations) && $function == 'count') {
-                        // When ORM is paginating, count doesnt need a aggregation, just a cursor operation
+                        // When ORM is paginating, count doesn't need an aggregation, just a cursor operation


I think @alcaeus was commenting on the "just a cursor operation" phrase in this comment. The link to https://docs.mongodb.com/manual/reference/method/cursor.count/ a few lines below is related I'm sure.

Since Collection::count() is deprecated, I wonder if this should instead be using countDocuments() or estimatedDocumentCount() depending on whether a query filter is used or not, respectively.

jmikola · 2023-07-11T00:36:22Z

src/Query/Builder.php

-
-            // Return results
-            return new Collection($results);
+            return ['aggregate' => [$pipeline, $options]];


This may be outside the scope of the PR, but I'm reading through the existing code above and it looks like this constructs a pipeline in a fixed order based on query builder options. I could see that being relevant for $this->groups (assuming it's mimicking a group operation from the deprecated group command) but it seems odd for $this->aggregate since there's no flexibility to construct a pipeline.

$this->aggregate can only contain function and columns. I'll try to add tests on this feature.

jmikola · 2023-07-11T01:20:28Z

tests/QueryBuilderTest.php

+        $builder = $this->getBuilder();
+        $builder->where('_id', '=', 1)->orWhereNull(['_id', 'expires_at']);
+        $this->assertSame(['find' => [
+            ['$or' => [['_id' => 1], ['_id' => null], ['expires_at' => null]]],


As with my comment below, this query seems unintuitive. I would have expected the orWhereNull() conditions to be grouped within an $and operator.

tests/QueryBuilderTest.php

jmikola · 2023-07-11T01:31:35Z

tests/QueryBuilderTest.php

+        // can accept some nested arrays as values.
+        $builder = $this->getBuilder();
+        $builder->whereIn('id', [
+            ['issue' => 45582],
+            ['id' => 2],
+            [3],
+        ]);


This seems wrong. Nested arrays are actually matched by $in. Consider:

Enterprise rs0 [direct: primary] test> db.coll.insertOne({x:1}) { acknowledged: true, insertedId: ObjectId("64acb00e70dc0350a6d117ac") } Enterprise rs0 [direct: primary] test> db.coll.insertOne({x:[1,2,3]}) { acknowledged: true, insertedId: ObjectId("64acb01370dc0350a6d117ad") } Enterprise rs0 [direct: primary] test> db.coll.find({x:{$in:[1,2,3,4]}}) [ { _id: ObjectId("64acb00e70dc0350a6d117ac"), x: 1 }, { _id: ObjectId("64acb01370dc0350a6d117ad"), x: [ 1, 2, 3 ] } ] Enterprise rs0 [direct: primary] test> db.coll.find({x:{$in:[[1,2,3]]}}) [ { _id: ObjectId("64acb01370dc0350a6d117ad"), x: [ 1, 2, 3 ] } ] Enterprise rs0 [direct: primary] test> db.coll.find({x:{$in:[[1,2,3,4]]}})

In the first query, both documents match because x either is or has a value within the array [1,2,3,4].

In the second query, the [1,2,3] array within the $in array is treated as an exact match for only a single document.

The third query matches nothing, since neither document has the value [1,2,3,4] for x.

Even if array keys were to be discarded here, it seems wrong that:

[ ['issue' => 45582], ['id' => 2], [3], ]

...would compile to the same query as the previous test:

['issue' => 45582, 'id' => 2, 3]

I agree, I won't put Arr::flatten() and leave the nested arrays.

jmikola · 2023-07-11T01:32:51Z

src/Query/Builder.php

+    }
+
+    /** @internal This method is not supported by MongoDB. */
+    public function whereColumn($first, $operator = null, $second = null, $boolean = 'and')


I noted that these are tested in testEloquentMethodsNotSupported(). Good job.

Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

GromNaN · 2023-07-12T08:26:09Z

src/Query/Builder.php

@@ -1147,6 +1173,12 @@ protected function compileWhereBetween(array $where): array
    {
        extract($where);

+        if ($values instanceof CarbonPeriod) {


https://jira.mongodb.org/browse/PHPORM-47

GromNaN requested review from jmikola and alcaeus July 7, 2023 19:09

GromNaN force-pushed the PHPORM-33 branch 4 times, most recently from 055cfb1 to ec40454 Compare July 7, 2023 21:35

Add Query::toMql() and tests on query syntax

f7fe1bf

GromNaN force-pushed the PHPORM-33 branch from ec40454 to f7fe1bf Compare July 7, 2023 21:39

GromNaN commented Jul 10, 2023

View reviewed changes

src/Query/Builder.php Show resolved Hide resolved

GromNaN commented Jul 10, 2023

View reviewed changes

src/Query/Builder.php Outdated Show resolved Hide resolved

alcaeus reviewed Jul 10, 2023

View reviewed changes

GromNaN added 2 commits July 10, 2023 17:43

Add tests and fix bugs

76983ef

review

1370e57

GromNaN commented Jul 10, 2023

View reviewed changes

Fix like to be case-sensitive and support not ilike

06d3624

jmikola reviewed Jul 11, 2023

View reviewed changes

GromNaN and others added 3 commits July 11, 2023 10:53

Review

c566ba0

Create UTCDateTime from DateTimeInterface

b13e7e0

Apply suggestions from code review

b258fac

Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

GromNaN mentioned this pull request Jul 11, 2023

PHPORM-45 Add Query\Builder::toMql() to simplify comprehensive query tests #6

Merged

GromNaN commented Jul 12, 2023

View reviewed changes

GromNaN mentioned this pull request Jul 12, 2023

PHPORM-51 Throw an exception when unsupported query builder method is used #9

Merged

jmikola mentioned this pull request Jul 12, 2023

PHPORM-47 Improve Builder::whereBetween to support CarbonPeriod and reject invalid array #10

Merged

This was referenced Jul 17, 2023

PHPORM-33 Add tests on Query\Builder methods #14

Merged

PHPORM-50 PHPORM-65 Remove call to deprecated Collection::count for countDocuments #18

Merged

GromNaN marked this pull request as draft July 20, 2023 14:29

GromNaN closed this Jul 26, 2023

GromNaN deleted the PHPORM-33 branch July 27, 2023 06:31

PHPORM-33: Add Query::toMql() and tests on query syntax #4

PHPORM-33: Add Query::toMql() and tests on query syntax #4

Uh oh!

Conversation

GromNaN commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

In this PR

The MQL format (my proposition)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN Jul 11, 2023 • edited by jmikola Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmikola Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PHPORM-33: Add `Query::toMql()` and tests on query syntax #4

PHPORM-33: Add `Query::toMql()` and tests on query syntax #4

GromNaN commented Jul 7, 2023 •

edited

Loading

jmikola Jul 11, 2023 •

edited

Loading

GromNaN Jul 11, 2023 •

edited by jmikola

Loading

GromNaN left a comment •

edited

Loading

jmikola Jul 11, 2023 •

edited

Loading