From e265e11acfd63c01397046e9bfb34deaa1f36af6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Sun, 15 Oct 2023 21:40:21 +0200 Subject: [PATCH 1/9] PHPORM-100 Support query on numerical field names --- src/Eloquent/Model.php | 8 ++++++++ src/Query/Builder.php | 21 ++++++++++++++------- tests/ModelTest.php | 16 ++++++++++++++++ tests/Query/BuilderTest.php | 13 ++++++++++++- 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/Eloquent/Model.php b/src/Eloquent/Model.php index 05a20bb31..666ca9c90 100644 --- a/src/Eloquent/Model.php +++ b/src/Eloquent/Model.php @@ -169,6 +169,8 @@ public function getAttribute($key) return null; } + $key = (string) $key; + // An unset attribute is null or throw an exception. if (isset($this->unset[$key])) { return $this->throwMissingAttributeExceptionIfApplicable($key); @@ -194,6 +196,8 @@ public function getAttribute($key) /** @inheritdoc */ protected function getAttributeFromArray($key) { + $key = (string) $key; + // Support keys in dot notation. if (str_contains($key, '.')) { return Arr::get($this->attributes, $key); @@ -212,6 +216,8 @@ public function setAttribute($key, $value) $value = $builder->convertKey($value); } + $key = (string) $key; + // Support keys in dot notation. if (str_contains($key, '.')) { // Store to a temporary key, then move data to the actual key @@ -314,6 +320,8 @@ public function originalIsEquivalent($key) /** @inheritdoc */ public function offsetUnset($offset): void { + $offset = (string) $offset; + if (str_contains($offset, '.')) { // Update the field in the subdocument Arr::forget($this->attributes, $offset); diff --git a/src/Query/Builder.php b/src/Query/Builder.php index 82ba9d09a..945051c4b 100644 --- a/src/Query/Builder.php +++ b/src/Query/Builder.php @@ -23,13 +23,11 @@ use MongoDB\BSON\UTCDateTime; use MongoDB\Driver\Cursor; use RuntimeException; -use Stringable; use function array_fill_keys; use function array_is_list; use function array_key_exists; use function array_merge; -use function array_merge_recursive; use function array_values; use function array_walk_recursive; use function assert; @@ -47,6 +45,7 @@ use function in_array; use function is_array; use function is_int; +use function is_scalar; use function is_string; use function md5; use function preg_match; @@ -60,6 +59,7 @@ use function strlen; use function strtolower; use function substr; +use function var_export; class Builder extends BaseBuilder { @@ -665,7 +665,7 @@ public function update(array $values, array $options = []) { // Use $set as default operator for field names that are not in an operator foreach ($values as $key => $value) { - if (str_starts_with($key, '$')) { + if (is_string($key) && str_starts_with($key, '$')) { continue; } @@ -966,8 +966,8 @@ public function where($column, $operator = null, $value = null, $boolean = 'and' } } - if (func_num_args() === 1 && is_string($column)) { - throw new ArgumentCountError(sprintf('Too few arguments to function %s("%s"), 1 passed and at least 2 expected when the 1st is a string.', __METHOD__, $column)); + if (func_num_args() === 1 && is_scalar($column)) { + throw new ArgumentCountError(sprintf('Too few arguments to function %s(%s), 1 passed and at least 2 expected when the 1st is a scalar.', __METHOD__, var_export($column, true))); } return parent::where(...$params); @@ -998,7 +998,7 @@ protected function compileWheres(): array } // Convert column name to string to use as array key - if (isset($where['column']) && $where['column'] instanceof Stringable) { + if (isset($where['column'])) { $where['column'] = (string) $where['column']; } @@ -1076,7 +1076,14 @@ protected function compileWheres(): array } // Merge the compiled where with the others. - $compiled = array_merge_recursive($compiled, $result); + // array_merge_recursive can't be used here because it converts int keys to sequential int. + foreach ($result as $key => $value) { + if (in_array($key, ['$and', '$or', '$nor'])) { + $compiled[$key] = array_merge($compiled[$key] ?? [], $value); + } else { + $compiled[$key] = $value; + } + } } return $compiled; diff --git a/tests/ModelTest.php b/tests/ModelTest.php index afa95c203..04f2abb51 100644 --- a/tests/ModelTest.php +++ b/tests/ModelTest.php @@ -971,4 +971,20 @@ public function testEnumCast(): void $this->assertSame(MemberStatus::Member->value, $check->getRawOriginal('member_status')); $this->assertSame(MemberStatus::Member, $check->member_status); } + + public function testNumericFieldName(): void + { + $user = new User(); + $user->{1} = 'one'; + $user->{2} = ['2' => 'two.two']; + $user->save(); + + $found = User::where(1, 'one')->first(); + $this->assertInstanceOf(User::class, $found); + $this->assertEquals('one', $found[1]); + + $found = User::where('2.2', 'two.two')->first(); + $this->assertInstanceOf(User::class, $found); + $this->assertEquals([2 => 'two.two'], $found[2]); + } } diff --git a/tests/Query/BuilderTest.php b/tests/Query/BuilderTest.php index 2bfc03515..667839c36 100644 --- a/tests/Query/BuilderTest.php +++ b/tests/Query/BuilderTest.php @@ -90,6 +90,11 @@ public static function provideQueryBuilderToMql(): iterable fn (Builder $builder) => $builder->where('foo', 'bar'), ]; + yield 'find with numeric field name' => [ + ['find' => [['123' => 'bar'], []]], + fn (Builder $builder) => $builder->where(123, 'bar'), + ]; + yield 'where with single array of conditions' => [ [ 'find' => [ @@ -1175,10 +1180,16 @@ public static function provideExceptions(): iterable yield 'find with single string argument' => [ ArgumentCountError::class, - 'Too few arguments to function MongoDB\Laravel\Query\Builder::where("foo"), 1 passed and at least 2 expected when the 1st is a string', + 'Too few arguments to function MongoDB\Laravel\Query\Builder::where(\'foo\'), 1 passed and at least 2 expected when the 1st is a scalar', fn (Builder $builder) => $builder->where('foo'), ]; + yield 'find with single numeric argument' => [ + ArgumentCountError::class, + 'Too few arguments to function MongoDB\Laravel\Query\Builder::where(123), 1 passed and at least 2 expected when the 1st is a scalar', + fn (Builder $builder) => $builder->where(123), + ]; + yield 'where regex not starting with /' => [ LogicException::class, 'Missing expected starting delimiter in regular expression "^ac/me$", supported delimiters are: / # ~', From 3fe1505a005b68740c6eef20d2509b2bd28cf9a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 18 Oct 2023 00:19:35 +0200 Subject: [PATCH 2/9] Fix incorrect reference --- src/Query/Builder.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Query/Builder.php b/src/Query/Builder.php index 945051c4b..de0d512ad 100644 --- a/src/Query/Builder.php +++ b/src/Query/Builder.php @@ -27,6 +27,7 @@ use function array_fill_keys; use function array_is_list; use function array_key_exists; +use function array_map; use function array_merge; use function array_values; use function array_walk_recursive; @@ -1006,9 +1007,7 @@ protected function compileWheres(): array if (isset($where['column']) && ($where['column'] === '_id' || str_ends_with($where['column'], '._id'))) { if (isset($where['values'])) { // Multiple values. - foreach ($where['values'] as &$value) { - $value = $this->convertKey($value); - } + $where['values'] = array_map($this->convertKey(...), $where['values']); } elseif (isset($where['value'])) { // Single value. $where['value'] = $this->convertKey($where['value']); From a1487d37c013aa40af2b7eb10ee472182ae50a46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 18 Oct 2023 00:29:13 +0200 Subject: [PATCH 3/9] Fix exception message --- src/Query/Builder.php | 2 +- tests/Query/BuilderTest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Query/Builder.php b/src/Query/Builder.php index de0d512ad..fc1cccce0 100644 --- a/src/Query/Builder.php +++ b/src/Query/Builder.php @@ -968,7 +968,7 @@ public function where($column, $operator = null, $value = null, $boolean = 'and' } if (func_num_args() === 1 && is_scalar($column)) { - throw new ArgumentCountError(sprintf('Too few arguments to function %s(%s), 1 passed and at least 2 expected when the 1st is a scalar.', __METHOD__, var_export($column, true))); + throw new ArgumentCountError(sprintf('Too few arguments to function %s(%s), 1 passed and at least 2 expected when the 1st is not an array.', __METHOD__, var_export($column, true))); } return parent::where(...$params); diff --git a/tests/Query/BuilderTest.php b/tests/Query/BuilderTest.php index 667839c36..c9b06a15b 100644 --- a/tests/Query/BuilderTest.php +++ b/tests/Query/BuilderTest.php @@ -1180,13 +1180,13 @@ public static function provideExceptions(): iterable yield 'find with single string argument' => [ ArgumentCountError::class, - 'Too few arguments to function MongoDB\Laravel\Query\Builder::where(\'foo\'), 1 passed and at least 2 expected when the 1st is a scalar', + 'Too few arguments to function MongoDB\Laravel\Query\Builder::where(\'foo\'), 1 passed and at least 2 expected when the 1st is not an array', fn (Builder $builder) => $builder->where('foo'), ]; yield 'find with single numeric argument' => [ ArgumentCountError::class, - 'Too few arguments to function MongoDB\Laravel\Query\Builder::where(123), 1 passed and at least 2 expected when the 1st is a scalar', + 'Too few arguments to function MongoDB\Laravel\Query\Builder::where(123), 1 passed and at least 2 expected when the 1st is not an array', fn (Builder $builder) => $builder->where(123), ]; From 3dfe5b7d5df374afa5e478205ba093c2356241c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 18 Oct 2023 13:18:38 +0200 Subject: [PATCH 4/9] Improve test on query numerical fields --- tests/ModelTest.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/ModelTest.php b/tests/ModelTest.php index 04f2abb51..7c40278c0 100644 --- a/tests/ModelTest.php +++ b/tests/ModelTest.php @@ -976,15 +976,19 @@ public function testNumericFieldName(): void { $user = new User(); $user->{1} = 'one'; - $user->{2} = ['2' => 'two.two']; + $user->{2} = ['3' => 'two.three']; $user->save(); $found = User::where(1, 'one')->first(); $this->assertInstanceOf(User::class, $found); $this->assertEquals('one', $found[1]); - $found = User::where('2.2', 'two.two')->first(); + $found = User::where('2.3', 'two.three')->first(); $this->assertInstanceOf(User::class, $found); - $this->assertEquals([2 => 'two.two'], $found[2]); + $this->assertEquals([3 => 'two.three'], $found[2]); + + $found = User::where(2.3, 'two.three')->first(); + $this->assertInstanceOf(User::class, $found); + $this->assertEquals([3 => 'two.three'], $found[2]); } } From 45c7c3c80ec76000318bf9121f4b36274c973c10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 18 Oct 2023 14:18:56 +0200 Subject: [PATCH 5/9] Fix where(array|callable) detection --- src/Query/Builder.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Query/Builder.php b/src/Query/Builder.php index fc1cccce0..a2f970d26 100644 --- a/src/Query/Builder.php +++ b/src/Query/Builder.php @@ -45,8 +45,8 @@ use function implode; use function in_array; use function is_array; +use function is_callable; use function is_int; -use function is_scalar; use function is_string; use function md5; use function preg_match; @@ -967,8 +967,8 @@ public function where($column, $operator = null, $value = null, $boolean = 'and' } } - if (func_num_args() === 1 && is_scalar($column)) { - throw new ArgumentCountError(sprintf('Too few arguments to function %s(%s), 1 passed and at least 2 expected when the 1st is not an array.', __METHOD__, var_export($column, true))); + if (func_num_args() === 1 && ! is_array($column) && ! is_callable($column)) { + throw new ArgumentCountError(sprintf('Too few arguments to function %s(%s), 1 passed and at least 2 expected when the 1st is not an array or a callable', __METHOD__, var_export($column, true))); } return parent::where(...$params); From 76be4ca74d90d0dd380b9a4cbce7acb4ebf041a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 18 Oct 2023 22:33:43 +0200 Subject: [PATCH 6/9] move cast --- src/Eloquent/Model.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Eloquent/Model.php b/src/Eloquent/Model.php index 666ca9c90..30497ad86 100644 --- a/src/Eloquent/Model.php +++ b/src/Eloquent/Model.php @@ -209,6 +209,8 @@ protected function getAttributeFromArray($key) /** @inheritdoc */ public function setAttribute($key, $value) { + $key = (string) $key; + // Convert _id to ObjectID. if ($key === '_id' && is_string($value)) { $builder = $this->newBaseQueryBuilder(); @@ -216,8 +218,6 @@ public function setAttribute($key, $value) $value = $builder->convertKey($value); } - $key = (string) $key; - // Support keys in dot notation. if (str_contains($key, '.')) { // Store to a temporary key, then move data to the actual key From f4e3f1b2f3c4b9ead0fc610f7b6b16c376354ce1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Thu, 19 Oct 2023 10:59:42 +0200 Subject: [PATCH 7/9] Reject float as column name --- src/Query/Builder.php | 4 ++++ tests/ModelTest.php | 4 ---- tests/Query/BuilderTest.php | 6 ++++++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Query/Builder.php b/src/Query/Builder.php index a2f970d26..cd39070dc 100644 --- a/src/Query/Builder.php +++ b/src/Query/Builder.php @@ -971,6 +971,10 @@ public function where($column, $operator = null, $value = null, $boolean = 'and' throw new ArgumentCountError(sprintf('Too few arguments to function %s(%s), 1 passed and at least 2 expected when the 1st is not an array or a callable', __METHOD__, var_export($column, true))); } + if (! is_int($column) && ! is_string($column)) { + throw new InvalidArgumentException(sprintf('First argument of %s must be a column name as "string". Got "%s"', __METHOD__, get_debug_type($column))); + } + return parent::where(...$params); } diff --git a/tests/ModelTest.php b/tests/ModelTest.php index 7c40278c0..ef25ebaef 100644 --- a/tests/ModelTest.php +++ b/tests/ModelTest.php @@ -986,9 +986,5 @@ public function testNumericFieldName(): void $found = User::where('2.3', 'two.three')->first(); $this->assertInstanceOf(User::class, $found); $this->assertEquals([3 => 'two.three'], $found[2]); - - $found = User::where(2.3, 'two.three')->first(); - $this->assertInstanceOf(User::class, $found); - $this->assertEquals([3 => 'two.three'], $found[2]); } } diff --git a/tests/Query/BuilderTest.php b/tests/Query/BuilderTest.php index c9b06a15b..d8cb9feb2 100644 --- a/tests/Query/BuilderTest.php +++ b/tests/Query/BuilderTest.php @@ -1219,6 +1219,12 @@ public static function provideExceptions(): iterable 'Invalid time format, expected HH:MM:SS, HH:MM or HH, got "stdClass"', fn (Builder $builder) => $builder->whereTime('created_at', new stdClass()), ]; + + yield 'where invalid column type' => [ + InvalidArgumentException::class, + 'First argument of MongoDB\Laravel\Query\Builder::where must be a column name as "string". Got "float"', + fn (Builder $builder) => $builder->where(2.3, '>', 1), + ]; } /** @dataProvider getEloquentMethodsNotSupported */ From 5a7e10873acf0855e9eb7e3d43ea9de542da729a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Thu, 19 Oct 2023 17:02:33 +0200 Subject: [PATCH 8/9] Add comment on Builder::where polymorphic signature --- src/Query/Builder.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/Query/Builder.php b/src/Query/Builder.php index cd39070dc..e1e771654 100644 --- a/src/Query/Builder.php +++ b/src/Query/Builder.php @@ -953,7 +953,20 @@ public function convertKey($id) return $id; } - /** @inheritdoc */ + /** + * Add a basic where clause to the query. + * + * If 1 argument, the signature is: where(array|Closure $where) + * If 2 arguments, the signature is: where(string $column, mixed $value) + * If 3 arguments, the signature is: where(string $colum, string $operator, mixed $value) + * + * @param Closure|string|array $column + * @param mixed $operator + * @param mixed $value + * @param string $boolean + * + * @return $this + */ public function where($column, $operator = null, $value = null, $boolean = 'and') { $params = func_get_args(); From d8f6e2a9a308fe9fd1a6d724f5cae2c5b63e3612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Thu, 19 Oct 2023 17:34:25 +0200 Subject: [PATCH 9/9] Revert column type detection, as the number of arguments is unreliable --- src/Query/Builder.php | 7 +++++-- tests/Query/BuilderTest.php | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/Query/Builder.php b/src/Query/Builder.php index e1e771654..cd2326dce 100644 --- a/src/Query/Builder.php +++ b/src/Query/Builder.php @@ -45,8 +45,11 @@ use function implode; use function in_array; use function is_array; +use function is_bool; use function is_callable; +use function is_float; use function is_int; +use function is_null; use function is_string; use function md5; use function preg_match; @@ -984,8 +987,8 @@ public function where($column, $operator = null, $value = null, $boolean = 'and' throw new ArgumentCountError(sprintf('Too few arguments to function %s(%s), 1 passed and at least 2 expected when the 1st is not an array or a callable', __METHOD__, var_export($column, true))); } - if (! is_int($column) && ! is_string($column)) { - throw new InvalidArgumentException(sprintf('First argument of %s must be a column name as "string". Got "%s"', __METHOD__, get_debug_type($column))); + if (is_float($column) || is_bool($column) || is_null($column)) { + throw new InvalidArgumentException(sprintf('First argument of %s must be a field path as "string". Got "%s"', __METHOD__, get_debug_type($column))); } return parent::where(...$params); diff --git a/tests/Query/BuilderTest.php b/tests/Query/BuilderTest.php index d8cb9feb2..1b3dcd2ad 100644 --- a/tests/Query/BuilderTest.php +++ b/tests/Query/BuilderTest.php @@ -1222,7 +1222,7 @@ public static function provideExceptions(): iterable yield 'where invalid column type' => [ InvalidArgumentException::class, - 'First argument of MongoDB\Laravel\Query\Builder::where must be a column name as "string". Got "float"', + 'First argument of MongoDB\Laravel\Query\Builder::where must be a field path as "string". Got "float"', fn (Builder $builder) => $builder->where(2.3, '>', 1), ]; }