Skip to content

Commit 14a570d

Browse files
nicolas-grekasnicolas-grekas
committed
[Workflow] State contamination due to class-based setter cache
1 parent 0f153b1 commit 14a570d

File tree

3 files changed

+114
-5
lines changed

3 files changed

+114
-5
lines changed

‎MarkingStore/MethodMarkingStore.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@
3131
*/
3232
final class MethodMarkingStore implements MarkingStoreInterface
3333
{
34-
/** @var array<class-string, MarkingStoreMethod> */
34+
/** @var array<class-string, \Closure(object): mixed> */
3535
private array $getters = [];
36-
/** @var array<class-string, MarkingStoreMethod> */
36+
/** @var array<class-string, \Closure(object, mixed, array)> */
3737
private array $setters = [];
3838

3939
/**
@@ -84,7 +84,7 @@ public function setMarking(object $subject, Marking $marking, array $context = [
8484
$marking = key($marking);
8585
}
8686

87-
($this->setters[$subject::class] ??= $this->getSetter($subject))($marking, $context);
87+
($this->setters[$subject::class] ??= $this->getSetter($subject))($subject, $marking, $context);
8888
}
8989

9090
private function getGetter(object $subject): callable
@@ -104,8 +104,8 @@ private function getSetter(object $subject): callable
104104
$method = 'set'.ucfirst($property);
105105

106106
return match (self::getType($subject, $property, $method, $type)) {
107-
MarkingStoreMethod::METHOD => $type ? static fn ($marking, $context) => $subject->{$method}($type::from($marking), $context) : $subject->{$method}(...),
108-
MarkingStoreMethod::PROPERTY => $type ? static fn ($marking) => $subject->{$property} = $type::from($marking) : static fn ($marking) => $subject->{$property} = $marking,
107+
MarkingStoreMethod::METHOD => $type ? static fn ($subject, $marking, $context) => $subject->{$method}($type::from($marking), $context) : static fn ($subject, $marking, $context) => $subject->{$method}($marking, $context),
108+
MarkingStoreMethod::PROPERTY => $type ? static fn ($subject, $marking) => $subject->{$property} = $type::from($marking) : static fn ($subject, $marking) => $subject->{$property} = $marking,
109109
};
110110
}
111111

‎Tests/MarkingStore/MethodMarkingStoreTest.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,65 @@ public function testGetMarkingWithSameSubjectMultipleTimes()
142142
$this->assertSame(['third_place' => 1], $marking3->getPlaces());
143143
}
144144

145+
public function testSetMarkingWithMultipleSubjectsSharingCachedSetter()
146+
{
147+
$subject1 = new Subject();
148+
$subject2 = new Subject();
149+
$subject3 = new Subject();
150+
151+
$markingStore = new MethodMarkingStore(true);
152+
153+
// First call caches the setter for Subject class
154+
$marking1 = $markingStore->getMarking($subject1);
155+
$marking1->mark('place1');
156+
$markingStore->setMarking($subject1, $marking1, ['context1' => 'value1']);
157+
158+
// Subsequent calls should use the cached setter but operate on different subjects
159+
$marking2 = $markingStore->getMarking($subject2);
160+
$marking2->mark('place2');
161+
$markingStore->setMarking($subject2, $marking2, ['context2' => 'value2']);
162+
163+
$marking3 = $markingStore->getMarking($subject3);
164+
$marking3->mark('place3');
165+
$markingStore->setMarking($subject3, $marking3, ['context3' => 'value3']);
166+
167+
// Each subject should have its own marking and context
168+
$this->assertSame('place1', $subject1->getMarking());
169+
$this->assertSame(['context1' => 'value1'], $subject1->getContext());
170+
171+
$this->assertSame('place2', $subject2->getMarking());
172+
$this->assertSame(['context2' => 'value2'], $subject2->getContext());
173+
174+
$this->assertSame('place3', $subject3->getMarking());
175+
$this->assertSame(['context3' => 'value3'], $subject3->getContext());
176+
}
177+
178+
public function testSetMarkingWithMultipleSubjectsSharingCachedSetterMultipleState()
179+
{
180+
$subject1 = new Subject();
181+
$subject2 = new Subject();
182+
183+
$markingStore = new MethodMarkingStore(false);
184+
185+
// First call caches the setter for Subject class
186+
$marking1 = $markingStore->getMarking($subject1);
187+
$marking1->mark('place1');
188+
$marking1->mark('place2');
189+
$markingStore->setMarking($subject1, $marking1, ['context1' => 'value1']);
190+
191+
// Second call should use the cached setter but operate on a different subject
192+
$marking2 = $markingStore->getMarking($subject2);
193+
$marking2->mark('place3');
194+
$markingStore->setMarking($subject2, $marking2, ['context2' => 'value2']);
195+
196+
// Each subject should have its own marking and context
197+
$this->assertSame(['place1' => 1, 'place2' => 1], $subject1->getMarking());
198+
$this->assertSame(['context1' => 'value1'], $subject1->getContext());
199+
200+
$this->assertSame(['place3' => 1], $subject2->getMarking());
201+
$this->assertSame(['context2' => 'value2'], $subject2->getContext());
202+
}
203+
145204
private function createValueObject(string $markingValue): object
146205
{
147206
return new class($markingValue) {

‎Tests/MarkingStore/PropertiesMarkingStoreTest.php‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,56 @@ public function testGetMarkingWithUninitializedProperty()
9292
$this->assertCount(0, $marking->getPlaces());
9393
}
9494

95+
public function testSetMarkingWithMultipleSubjectsSharingCachedSetter()
96+
{
97+
$subject1 = new SubjectWithProperties();
98+
$subject2 = new SubjectWithProperties();
99+
$subject3 = new SubjectWithProperties();
100+
101+
$markingStore = new MethodMarkingStore(false);
102+
103+
// First call caches the setter for SubjectWithProperties class
104+
$marking1 = $markingStore->getMarking($subject1);
105+
$marking1->mark('place1');
106+
$markingStore->setMarking($subject1, $marking1, ['context1' => 'value1']);
107+
108+
// Subsequent calls should use the cached setter but operate on different subjects
109+
$marking2 = $markingStore->getMarking($subject2);
110+
$marking2->mark('place2');
111+
$markingStore->setMarking($subject2, $marking2, ['context2' => 'value2']);
112+
113+
$marking3 = $markingStore->getMarking($subject3);
114+
$marking3->mark('place3');
115+
$markingStore->setMarking($subject3, $marking3, ['context3' => 'value3']);
116+
117+
// Each subject should have its own marking set correctly
118+
$this->assertSame(['place1' => 1], $subject1->marking);
119+
$this->assertSame(['place2' => 1], $subject2->marking);
120+
$this->assertSame(['place3' => 1], $subject3->marking);
121+
}
122+
123+
public function testSetMarkingWithMultipleSubjectsSharingCachedSetterSingleState()
124+
{
125+
$subject1 = new SubjectWithProperties();
126+
$subject2 = new SubjectWithProperties();
127+
128+
$markingStore = new MethodMarkingStore(true, 'place');
129+
130+
// First call caches the setter for SubjectWithProperties class
131+
$marking1 = $markingStore->getMarking($subject1);
132+
$marking1->mark('place1');
133+
$markingStore->setMarking($subject1, $marking1, ['context1' => 'value1']);
134+
135+
// Second call should use the cached setter but operate on a different subject
136+
$marking2 = $markingStore->getMarking($subject2);
137+
$marking2->mark('place2');
138+
$markingStore->setMarking($subject2, $marking2, ['context2' => 'value2']);
139+
140+
// Each subject should have its own marking set correctly
141+
$this->assertSame('place1', $subject1->place);
142+
$this->assertSame('place2', $subject2->place);
143+
}
144+
95145
private function createValueObject(string $markingValue): object
96146
{
97147
return new class($markingValue) {

0 commit comments

Comments
 (0)