From 837078f8660c4d2846e94f43b26c431741703b89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Thu, 5 Sep 2024 19:54:40 +0200 Subject: [PATCH] PHPORM-236 Remove _id from query results (#3136) --- CHANGELOG.md | 2 +- docs/includes/usage-examples/FindOneTest.php | 2 +- src/Query/Builder.php | 8 ++++---- tests/QueryBuilderTest.php | 20 +++++++++++++++++-- .../Failed/DatabaseFailedJobProviderTest.php | 8 +++++--- 5 files changed, 29 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f53f4c22..7a175e414 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. ## [5.0.0] - next * Remove support for Laravel 10 by @GromNaN in [#3123](https://github.com/mongodb/laravel-mongodb/pull/3123) -* **BREAKING CHANGE** Use `id` as an alias for `_id` in commands and queries for compatibility with Eloquent packages by @GromNaN in [#3040](https://github.com/mongodb/laravel-mongodb/pull/3040) +* **BREAKING CHANGE** Use `id` as an alias for `_id` in commands and queries for compatibility with Eloquent packages by @GromNaN in [#3040](https://github.com/mongodb/laravel-mongodb/pull/3040) and [#3136](https://github.com/mongodb/laravel-mongodb/pull/3136) * **BREAKING CHANGE** Make Query\Builder return objects instead of array to match Laravel behavior by @GromNaN in [#3107](https://github.com/mongodb/laravel-mongodb/pull/3107) * **BREAKING CHANGE** In DB query results, convert BSON `UTCDateTime` objects into `Carbon` date with the default timezone by @GromNaN in [#3119](https://github.com/mongodb/laravel-mongodb/pull/3119) * Remove `MongoFailedJobProvider`, replaced by Laravel `DatabaseFailedJobProvider` by @GromNaN in [#3122](https://github.com/mongodb/laravel-mongodb/pull/3122) diff --git a/docs/includes/usage-examples/FindOneTest.php b/docs/includes/usage-examples/FindOneTest.php index 8472727be..e46ba1be4 100644 --- a/docs/includes/usage-examples/FindOneTest.php +++ b/docs/includes/usage-examples/FindOneTest.php @@ -31,6 +31,6 @@ public function testFindOne(): void // end-find-one $this->assertInstanceOf(Movie::class, $movie); - $this->expectOutputRegex('/^{"_id":"[a-z0-9]{24}","title":"The Shawshank Redemption","directors":\["Frank Darabont","Rob Reiner"\],"id":"[a-z0-9]{24}"}$/'); + $this->expectOutputRegex('/^{"title":"The Shawshank Redemption","directors":\["Frank Darabont","Rob Reiner"\],"id":"[a-z0-9]{24}"}$/'); } } diff --git a/src/Query/Builder.php b/src/Query/Builder.php index f4f31b58f..e2f8867b3 100644 --- a/src/Query/Builder.php +++ b/src/Query/Builder.php @@ -1616,7 +1616,7 @@ public function orWhereIntegerNotInRaw($column, $values, $boolean = 'and') private function aliasIdForQuery(array $values): array { if (array_key_exists('id', $values)) { - if (array_key_exists('_id', $values)) { + if (array_key_exists('_id', $values) && $values['id'] !== $values['_id']) { throw new InvalidArgumentException('Cannot have both "id" and "_id" fields.'); } @@ -1627,7 +1627,7 @@ private function aliasIdForQuery(array $values): array foreach ($values as $key => $value) { if (is_string($key) && str_ends_with($key, '.id')) { $newkey = substr($key, 0, -3) . '._id'; - if (array_key_exists($newkey, $values)) { + if (array_key_exists($newkey, $values) && $value !== $values[$newkey]) { throw new InvalidArgumentException(sprintf('Cannot have both "%s" and "%s" fields.', $key, $newkey)); } @@ -1659,7 +1659,7 @@ private function aliasIdForResult(array|object $values): array|object if (is_array($values)) { if (array_key_exists('_id', $values) && ! array_key_exists('id', $values)) { $values['id'] = $values['_id']; - //unset($values['_id']); + unset($values['_id']); } foreach ($values as $key => $value) { @@ -1675,7 +1675,7 @@ private function aliasIdForResult(array|object $values): array|object if ($values instanceof stdClass) { if (property_exists($values, '_id') && ! property_exists($values, 'id')) { $values->id = $values->_id; - //unset($values->_id); + unset($values->_id); } foreach (get_object_vars($values) as $key => $value) { diff --git a/tests/QueryBuilderTest.php b/tests/QueryBuilderTest.php index 846f48514..910adecfc 100644 --- a/tests/QueryBuilderTest.php +++ b/tests/QueryBuilderTest.php @@ -449,18 +449,33 @@ public function testDistinct() public function testCustomId() { + $tags = [['id' => 'sharp', 'name' => 'Sharp']]; DB::table('items')->insert([ - ['id' => 'knife', 'type' => 'sharp', 'amount' => 34], - ['id' => 'fork', 'type' => 'sharp', 'amount' => 20], + ['id' => 'knife', 'type' => 'sharp', 'amount' => 34, 'tags' => $tags], + ['id' => 'fork', 'type' => 'sharp', 'amount' => 20, 'tags' => $tags], ['id' => 'spoon', 'type' => 'round', 'amount' => 3], ]); $item = DB::table('items')->find('knife'); $this->assertEquals('knife', $item->id); + $this->assertObjectNotHasProperty('_id', $item); + $this->assertEquals('sharp', $item->tags[0]['id']); + $this->assertArrayNotHasKey('_id', $item->tags[0]); $item = DB::table('items')->where('id', 'fork')->first(); $this->assertEquals('fork', $item->id); + $item = DB::table('items')->where('_id', 'fork')->first(); + $this->assertEquals('fork', $item->id); + + // tags.id is translated into tags._id in query + $items = DB::table('items')->whereIn('tags.id', ['sharp'])->get(); + $this->assertCount(2, $items); + + // Ensure the field _id is stored in the database + $items = DB::table('items')->whereIn('tags._id', ['sharp'])->get(); + $this->assertCount(2, $items); + DB::table('users')->insert([ ['id' => 1, 'name' => 'Jane Doe'], ['id' => 2, 'name' => 'John Doe'], @@ -468,6 +483,7 @@ public function testCustomId() $item = DB::table('users')->find(1); $this->assertEquals(1, $item->id); + $this->assertObjectNotHasProperty('_id', $item); } public function testTake() diff --git a/tests/Queue/Failed/DatabaseFailedJobProviderTest.php b/tests/Queue/Failed/DatabaseFailedJobProviderTest.php index 88a7f0e7b..01f38b9df 100644 --- a/tests/Queue/Failed/DatabaseFailedJobProviderTest.php +++ b/tests/Queue/Failed/DatabaseFailedJobProviderTest.php @@ -77,8 +77,9 @@ public function testAll(): void $all = $this->getProvider()->all(); $this->assertCount(5, $all); - $this->assertEquals(new ObjectId(sprintf('%024d', 5)), $all[0]->_id); - $this->assertEquals(sprintf('%024d', 5), $all[0]->id, 'id field is added for compatibility with DatabaseFailedJobProvider'); + $this->assertInstanceOf(ObjectId::class, $all[0]->id); + $this->assertEquals(new ObjectId(sprintf('%024d', 5)), $all[0]->id); + $this->assertEquals(sprintf('%024d', 5), (string) $all[0]->id, 'id field is added for compatibility with DatabaseFailedJobProvider'); } public function testFindAndForget(): void @@ -89,7 +90,8 @@ public function testFindAndForget(): void $found = $provider->find($id); $this->assertIsObject($found, 'The job is found'); - $this->assertEquals(new ObjectId($id), $found->_id); + $this->assertInstanceOf(ObjectId::class, $found->id); + $this->assertEquals(new ObjectId($id), $found->id); $this->assertObjectHasProperty('failed_at', $found); // Delete the job