Skip to content

PHPORM-90 Fix whereNot to use $nor #2624

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

Merged
merged 2 commits into from
Sep 19, 2023
Merged

PHPORM-90 Fix whereNot to use $nor #2624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00cb3d2
PHPORM-90 Fix whereNot to use $nor
GromNaN Sep 18, 2023
3d29332
add double negation nested test
GromNaN Sep 19, 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
3 changes: 2 additions & 1 deletion src/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,8 +1053,9 @@ protected function compileWheres(): array
$method = 'compileWhere' . $where['type'];
$result = $this->{$method}($where);

// Negate the expression
Copy link
Member

@jmikola jmikola Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is $result always going to be an <expression> at this point, and never an <operator-expression> as it expected by $not?

This is related to my other comment, in that I'm curious how this wasn't discovered sooner unless it was only recently introduced. This also hints at a gap in functional test coverage running generated queries on a live server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, compileWhere* function always return expressions with $and, $or or a field name, and now $nor.

if (str_ends_with($where['boolean'], 'not')) {
$result = ['$not' => $result];
$result = ['$nor' => [$result]];
Copy link
Member

Choose a reason for hiding this comment

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

I don't see PHPORM-90 linked to anything. This this bug predate us taking over the library, or was it introduced during some recent query refactoring? If the latter, we should link it up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented whereNot in GromNaN/laravel-mongodb#13. The issue PHPORM-49 is now linked in Jira.

}

// Wrap the where with an $or operator.
Expand Down
58 changes: 33 additions & 25 deletions tests/Query/BuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,8 @@ public static function provideQueryBuilderToMql(): iterable
'find' => [
[
'$and' => [
['$not' => ['name' => 'foo']],
['$not' => ['name' => ['$ne' => 'bar']]],
['$nor' => [['name' => 'foo']]],
['$nor' => [['name' => ['$ne' => 'bar']]]],
],
],
[], // options
Expand Down Expand Up @@ -231,8 +231,8 @@ public static function provideQueryBuilderToMql(): iterable
'find' => [
[
'$or' => [
['$not' => ['name' => 'foo']],
['$not' => ['name' => ['$ne' => 'bar']]],
['$nor' => [['name' => 'foo']]],
['$nor' => [['name' => ['$ne' => 'bar']]]],
],
],
[], // options
Expand All @@ -248,7 +248,7 @@ public static function provideQueryBuilderToMql(): iterable
'find' => [
[
'$or' => [
['$not' => ['name' => 'foo']],
['$nor' => [['name' => 'foo']]],
['name' => ['$ne' => 'bar']],
],
],
Expand All @@ -264,7 +264,7 @@ public static function provideQueryBuilderToMql(): iterable
yield 'whereNot callable' => [
[
'find' => [
['$not' => ['name' => 'foo']],
['$nor' => [['name' => 'foo']]],
[], // options
],
],
Expand All @@ -278,7 +278,7 @@ public static function provideQueryBuilderToMql(): iterable
[
'$and' => [
['name' => 'bar'],
['$not' => ['email' => 'foo']],
['$nor' => [['email' => 'foo']]],
],
],
[], // options
Expand All @@ -295,10 +295,12 @@ public static function provideQueryBuilderToMql(): iterable
[
'find' => [
[
'$not' => [
'$and' => [
['name' => 'foo'],
['$not' => ['email' => ['$ne' => 'bar']]],
'$nor' => [
[
'$and' => [
['name' => 'foo'],
['$nor' => [['email' => ['$ne' => 'bar']]]],
],
],
],
],
Expand All @@ -318,7 +320,7 @@ public static function provideQueryBuilderToMql(): iterable
[
'$or' => [
['name' => 'bar'],
['$not' => ['email' => 'foo']],
['$nor' => [['email' => 'foo']]],
],
],
[], // options
Expand All @@ -337,7 +339,7 @@ public static function provideQueryBuilderToMql(): iterable
[
'$or' => [
['name' => 'bar'],
['$not' => ['email' => 'foo']],
['$nor' => [['email' => 'foo']]],
],
],
[], // options
Expand All @@ -353,10 +355,12 @@ public static function provideQueryBuilderToMql(): iterable
[
'find' => [
[
'$not' => [
'$and' => [
['foo' => 1],
['bar' => 2],
'$nor' => [
[
'$and' => [
['foo' => 1],
['bar' => 2],
],
],
],
],
Expand All @@ -371,10 +375,12 @@ public static function provideQueryBuilderToMql(): iterable
[
'find' => [
[
'$not' => [
'$and' => [
['foo' => 1],
['bar' => 2],
'$nor' => [
[
'$and' => [
['foo' => 1],
['bar' => 2],
],
],
],
],
Expand All @@ -389,10 +395,12 @@ public static function provideQueryBuilderToMql(): iterable
[
'find' => [
[
'$not' => [
'$and' => [
['foo' => 1],
['bar' => ['$lt' => 2]],
'$nor' => [
[
'$and' => [
['foo' => 1],
['bar' => ['$lt' => 2]],
],
],
],
],
Expand Down
44 changes: 44 additions & 0 deletions tests/QueryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

use DateTimeImmutable;
use LogicException;
use MongoDB\BSON\Regex;
use MongoDB\Laravel\Eloquent\Builder;
use MongoDB\Laravel\Tests\Models\Birthday;
use MongoDB\Laravel\Tests\Models\Scoped;
use MongoDB\Laravel\Tests\Models\User;
Expand Down Expand Up @@ -154,6 +156,48 @@ public function testSelect(): void
$this->assertNull($user->age);
}

public function testWhereNot(): void
{
// implicit equality operator
$users = User::whereNot('title', 'admin')->get();
$this->assertCount(6, $users);
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is failing if I run it on version 3.9.3: the generated query was { title: 'admin' }, which is wrong because it ignore the negation.
In the current master, the query is { $not: { title: 'admin' } }, which is invalid and rejected by the server.
With this patch, the working query is { $nor: [ { title: 'admin' } ] }.

Copy link
Member

Choose a reason for hiding this comment

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

reviewing the generated query was { title: 'admin' }, which is wrong because it ignore the negation.

I'm not sure if I realized this when originally reviewing PHPORM-49, but the fact that earlier versions completely ignored negations with no feedback to the user sounds like pretty disastrous bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like someone asked for this feature: #2418


// nested query
$users = User::whereNot(fn (Builder $builder) => $builder->where('title', 'admin'))->get();
$this->assertCount(6, $users);

// double negation
$users = User::whereNot('title', '!=', 'admin')->get();
$this->assertCount(3, $users);

// nested negation
$users = User::whereNot(fn (Builder $builder) => $builder
->whereNot('title', 'admin'))->get();
$this->assertCount(3, $users);

// explicit equality operator
$users = User::whereNot('title', '=', 'admin')->get();
$this->assertCount(6, $users);

// custom query operator
$users = User::whereNot('title', ['$in' => ['admin']])->get();
$this->assertCount(6, $users);

// regex
$users = User::whereNot('title', new Regex('^admin$'))->get();
$this->assertCount(6, $users);

// equals null
$users = User::whereNot('title', null)->get();
$this->assertCount(8, $users);

// nested $or
$users = User::whereNot(fn (Builder $builder) => $builder
->where('title', 'admin')
->orWhere('age', 35))->get();
$this->assertCount(5, $users);
}

public function testOrWhere(): void
{
$users = User::where('age', 13)->orWhere('title', 'admin')->get();
Expand Down