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

GromNaN · 2023-07-17T15:28:41Z

Add tests on query builder methods that don't need to be fixed.
Throw exception when calling unsupported methods: whereIntegerInRaw, orWhereIntegerInRaw, whereIntegerNotInRaw, orWhereIntegerNotInRaw
Throw an exception when Query\Builder::where is called with only a column name

tests/Query/BuilderTest.php

jmikola · 2023-07-17T17:22:28Z

tests/Query/BuilderTest.php

            ['find' => [['foo' => ['$in' => ['bar', 'baz']]], []]],
            fn (Builder $builder) => $builder->whereIn('foo', ['bar', 'baz']),
        ];

+        yield 'whereIn associative array' => [
+            ['find' => [['id' => ['$in' => [45582, 2, 3]]], []]],
+            fn (Builder $builder) => $builder->whereIn('id', ['issue' => 45582, 'id' => 2, 3]),


Can we consider raising an exception if the value is not a list, or is that likely to ruffle some feathers among users? Passing an associative array this way seems incorrect, and MongoDB itself would rightfully complain if a BSON object was provided where an array is expected.

Exception added. I agree that we need to be more strict than Laravel, with didactic error messages.

Where was the exception added? I only error tests for "find with single string argument" and various "IntegerInRaw" cases, neither of which are related to this.

Sorry I reverted. I inadvertently made the change in laravel/framework before realising it would be complex to do in laravel-mongodb. If you insist I can try to add this exception in an other PR.

Let's revisit this in a separate PR -- feel free to create a new issue to track it.

I'm very much in favor of raising exceptions for poorly formatted values. As annoying as it may be for some users, I think it will ultimately avoid unexpected edge cases and bugs down the line.

tests/Query/BuilderTest.php

jmikola · 2023-07-19T17:03:20Z

tests/Query/BuilderTest.php

            ['find' => [['foo' => ['$in' => ['bar', 'baz']]], []]],
            fn (Builder $builder) => $builder->whereIn('foo', ['bar', 'baz']),
        ];

