Merge pull request #27394 from rodrigopedra/resource

[5.8] Fix how resources handle arrays with mixed numeric and string keys
laravel · Feb 7, 2019 · e456e7f · e456e7f
2 parents aabded0 + c1984c9
commit e456e7f
Show file tree

Hide file tree

Showing 3 changed files with 121 additions and 16 deletions.
diff --git a/src/Illuminate/Http/Resources/ConditionallyLoadsAttributes.php b/src/Illuminate/Http/Resources/ConditionallyLoadsAttributes.php
@@ -16,8 +16,6 @@ protected function filter($data)
     {
         $index = -1;
 
-        $numericKeys = array_values($data) === $data;
-
         foreach ($data as $key => $value) {
             $index++;
 
@@ -28,15 +26,18 @@ protected function filter($data)
             }
 
             if (is_numeric($key) && $value instanceof MergeValue) {
-                return $this->mergeData($data, $index, $this->filter($value->data), $numericKeys);
+                return $this->mergeData(
+                    $data, $index, $this->filter($value->data),
+                    array_values($value->data) === $value->data
+                );
             }
 
             if ($value instanceof self && is_null($value->resource)) {
                 $data[$key] = null;
             }
         }
 
-        return $this->removeMissingValues($data, $numericKeys);
+        return $this->removeMissingValues($data);
     }
 
     /**
@@ -54,7 +55,7 @@ protected function mergeData($data, $index, $merge, $numericKeys)
             return $this->removeMissingValues(array_merge(
                 array_merge(array_slice($data, 0, $index, true), $merge),
                 $this->filter(array_values(array_slice($data, $index + 1, null, true)))
-            ), $numericKeys);
+            ));
         }
 
         return $this->removeMissingValues(array_slice($data, 0, $index, true) +
@@ -66,22 +67,28 @@ protected function mergeData($data, $index, $merge, $numericKeys)
      * Remove the missing values from the filtered data.
      *
      * @param  array  $data
-     * @param  bool  $numericKeys
      * @return array
      */
