PHPORM-236 Remove _id from query results (#3136)

mongodb · Sep 5, 2024 · 837078f · 837078f
1 parent 807f5fa
commit 837078f
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -449,25 +449,41 @@ 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'],
         ]);
 
         $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
@@ -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