PHPORM-238 Add support for `withCount` and other aggregations by GromNaN · Pull Request #3182 · mongodb/laravel-mongodb

GromNaN · 2024-10-17T17:21:49Z

Implement this feature from Laravel: https://laravel.com/docs/11.x/eloquent-relationships#counting-related-models
Counting relations with SQL is done using a subquery. For MongoDB, there is 2 ways:

Making a query during eager loading, that count the objects for each result ID. This is an aggregation with a $group by foreign key.
Using $lookup to load references and count them. The process would be similar for embedded relations (without the $lookup). But this doesn't work with hybrid relations.

Checklist

Add tests and ensure they pass
Support mongodb relationships
CANCELLED: Support hybrid relationships (PR reverted from Laravel: [11.x] Support DB aggregate by group (new methods) laravel/framework#53679)
Support embedded relationships, contraint query not supported PHPORM-292

src/Query/Builder.php

src/Eloquent/Builder.php

tests/Eloquent/EloquentWithAggregateTest.php

tests/HybridRelationsTest.php

jmikola · 2025-01-23T19:38:47Z

tests/QueryBuilderTest.php

+        $this->assertFalse(DB::table('items')->where('name', 'ladle')->exists());
+        $this->assertFalse(DB::table('items')->doesntExist());
+        $this->assertFalse(DB::table('items')->where('name', 'knife')->doesntExist());
+        $this->assertTrue(DB::table('items')->where('name', 'ladle')->doesntExist());


This looks like basic tests for exists() and doesntExist() methods. How is this related to the PR?

I realized that was not tested when working on the aggregate method. That could be part of PHPORM-285 that I had to postpone.

I'd agree that this is better off in a separate PR/commit. Or include it here but reference PHPORM-285 in the commit message (which can still be squashed at merge time, but at least we have a note pointing to the relevant issue).

I removed this tests and created #3257
I realized, only doesntExist was not tested.

tests/HybridRelationsTest.php

jmikola · 2025-01-23T19:44:03Z

tests/Eloquent/EloquentWithAggregateTest.php

+            ['id' => 2, 'twos_min' => 4],
+        ], $results->get());
+
+        $results = EloquentWithAggregateModel1::withAvg(['twos' => $filter], 'value')->where('id', 2);


In QueryBuilderTest, I noted there were two names for the same method: avg() and average(). Is there also a withAverage() method that should be tested?

tests/Eloquent/EloquentWithAggregateTest.php

src/Eloquent/Builder.php

jmikola · 2025-01-23T20:53:59Z

src/Eloquent/Builder.php

+            $modelIds = collect($models)->pluck($this->model->getKeyName())->all();
+
+            foreach ($this->withAggregate as $withAggregate) {
+                if ($withAggregate['relation'] instanceof HasMany) {


Given that this only applies some processing for HasMany relations, under what circumstances would different relations get appended to $this->withAggregate? And are they OK being left as-is?

Adding an exception in the "else" case. I thing the only other case if "HasOne", but I need to check "BelongsToMany".

src/Eloquent/Builder.php

GromNaN

Thanks, I addressed most comments but need to add tests on the new error cases.

GromNaN · 2025-01-23T21:08:20Z

src/Query/Builder.php


+    public function count($columns = '*')
+    {
+        // Can be removed when available in Laravel: https://github.com/laravel/framework/pull/53209


Then reverted again: laravel/framework#54196

GromNaN · 2025-01-23T21:09:26Z

tests/Eloquent/EloquentWithAggregateTest.php

+
+    public function testWithAggregate()
+    {
+        EloquentWithAggregateModel1::create(['id' => 1]);


This ensures that the initial query filter on id works (perhaps I'm being too creative about what might fail).

tests/HybridRelationsTest.php

GromNaN · 2025-01-23T21:14:07Z

tests/QueryBuilderTest.php

+        $this->assertFalse(DB::table('items')->where('name', 'ladle')->exists());
+        $this->assertFalse(DB::table('items')->doesntExist());
+        $this->assertFalse(DB::table('items')->where('name', 'knife')->doesntExist());
+        $this->assertTrue(DB::table('items')->where('name', 'ladle')->doesntExist());


I realized that was not tested when working on the aggregate method. That could be part of PHPORM-285 that I had to postpone.

GromNaN · 2025-01-23T21:36:23Z

src/Eloquent/Builder.php

+
+        $relations = is_array($relations) ? $relations : [$relations];
+
+        foreach ($this->parseWithRelations($relations) as $name => $constraints) {


It's adding the constraint closure from the scope and the relationships.

src/Eloquent/Builder.php

tests/Eloquent/EloquentWithAggregateTest.php

src/Eloquent/Builder.php

GromNaN commented Oct 17, 2024

View reviewed changes

src/Query/Builder.php Show resolved Hide resolved

GromNaN commented Oct 17, 2024

View reviewed changes

src/Query/Builder.php Show resolved Hide resolved

GromNaN mentioned this pull request Jan 10, 2025

PHPORM-286 Add Query::countByGroup() and other aggregateByGroup() functions #3243

Merged

1 task

GromNaN added 4 commits January 20, 2025 21:27

PHPORM-238 Add support for withCount using a subquery

b55bdc6

Ensure Hydratation of _count is done in a single query

2cf1828

Validate arg type and avoid subsequent error

4285880

Add more tests

f86f52e

GromNaN force-pushed the PHPORM-238 branch from 8fe3d6b to f86f52e Compare January 20, 2025 20:29

github-advanced-security bot found potential problems Jan 20, 2025

View reviewed changes

src/Eloquent/Builder.php Fixed Show fixed Hide fixed

src/Eloquent/Builder.php Fixed Show fixed Hide fixed

GromNaN added 2 commits January 21, 2025 18:19

Fix withAggregate

e77c16d

Implement withAggregate for embedded relations

29ff22a

github-advanced-security bot found potential problems Jan 21, 2025

View reviewed changes

src/Eloquent/Builder.php Fixed Show fixed Hide fixed

src/Eloquent/Builder.php Fixed Show fixed Hide fixed

GromNaN commented Jan 21, 2025

View reviewed changes

tests/Eloquent/EloquentWithAggregateTest.php Outdated Show resolved Hide resolved

GromNaN added 2 commits January 22, 2025 16:50

Fix CS

303ceb7

Add exception for non-supported withAggregate for hybrid relationships

299d5ef

GromNaN marked this pull request as ready for review January 22, 2025 17:28

GromNaN requested a review from a team as a code owner January 22, 2025 17:28

GromNaN requested a review from jmikola January 22, 2025 17:28

jmikola reviewed Jan 23, 2025

View reviewed changes

Review

1d4699e

GromNaN commented Jan 23, 2025

View reviewed changes

github-advanced-security bot found potential problems Jan 23, 2025

View reviewed changes

src/Eloquent/Builder.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jan 23, 2025

View reviewed changes

src/Eloquent/Builder.php Fixed Show fixed Hide fixed

jmikola reviewed Jan 24, 2025

View reviewed changes

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

Fix aggregation alias

42053d0

GromNaN force-pushed the PHPORM-238 branch from 335b30e to 42053d0 Compare January 29, 2025 12:47

GromNaN requested a review from jmikola January 29, 2025 14:03

Remove tests on exists and doesntExist

3bcfe7c

GromNaN mentioned this pull request Jan 29, 2025

Add tests on doesntExist #3257

Merged


		$relations = is_array($relations) ? $relations : [$relations];

		foreach ($this->parseWithRelations($relations) as $name => $constraints) {

Conversation

GromNaN commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GromNaN left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GromNaN commented Oct 17, 2024 •

edited

Loading