From f217dd5e09d24c7d63478f27d84d93ba64477da3 Mon Sep 17 00:00:00 2001 From: Samuel Levy Date: Mon, 12 Aug 2024 12:16:48 +1000 Subject: [PATCH 1/6] [11.x] Added failing test for serializing circular relations If a circular relationship is set up between two models using `setRelation()` (or similar methods) then calling `$model->relationsToArray()` will call `toArray()` on each related model, which will in turn call `relationsToArray()`. In an instance where one of the related models is an object that has already had `toArray()` called further up the stack, it will infinitely recurse down and result in a stack overflow. The same issue exists with `getQueueableRelations()`, `push()`, and potentially other methods. This adds tests which will fail if one of the known potentially problematic methods gets into a recursive loop. --- tests/Database/DatabaseEloquentModelTest.php | 187 +++++++++++++++++++ 1 file changed, 187 insertions(+) diff --git a/tests/Database/DatabaseEloquentModelTest.php b/tests/Database/DatabaseEloquentModelTest.php index 11757e80384b..d9f73265ffb0 100755 --- a/tests/Database/DatabaseEloquentModelTest.php +++ b/tests/Database/DatabaseEloquentModelTest.php @@ -1156,6 +1156,28 @@ public function testPushManyRelation() $this->assertEquals([2, 3], $model->relationMany->pluck('id')->all()); } + public function testPushCircularRelations() + { + $parent = new EloquentModelWithRecursiveRelationshipsStub(['id' => 1, 'parent_id' => null]); + $lastId = $parent->id; + $parent->setRelation('self', $parent); + + $children = new Collection(); + for ($count = 0; $count < 2; $count++) { + $child = new EloquentModelWithRecursiveRelationshipsStub(['id' => ++$lastId, 'parent_id' => $parent->id]); + $child->setRelation('parent', $parent); + $child->setRelation('self', $child); + $children->push($child); + } + $parent->setRelation('children', $children); + + try { + $this->assertTrue($parent->push()); + } catch (\RuntimeException $e) { + $this->fail($e->getMessage()); + } + } + public function testNewQueryReturnsEloquentQueryBuilder() { $conn = m::mock(Connection::class); @@ -1231,6 +1253,79 @@ public function testToArray() $this->assertSame('appended', $array['appendable']); } + public function testToArrayWithCircularRelations() + { + $parent = new EloquentModelWithRecursiveRelationshipsStub(['id' => 1, 'parent_id' => null]); + $lastId = $parent->id; + $parent->setRelation('self', $parent); + + $children = new Collection(); + for ($count = 0; $count < 2; $count++) { + $child = new EloquentModelWithRecursiveRelationshipsStub(['id' => ++$lastId, 'parent_id' => $parent->id]); + $child->setRelation('parent', $parent); + $child->setRelation('self', $child); + $children->push($child); + } + $parent->setRelation('children', $children); + + try { + $this->assertSame( + [ + 'id' => 1, + 'parent_id' => null, + 'self' => ['id' => 1, 'parent_id' => null], + 'children' => [ + [ + 'id' => 2, + 'parent_id' => 1, + 'parent' => ['id' => 1, 'parent_id' => null], + 'self' => ['id' => 2, 'parent_id' => 1], + ], + [ + 'id' => 3, + 'parent_id' => 1, + 'parent' => ['id' => 1, 'parent_id' => null], + 'self' => ['id' => 3, 'parent_id' => 1], + ], + ], + ], + $parent->toArray() + ); + } catch (\RuntimeException $e) { + $this->fail($e->getMessage()); + } + } + + public function testGetQueueableRelationsWithCircularRelations() + { + $parent = new EloquentModelWithRecursiveRelationshipsStub(['id' => 1, 'parent_id' => null]); + $lastId = $parent->id; + $parent->setRelation('self', $parent); + + $children = new Collection(); + for ($count = 0; $count < 2; $count++) { + $child = new EloquentModelWithRecursiveRelationshipsStub(['id' => ++$lastId, 'parent_id' => $parent->id]); + $child->setRelation('parent', $parent); + $child->setRelation('self', $child); + $children->push($child); + } + $parent->setRelation('children', $children); + + try { + $this->assertSame( + [ + 'self', + 'children', + 'children.parent', + 'children.self', + ], + $parent->getQueueableRelations() + ); + } catch (\RuntimeException $e) { + $this->fail($e->getMessage()); + } + } + public function testVisibleCreatesArrayWhitelist() { $model = new EloquentModelStub; @@ -3683,3 +3778,95 @@ public function set(Model $model, string $key, mixed $value, array $attributes): }; } } + +class EloquentModelWithRecursiveRelationshipsStub extends Model +{ + public $fillable = ['id', 'parent_id']; + + protected static \WeakMap $recursionDetectionCache; + + public function getQueueableRelations() + { + try { + $this->stepIn(); + + return parent::getQueueableRelations(); + } finally { + $this->stepOut(); + } + } + + public function push() + { + try { + $this->stepIn(); + + return parent::push(); + } finally { + $this->stepOut(); + } + } + + public function save(array $options = []) + { + return true; + } + + public function relationsToArray() + { + try { + $this->stepIn(); + + return parent::relationsToArray(); + } finally { + $this->stepOut(); + } + } + + public function parent(): BelongsTo + { + return $this->belongsTo(static::class, 'parent_id'); + } + + public function children(): HasMany + { + return $this->hasMany(static::class, 'parent_id'); + } + + public function self(): BelongsTo + { + return $this->belongsTo(static::class, 'id'); + } + + protected static function getRecursionDetectionCache() + { + return static::$recursionDetectionCache ??= new \WeakMap; + } + + protected function getRecursionDepth(): int + { + $cache = static::getRecursionDetectionCache(); + + return $cache->offsetExists($this) ? $cache->offsetGet($this) : 0; + } + + protected function stepIn(): void + { + $depth = $this->getRecursionDepth(); + + if ($depth > 1) { + throw new \RuntimeException('Recursion detected'); + } + static::getRecursionDetectionCache()->offsetSet($this, $depth + 1); + } + + protected function stepOut(): void + { + $cache = static::getRecursionDetectionCache(); + if ($depth = $this->getRecursionDepth()) { + $cache->offsetSet($this, $depth - 1); + } else { + $cache->offsetUnset($this); + } + } +} From 1e175f49ab49d042bbc0fe1a4b35a15866dd9940 Mon Sep 17 00:00:00 2001 From: Samuel Levy Date: Mon, 12 Aug 2024 16:22:30 +1000 Subject: [PATCH 2/6] [11.x] Added PreventsCircularRecursion This adds a trait for Eloquent which can be used to prevent recursively serializing circular references. --- .../Concerns/PreventsCircularRecursion.php | 74 ++++++++ src/Illuminate/Database/Eloquent/Model.php | 70 ++++---- ...eConcernsPreventsCircularRecursionTest.php | 162 ++++++++++++++++++ 3 files changed, 275 insertions(+), 31 deletions(-) create mode 100644 src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php create mode 100644 tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php diff --git a/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php b/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php new file mode 100644 index 000000000000..8c06c5351579 --- /dev/null +++ b/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php @@ -0,0 +1,74 @@ +> + */ + protected static $recursionCache; + + /** + * Get the current recursion cache being used by the model. + * + * @return \WeakMap + */ + protected static function getRecursionCache() + { + return static::$recursionCache ??= new \WeakMap(); + } + + /** + * Get the current stack of methods being called recursively. + * + * @param object $object + * @return array + */ + protected static function getRecursiveCallStack($object): array + { + return static::getRecursionCache()->offsetExists($object) + ? static::getRecursionCache()->offsetGet($object) + : []; + } + + /** + * Prevent a method from being called multiple times on the same object within the same call stack. + * + * @param callable $callback + * @param mixed $default + * @return mixed + */ + protected function once($callback, $default = null) + { + $trace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT, 2); + + $onceable = Onceable::tryFromTrace($trace, $callback); + + $object = $onceable->object ?? $this; + $stack = static::getRecursiveCallStack($object); + + if (isset($stack[$onceable->hash])) { + return $stack[$onceable->hash]; + } + + try { + // Set the default first to prevent recursion + $stack[$onceable->hash] = $default; + static::getRecursionCache()->offsetSet($object, $stack); + + return call_user_func($onceable->callable); + } finally { + if ($stack = Arr::except($this->getRecursiveCallStack($object), $onceable->hash)) { + static::getRecursionCache()->offsetSet($object, $stack); + } elseif (static::getRecursionCache()->offsetExists($object)) { + static::getRecursionCache()->offsetUnset($object); + } + } + } +} diff --git a/src/Illuminate/Database/Eloquent/Model.php b/src/Illuminate/Database/Eloquent/Model.php index 59b81aeed178..5ad8f7a3ce2f 100644 --- a/src/Illuminate/Database/Eloquent/Model.php +++ b/src/Illuminate/Database/Eloquent/Model.php @@ -35,6 +35,7 @@ abstract class Model implements Arrayable, ArrayAccess, CanBeEscapedWhenCastToSt Concerns\HasUniqueIds, Concerns\HidesAttributes, Concerns\GuardsAttributes, + Concerns\PreventsCircularRecursion, ForwardsCalls; /** @use HasCollection<\Illuminate\Database\Eloquent\Collection> */ use HasCollection; @@ -1083,25 +1084,27 @@ protected function decrementQuietly($column, $amount = 1, array $extra = []) */ public function push() { - if (! $this->save()) { - return false; - } - - // To sync all of the relationships to the database, we will simply spin through - // the relationships and save each model via this "push" method, which allows - // us to recurse into all of these nested relations for the model instance. - foreach ($this->relations as $models) { - $models = $models instanceof Collection - ? $models->all() : [$models]; + return $this->once(function () { + if (! $this->save()) { + return false; + } - foreach (array_filter($models) as $model) { - if (! $model->push()) { - return false; + // To sync all of the relationships to the database, we will simply spin through + // the relationships and save each model via this "push" method, which allows + // us to recurse into all of these nested relations for the model instance. + foreach ($this->relations as $models) { + $models = $models instanceof Collection + ? $models->all() : [$models]; + + foreach (array_filter($models) as $model) { + if (! $model->push()) { + return false; + } } } - } - return true; + return true; + }, true); } /** @@ -1644,7 +1647,10 @@ public function callNamedScope($scope, array $parameters = []) */ public function toArray() { - return array_merge($this->attributesToArray(), $this->relationsToArray()); + return $this->once( + fn () => array_merge($this->attributesToArray(), $this->relationsToArray()), + $this->attributesToArray(), + ); } /** @@ -1991,29 +1997,31 @@ public function getQueueableId() */ public function getQueueableRelations() { - $relations = []; + return $this->once(function () { + $relations = []; - foreach ($this->getRelations() as $key => $relation) { - if (! method_exists($this, $key)) { - continue; - } + foreach ($this->getRelations() as $key => $relation) { + if (! method_exists($this, $key)) { + continue; + } - $relations[] = $key; + $relations[] = $key; - if ($relation instanceof QueueableCollection) { - foreach ($relation->getQueueableRelations() as $collectionValue) { - $relations[] = $key.'.'.$collectionValue; + if ($relation instanceof QueueableCollection) { + foreach ($relation->getQueueableRelations() as $collectionValue) { + $relations[] = $key.'.'.$collectionValue; + } } - } - if ($relation instanceof QueueableEntity) { - foreach ($relation->getQueueableRelations() as $entityValue) { - $relations[] = $key.'.'.$entityValue; + if ($relation instanceof QueueableEntity) { + foreach ($relation->getQueueableRelations() as $entityValue) { + $relations[] = $key.'.'.$entityValue; + } } } - } - return array_unique($relations); + return array_unique($relations); + }, []); } /** diff --git a/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php b/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php new file mode 100644 index 000000000000..3c3cc8fa46df --- /dev/null +++ b/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php @@ -0,0 +1,162 @@ +assertEquals(0, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(0, $instance->instanceStack); + + $this->assertEquals(0, $instance->callStack()); + $this->assertEquals(1, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(1, $instance->instanceStack); + + $this->assertEquals(1, $instance->callStack()); + $this->assertEquals(2, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(2, $instance->instanceStack); + } + + public function testRecursiveCallsAreLimitedToIndividualInstances() + { + $instance = new PreventsCircularRecursionWithRecursiveMethod(); + $other = $instance->other; + + $this->assertEquals(0, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(0, $instance->instanceStack); + $this->assertEquals(0, $other->instanceStack); + + $instance->callStack(); + $this->assertEquals(1, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(1, $instance->instanceStack); + $this->assertEquals(0, $other->instanceStack); + + $instance->callStack(); + $this->assertEquals(2, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(2, $instance->instanceStack); + $this->assertEquals(0, $other->instanceStack); + + $other->callStack(); + $this->assertEquals(3, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(2, $instance->instanceStack); + $this->assertEquals(1, $other->instanceStack); + + $other->callStack(); + $this->assertEquals(4, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(2, $instance->instanceStack); + $this->assertEquals(2, $other->instanceStack); + } + + public function testRecursiveCallsToCircularReferenceCallsOtherInstanceOnce() + { + $instance = new PreventsCircularRecursionWithRecursiveMethod(); + $other = $instance->other; + + $this->assertEquals(0, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(0, $instance->instanceStack); + $this->assertEquals(0, $other->instanceStack); + + $instance->callOtherStack(); + $this->assertEquals(2, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(1, $instance->instanceStack); + $this->assertEquals(1, $other->instanceStack); + + $instance->callOtherStack(); + $this->assertEquals(4, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(2, $instance->instanceStack); + $this->assertEquals(2, $other->instanceStack); + + $other->callOtherStack(); + $this->assertEquals(6, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(3, $other->instanceStack); + $this->assertEquals(3, $instance->instanceStack); + + $other->callOtherStack(); + $this->assertEquals(8, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(4, $other->instanceStack); + $this->assertEquals(4, $instance->instanceStack); + } + + public function testRecursiveCallsToCircularLinkedListCallsEachInstanceOnce() + { + $instance = new PreventsCircularRecursionWithRecursiveMethod(); + $second = $instance->other; + $third = new PreventsCircularRecursionWithRecursiveMethod($second); + $instance->other = $third; + + $this->assertEquals(0, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(0, $instance->instanceStack); + $this->assertEquals(0, $second->instanceStack); + $this->assertEquals(0, $third->instanceStack); + + $instance->callOtherStack(); + $this->assertEquals(3, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(1, $instance->instanceStack); + $this->assertEquals(1, $second->instanceStack); + $this->assertEquals(1, $third->instanceStack); + + $second->callOtherStack(); + $this->assertEquals(6, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(2, $instance->instanceStack); + $this->assertEquals(2, $second->instanceStack); + $this->assertEquals(2, $third->instanceStack); + + $third->callOtherStack(); + $this->assertEquals(9, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(3, $instance->instanceStack); + $this->assertEquals(3, $second->instanceStack); + $this->assertEquals(3, $third->instanceStack); + } +} + +class PreventsCircularRecursionWithRecursiveMethod +{ + use PreventsCircularRecursion; + + public function __construct( + public ?PreventsCircularRecursionWithRecursiveMethod $other = null, + ) { + $this->other ??= new PreventsCircularRecursionWithRecursiveMethod($this); + } + + public static int $globalStack = 0; + public int $instanceStack = 0; + + public function callStack(): int + { + return $this->once( + function () { + static::$globalStack++; + $this->instanceStack++; + + return $this->callStack(); + }, + $this->instanceStack + ); + } + + public function callOtherStack(): int + { + return $this->once( + function () { + $this->other->callStack(); + + return $this->other->callOtherStack(); + }, + $this->instanceStack + ); + } +} From 05cdf3b15d24c82cee48b3b0e3c6468b05257a24 Mon Sep 17 00:00:00 2001 From: Samuel Levy Date: Tue, 13 Aug 2024 19:43:15 +1000 Subject: [PATCH 3/6] [11.x] Changed the name to `withoutRecursion()`, accept a callable default --- .../Eloquent/Concerns/PreventsCircularRecursion.php | 7 +++---- src/Illuminate/Database/Eloquent/Model.php | 8 ++++---- .../DatabaseConcernsPreventsCircularRecursionTest.php | 4 ++-- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php b/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php index 8c06c5351579..67d1007a05ec 100644 --- a/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php +++ b/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php @@ -44,7 +44,7 @@ protected static function getRecursiveCallStack($object): array * @param mixed $default * @return mixed */ - protected function once($callback, $default = null) + protected function withoutRecursion($callback, $default = null) { $trace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT, 2); @@ -53,13 +53,12 @@ protected function once($callback, $default = null) $object = $onceable->object ?? $this; $stack = static::getRecursiveCallStack($object); - if (isset($stack[$onceable->hash])) { + if (array_key_exists($onceable->hash, $stack)) { return $stack[$onceable->hash]; } try { - // Set the default first to prevent recursion - $stack[$onceable->hash] = $default; + $stack[$onceable->hash] = is_callable($default) ? call_user_func($default) : $default; static::getRecursionCache()->offsetSet($object, $stack); return call_user_func($onceable->callable); diff --git a/src/Illuminate/Database/Eloquent/Model.php b/src/Illuminate/Database/Eloquent/Model.php index 5ad8f7a3ce2f..69a8a3b07fe8 100644 --- a/src/Illuminate/Database/Eloquent/Model.php +++ b/src/Illuminate/Database/Eloquent/Model.php @@ -1084,7 +1084,7 @@ protected function decrementQuietly($column, $amount = 1, array $extra = []) */ public function push() { - return $this->once(function () { + return $this->withoutRecursion(function () { if (! $this->save()) { return false; } @@ -1647,9 +1647,9 @@ public function callNamedScope($scope, array $parameters = []) */ public function toArray() { - return $this->once( + return $this->withoutRecursion( fn () => array_merge($this->attributesToArray(), $this->relationsToArray()), - $this->attributesToArray(), + fn () => $this->attributesToArray(), ); } @@ -1997,7 +1997,7 @@ public function getQueueableId() */ public function getQueueableRelations() { - return $this->once(function () { + return $this->withoutRecursion(function () { $relations = []; foreach ($this->getRelations() as $key => $relation) { diff --git a/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php b/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php index 3c3cc8fa46df..057c4375422b 100644 --- a/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php +++ b/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php @@ -137,7 +137,7 @@ public function __construct( public function callStack(): int { - return $this->once( + return $this->withoutRecursion( function () { static::$globalStack++; $this->instanceStack++; @@ -150,7 +150,7 @@ function () { public function callOtherStack(): int { - return $this->once( + return $this->withoutRecursion( function () { $this->other->callStack(); From dc3dc5db4bde21c6a7b40dac0af5a94389891112 Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Wed, 21 Aug 2024 22:37:09 -0500 Subject: [PATCH 4/6] formatting --- .../Concerns/PreventsCircularRecursion.php | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php b/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php index 67d1007a05ec..bd26c1d6c0ac 100644 --- a/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php +++ b/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php @@ -4,6 +4,7 @@ use Illuminate\Support\Arr; use Illuminate\Support\Onceable; +use WeakMap; trait PreventsCircularRecursion { @@ -14,29 +15,6 @@ trait PreventsCircularRecursion */ protected static $recursionCache; - /** - * Get the current recursion cache being used by the model. - * - * @return \WeakMap - */ - protected static function getRecursionCache() - { - return static::$recursionCache ??= new \WeakMap(); - } - - /** - * Get the current stack of methods being called recursively. - * - * @param object $object - * @return array - */ - protected static function getRecursiveCallStack($object): array - { - return static::getRecursionCache()->offsetExists($object) - ? static::getRecursionCache()->offsetGet($object) - : []; - } - /** * Prevent a method from being called multiple times on the same object within the same call stack. * @@ -51,6 +29,7 @@ protected function withoutRecursion($callback, $default = null) $onceable = Onceable::tryFromTrace($trace, $callback); $object = $onceable->object ?? $this; + $stack = static::getRecursiveCallStack($object); if (array_key_exists($onceable->hash, $stack)) { @@ -59,6 +38,7 @@ protected function withoutRecursion($callback, $default = null) try { $stack[$onceable->hash] = is_callable($default) ? call_user_func($default) : $default; + static::getRecursionCache()->offsetSet($object, $stack); return call_user_func($onceable->callable); @@ -70,4 +50,27 @@ protected function withoutRecursion($callback, $default = null) } } } + + /** + * Get the current stack of methods being called recursively. + * + * @param object $object + * @return array + */ + protected static function getRecursiveCallStack($object): array + { + return static::getRecursionCache()->offsetExists($object) + ? static::getRecursionCache()->offsetGet($object) + : []; + } + + /** + * Get the current recursion cache being used by the model. + * + * @return \WeakMap + */ + protected static function getRecursionCache() + { + return static::$recursionCache ??= new WeakMap(); + } } From 8982c36a37da9e46bda06875cde23ae6943fee68 Mon Sep 17 00:00:00 2001 From: Samuel Levy Date: Thu, 22 Aug 2024 14:42:58 +1000 Subject: [PATCH 5/6] [11.x] Delay calling a "default" callback until the last possible second --- .../Concerns/PreventsCircularRecursion.php | 57 ++++++++++++++----- 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php b/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php index bd26c1d6c0ac..27d69a98b1ae 100644 --- a/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php +++ b/src/Illuminate/Database/Eloquent/Concerns/PreventsCircularRecursion.php @@ -11,7 +11,7 @@ trait PreventsCircularRecursion /** * The cache of objects processed to prevent infinite recursion. * - * @var \WeakMap> + * @var WeakMap> */ protected static $recursionCache; @@ -28,31 +28,40 @@ protected function withoutRecursion($callback, $default = null) $onceable = Onceable::tryFromTrace($trace, $callback); - $object = $onceable->object ?? $this; - - $stack = static::getRecursiveCallStack($object); + $stack = static::getRecursiveCallStack($this); if (array_key_exists($onceable->hash, $stack)) { - return $stack[$onceable->hash]; + return is_callable($stack[$onceable->hash]) + ? static::setRecursiveCallValue($this, $onceable->hash, call_user_func($stack[$onceable->hash])) + : $stack[$onceable->hash]; } try { - $stack[$onceable->hash] = is_callable($default) ? call_user_func($default) : $default; - - static::getRecursionCache()->offsetSet($object, $stack); + static::setRecursiveCallValue($this, $onceable->hash, $default); return call_user_func($onceable->callable); } finally { - if ($stack = Arr::except($this->getRecursiveCallStack($object), $onceable->hash)) { - static::getRecursionCache()->offsetSet($object, $stack); - } elseif (static::getRecursionCache()->offsetExists($object)) { - static::getRecursionCache()->offsetUnset($object); - } + static::clearRecursiveCallValue($this, $onceable->hash); } } /** - * Get the current stack of methods being called recursively. + * Remove an entry from the recursion cache for an object. + * + * @param object $object + * @param string $hash + */ + protected static function clearRecursiveCallValue($object, string $hash) + { + if ($stack = Arr::except(static::getRecursiveCallStack($object), $hash)) { + static::getRecursionCache()->offsetSet($object, $stack); + } elseif (static::getRecursionCache()->offsetExists($object)) { + static::getRecursionCache()->offsetUnset($object); + } + } + + /** + * Get the stack of methods being called recursively for the current object. * * @param object $object * @return array @@ -67,10 +76,28 @@ protected static function getRecursiveCallStack($object): array /** * Get the current recursion cache being used by the model. * - * @return \WeakMap + * @return WeakMap */ protected static function getRecursionCache() { return static::$recursionCache ??= new WeakMap(); } + + /** + * Set a value in the recursion cache for the given object and method. + * + * @param object $object + * @param string $hash + * @param mixed $value + * @return mixed + */ + protected static function setRecursiveCallValue($object, string $hash, $value) + { + static::getRecursionCache()->offsetSet( + $object, + tap(static::getRecursiveCallStack($object), fn (&$stack) => $stack[$hash] = $value), + ); + + return static::getRecursiveCallStack($object)[$hash]; + } } From 8df234daf24e34a4a57fdcb11c52b17e65b1833f Mon Sep 17 00:00:00 2001 From: Samuel Levy Date: Thu, 22 Aug 2024 17:21:04 +1000 Subject: [PATCH 6/6] [11.x] Added additional tests for "callable" defaults --- ...eConcernsPreventsCircularRecursionTest.php | 93 ++++++++++++++++++- 1 file changed, 91 insertions(+), 2 deletions(-) diff --git a/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php b/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php index 057c4375422b..c04e85957718 100644 --- a/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php +++ b/tests/Database/DatabaseConcernsPreventsCircularRecursionTest.php @@ -30,6 +30,58 @@ public function testRecursiveCallsArePreventedWithoutPreventingSubsequentCalls() $this->assertEquals(2, $instance->instanceStack); } + public function testRecursiveDefaultCallbackIsCalledOnlyOnRecursion() + { + $instance = new PreventsCircularRecursionWithRecursiveMethod(); + + $this->assertEquals(0, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(0, $instance->instanceStack); + $this->assertEquals(0, $instance->defaultStack); + + $this->assertEquals(['instance' => 1, 'default' => 0], $instance->callCallableDefaultStack()); + $this->assertEquals(1, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(1, $instance->instanceStack); + $this->assertEquals(1, $instance->defaultStack); + + $this->assertEquals(['instance' => 2, 'default' => 1], $instance->callCallableDefaultStack()); + $this->assertEquals(2, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(2, $instance->instanceStack); + $this->assertEquals(2, $instance->defaultStack); + } + + public function testRecursiveDefaultCallbackIsCalledOnlyOncePerCallStack() + { + $instance = new PreventsCircularRecursionWithRecursiveMethod(); + + $this->assertEquals(0, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(0, $instance->instanceStack); + $this->assertEquals(0, $instance->defaultStack); + + $this->assertEquals( + [ + ['instance' => 1, 'default' => 0], + ['instance' => 1, 'default' => 0], + ['instance' => 1, 'default' => 0], + ], + $instance->callCallableDefaultStackRepeatedly(), + ); + $this->assertEquals(1, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(1, $instance->instanceStack); + $this->assertEquals(1, $instance->defaultStack); + + $this->assertEquals( + [ + ['instance' => 2, 'default' => 1], + ['instance' => 2, 'default' => 1], + ['instance' => 2, 'default' => 1], + ], + $instance->callCallableDefaultStackRepeatedly(), + ); + $this->assertEquals(2, PreventsCircularRecursionWithRecursiveMethod::$globalStack); + $this->assertEquals(2, $instance->instanceStack); + $this->assertEquals(2, $instance->defaultStack); + } + public function testRecursiveCallsAreLimitedToIndividualInstances() { $instance = new PreventsCircularRecursionWithRecursiveMethod(); @@ -134,6 +186,7 @@ public function __construct( public static int $globalStack = 0; public int $instanceStack = 0; + public int $defaultStack = 0; public function callStack(): int { @@ -144,7 +197,43 @@ function () { return $this->callStack(); }, - $this->instanceStack + $this->instanceStack, + ); + } + + public function callCallableDefaultStack(): array + { + return $this->withoutRecursion( + function () { + static::$globalStack++; + $this->instanceStack++; + + return $this->callCallableDefaultStack(); + }, + fn () => [ + 'instance' => $this->instanceStack, + 'default' => $this->defaultStack++, + ], + ); + } + + public function callCallableDefaultStackRepeatedly(): array + { + return $this->withoutRecursion( + function () { + static::$globalStack++; + $this->instanceStack++; + + return [ + $this->callCallableDefaultStackRepeatedly(), + $this->callCallableDefaultStackRepeatedly(), + $this->callCallableDefaultStackRepeatedly(), + ]; + }, + fn () => [ + 'instance' => $this->instanceStack, + 'default' => $this->defaultStack++, + ], ); } @@ -156,7 +245,7 @@ function () { return $this->other->callOtherStack(); }, - $this->instanceStack + $this->instanceStack, ); } }