-    protected function removeMissingValues($data, $numericKeys = false)
+    protected function removeMissingValues($data)
     {
+        $numericKeys = true;
+
         foreach ($data as $key => $value) {
             if (($value instanceof PotentiallyMissing && $value->isMissing()) ||
                 ($value instanceof self &&
                 $value->resource instanceof PotentiallyMissing &&
                 $value->isMissing())) {
                 unset($data[$key]);
+            } else {
+                $numericKeys = $numericKeys && is_numeric($key);
             }
         }
 
-        return ! empty($data) && is_numeric(array_keys($data)[0])
-                        ? array_values($data) : $data;
+        if (property_exists($this, 'preserveKeys') && $this->preserveKeys === true) {
+            return $data;
+        }
+
+        return $numericKeys ? array_values($data) : $data;
     }
 
     /**

diff --git a/tests/Integration/Http/Fixtures/ResourceWithPreservedKeys.php b/tests/Integration/Http/Fixtures/ResourceWithPreservedKeys.php
@@ -0,0 +1,13 @@
+<?php
+
+namespace Illuminate\Tests\Integration\Http\Fixtures;
+
+class ResourceWithPreservedKeys extends PostResource
+{
+    protected $preserveKeys = true;
+
+    public function toArray($request)
+    {
+        return $this->resource;
+    }
+}
diff --git a/tests/Integration/Http/ResourceTest.php b/tests/Integration/Http/ResourceTest.php
@@ -5,6 +5,7 @@
 use Orchestra\Testbench\TestCase;
 use Illuminate\Support\Facades\Route;
 use Illuminate\Http\Resources\MergeValue;
+use Illuminate\Http\Resources\MissingValue;
 use Illuminate\Pagination\LengthAwarePaginator;
 use Illuminate\Http\Resources\Json\JsonResource;
 use Illuminate\Tests\Integration\Http\Fixtures\Post;
@@ -17,6 +18,7 @@
 use Illuminate\Tests\Integration\Http\Fixtures\ReallyEmptyPostResource;
 use Illuminate\Tests\Integration\Http\Fixtures\SerializablePostResource;
 use Illuminate\Tests\Integration\Http\Fixtures\PostResourceWithExtraData;
+use Illuminate\Tests\Integration\Http\Fixtures\ResourceWithPreservedKeys;
 use Illuminate\Tests\Integration\Http\Fixtures\EmptyPostCollectionResource;
 use Illuminate\Tests\Integration\Http\Fixtures\PostResourceWithOptionalData;
 use Illuminate\Tests\Integration\Http\Fixtures\PostResourceWithOptionalMerging;
@@ -589,7 +591,45 @@ public function test_collection_resources_are_countable()
         $this->assertSame(2, count($collection));
     }
 
-    public function test_leading_merge__keyed_value_is_merged_correctly()
+    public function test_keys_are_preserved_if_the_resource_is_flagged_to_preserve_keys()
+    {
+        $data = [
+            'authorBook' => [
+                'byId' => [
+                    1 => [
+                        'id' => 1,
+                        'authorId' => 5,
+                        'bookId' => 22,
+                    ],
+                    2 => [
+                        'id' => 2,
+                        'authorId' => 5,
+                        'bookId' => 15,
+                    ],
+                    3 => [
+                        'id' => 3,
+                        'authorId' => 42,
+                        'bookId' => 12,
+                    ],
+                ],
+                'allIds' => [1, 2, 3],
+            ],
+        ];
+
+        Route::get('/', function () use ($data) {
+            return new ResourceWithPreservedKeys($data);
+        });
+
+        $response = $this->withoutExceptionHandling()->get(
+            '/', ['Accept' => 'application/json']
+        );
+
+        $response->assertStatus(200);
+
+        $response->assertJson(['data' => $data]);
+    }
+
+    public function test_leading_merge_keyed_value_is_merged_correctly()
     {
         $filter = new class {
             use ConditionallyLoadsAttributes;
@@ -609,6 +649,30 @@ public function work()
         ], $results);
     }
 
+    public function test_leading_merge_keyed_value_is_merged_correctly_when_first_value_is_missing()
+    {
+        $filter = new class {
+            use ConditionallyLoadsAttributes;
+
+            public function work()
+            {
+                return $this->filter([
+                    new MergeValue([
+                        0 => new MissingValue,
+                        'name' => 'mohamed',
+                        'location' => 'hurghada',
+                    ]),
+                ]);
+            }
+        };
+
+        $results = $filter->work();
+
+        $this->assertEquals([
+            'name' => 'mohamed', 'location' => 'hurghada',
+        ], $results);
+    }
+
     public function test_leading_merge_value_is_merged_correctly()
     {
         $filter = new class {
@@ -814,6 +878,27 @@ public function test_the_resource_can_be_an_array()
         ]);
     }
 
+    public function test_it_will_return_as_an_array_when_string_keys_are_stripped()
+    {
+        $this->assertJsonResourceResponse([
+            1 => 'John',
+            2 => 'Hank',
+            'foo' => new MissingValue,
+        ], ['data' => ['John', 'Hank']]);
+
+        $this->assertJsonResourceResponse([
+            1 => 'John',
+            'foo' => new MissingValue,
+            3 => 'Hank',
+        ], ['data' => ['John', 'Hank']]);
+
+        $this->assertJsonResourceResponse([
+            'foo' => new MissingValue,
+            2 => 'John',
+            3 => 'Hank',
+        ], ['data' => ['John', 'Hank']]);
+    }
+
     public function test_it_strips_numeric_keys()
     {
         $this->assertJsonResourceResponse([
@@ -833,19 +918,19 @@ public function test_it_strips_numeric_keys()
         ], ['data' => ['John', 'Hank']]);
     }
 
-    public function test_it_strips_all_keys_if_any_of_them_are_numeric()
+    public function test_it_wont_keys_if_any_of_them_are_strings()
     {
         $this->assertJsonResourceResponse([
             '5' => 'John',
             '6' => 'Hank',
             'a' => 'Bill',
-        ], ['data' => ['John', 'Hank', 'Bill']]);
+        ], ['data' => ['5' => 'John', '6' => 'Hank', 'a' => 'Bill']]);
 
         $this->assertJsonResourceResponse([
-            5 => 'John',
-            6 => 'Hank',
-            'a' => 'Bill',
-        ], ['data' => ['John', 'Hank', 'Bill']]);
+            0 => 10,
+            1 => 20,
+            'total' => 30,
+        ], ['data' => [0 => 10, 1 => 20, 'total' => 30]]);
     }
 
     private function assertJsonResourceResponse($data, $expectedJson)