+        yield 'whereIn associative array' => [
+            ['find' => [['id' => ['$in' => [45582, 2, 3]]], []]],
+            fn (Builder $builder) => $builder->whereIn('id', ['issue' => 45582, 'id' => 2, 3]),


Where was the exception added? I only error tests for "find with single string argument" and various "IntegerInRaw" cases, neither of which are related to this.

tests/Query/BuilderTest.php

jmikola · 2023-07-20T18:58:26Z

tests/Query/BuilderTest.php

+
+        yield 'offset limit negative' => [
+            ['find' => [[], []]],
+            fn (Builder $builder) => $builder->offset(-5)->limit(-10),


Not sure if I missed this in a previous review or if this was recently added, but I'd propose throwing an exception if negative values are passed for offset and limit. That seems preferable to ignoring them outright.

That's a Laravel feature. I'm not sure we should care about changing this behavior.
https://github.com/laravel/framework/blob/10.x/src/Illuminate/Database/Query/Builder.php#L2435-L2437

Noted. We can leave this as-is.

jmikola · 2023-07-20T19:02:02Z

tests/Query/BuilderTest.php

+
+        yield 'offset 0 limit 0' => [
+            ['find' => [[], []]],
+            fn (Builder $builder) => $builder->offset(0)->limit(0),


Offhand, do you know if zero is handled exactly the same as "null (reset)"?

If so, I'd be in favor of the following changes:

Rename this test to "offset limit zero (unset)"

Create a new test below named "offset limit null (unset)", based on this test. The point of these tests would be to demonstrate that zero and null values both result in the options being unspecified.

Create a new test here named "offset limit zero (reset)", based on the "offset limit null (reset)" test below. Like that, it would demonstrate that zero and null both reset the options back to an unset state

This comment might have been missed.

I'll defer to you if you'd like to make the suggested test changes. Otherwise, feel free to resolve.

Test updated. Thanks for the reminder.

GromNaN · 2023-07-25T07:40:50Z

tests/TransactionTest.php

@@ -332,7 +332,7 @@ public function testTransaction(): void
        $count = User::count();
        $this->assertEquals(2, $count);

-        $this->assertTrue(User::where('alcaeus')->exists());
+        $this->assertTrue(User::where('name', 'alcaeus')->exists());


Being too permissive didn't allow to detect this incorrect query.

- { "alcaeus": null } + { "name": "alcaeus" }

The new exception made it surface.

I have to stop using my name in tests. It makes blaming me for my mistakes too easy 😅

alcaeus

LGTM.

alcaeus · 2023-07-25T07:47:14Z

src/Query/Builder.php

@@ -923,6 +923,10 @@ public function where($column, $operator = null, $value = null, $boolean = 'and'
            }
        }


Can't comment on the if itself, but I'm wondering if the condition should read func_num_args() >= 3 - we wouldn't be removing the $ from an operator if $boolean was defined as well 🤔

It's not clear to me how $boolean is used, but I see it's passed on to the parent method below. I think that's right, and this suggests another lingering bug.

I'll defer to you guys whether to fix here (if so, add a regression test that passes a fourth parameter to where() (I suppose an explicit 'and' string is fine) or report a new JIRA issue and address in a subsequent PR. A new issue probably makes more sense for visibility in the release notes.

Fixed in #20

alcaeus · 2023-07-25T07:47:39Z

src/Query/Builder.php

@@ -1376,4 +1380,28 @@ public function havingBetween($column, iterable $values, $boolean = 'and', $not
    {
        throw new \BadMethodCallException('This method is not supported by MongoDB');
    }
+
+    /** @internal This method is not supported by MongoDB. */


Good strategy to mark these as internal to have the IDE notify users already 👍

alcaeus · 2023-07-25T07:49:43Z

tests/TransactionTest.php

@@ -332,7 +332,7 @@ public function testTransaction(): void
        $count = User::count();
        $this->assertEquals(2, $count);

-        $this->assertTrue(User::where('alcaeus')->exists());
+        $this->assertTrue(User::where('name', 'alcaeus')->exists());


I have to stop using my name in tests. It makes blaming me for my mistakes too easy 😅

jmikola

Two follow-up comments.

LGTM to merge as-is if you'd rather not make the test changes. In that case, the issue @alcaeus mentioned in https://github.com/GromNaN/laravel-mongodb/pull/14/files#r1273137525 should be tracked in a separate ticket.

jmikola · 2023-07-26T03:43:55Z

src/Query/Builder.php

@@ -923,6 +923,10 @@ public function where($column, $operator = null, $value = null, $boolean = 'and'
            }
        }


It's not clear to me how $boolean is used, but I see it's passed on to the parent method below. I think that's right, and this suggests another lingering bug.

I'll defer to you guys whether to fix here (if so, add a regression test that passes a fourth parameter to where() (I suppose an explicit 'and' string is fine) or report a new JIRA issue and address in a subsequent PR. A new issue probably makes more sense for visibility in the release notes.

jmikola · 2023-07-26T03:45:09Z

tests/Query/BuilderTest.php

+
+        yield 'offset 0 limit 0' => [
+            ['find' => [[], []]],
+            fn (Builder $builder) => $builder->offset(0)->limit(0),


This comment might have been missed.

I'll defer to you if you'd like to make the suggested test changes. Otherwise, feel free to resolve.

- Add tests on query builder methods that don't need to be fixed. - Throw exception when calling unsupported methods: whereIntegerInRaw, orWhereIntegerInRaw, whereIntegerNotInRaw, orWhereIntegerNotInRaw - Throw an exception when Query\Builder::where is called with only a column name

GromNaN requested review from jmikola and alcaeus July 17, 2023 15:28

jmikola reviewed Jul 17, 2023

View reviewed changes

GromNaN force-pushed the PHPORM-33-1 branch 3 times, most recently from 437d9cf to 44baa9b Compare July 19, 2023 14:11

jmikola reviewed Jul 19, 2023

View reviewed changes

GromNaN force-pushed the PHPORM-33-1 branch from 44baa9b to beb6356 Compare July 20, 2023 14:38

jmikola suggested changes Jul 20, 2023

View reviewed changes

GromNaN force-pushed the PHPORM-33-1 branch 2 times, most recently from 7e0f82d to 43dfe24 Compare July 25, 2023 07:31

GromNaN commented Jul 25, 2023

View reviewed changes

GromNaN requested a review from jmikola July 25, 2023 07:42

alcaeus approved these changes Jul 25, 2023

View reviewed changes

jmikola approved these changes Jul 26, 2023

View reviewed changes

GromNaN added 5 commits July 26, 2023 09:19

PHPORM-33 Add tests on Query\Builder methods

6ebcdb5

Add tests on latest/oldest & skip/limit

43b7fa5

review

965c107

Fix query in transaction test

df3167e

Add test

89234a0

GromNaN force-pushed the PHPORM-33-1 branch from 5488911 to 89234a0 Compare July 26, 2023 07:23

GromNaN merged commit b0b796c into master Jul 26, 2023

GromNaN deleted the PHPORM-33-1 branch July 26, 2023 07:28

		@@ -923,6 +923,10 @@ public function where($column, $operator = null, $value = null, $boolean = 'and'
		}
		}

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

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

Uh oh!

Conversation

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

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!

jmikola Jul 20, 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Jul 21, 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!

GromNaN Jul 25, 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!

alcaeus 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!

jmikola 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!

GromNaN commented Jul 17, 2023 •

edited

Loading

jmikola Jul 20, 2023 •

edited

Loading

jmikola Jul 21, 2023 •

edited

Loading

GromNaN Jul 25, 2023 •

edited

Loading

alcaeus left a comment •

edited

Loading