Skip to content

Commit 9daf76a

Browse files
abnegateabnegate
committed
fix(memory): address PR review findings
- rollbackTransaction now decrements inTransaction and pops a single snapshot, matching the SQL adapter and the nested-transaction contract - updateAttribute applies new metadata after a rename instead of returning early and silently dropping type/size/required changes - renameAttribute and deleteAttribute keep index attribute lists in sync so indexed queries and uniqueness checks remain consistent - createIndex(INDEX_UNIQUE) refuses to register when existing rows already contain duplicate values for the indexed columns - enforceUniqueIndexes skips signatures with NULLs so multiple null values do not collide on a unique index - updateDocuments runs enforceUniqueIndexes per row so batch updates cannot introduce duplicate unique values - deleteDocuments always purges permissions for documents removed by sequence, not only when explicit permissionIds are provided - rowToDocument decodes JSON-encoded array attributes so array fields round-trip from getDocument/find - introduce documentKey() and key documents by tenant|id under sharedTables so tenants no longer clobber each other on the same id Adds regression coverage for each fix.
1 parent 7e02eb5 commit 9daf76a

2 files changed

Lines changed: 382 additions & 33 deletions

File tree

‎src/Database/Adapter/Memory.php‎

Lines changed: 117 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,11 @@ protected function key(string $collection): string
6161
return $this->getNamespace() . '_' . $this->filter($collection);
6262
}
6363

64+
protected function documentKey(string $id): string
65+
{
66+
return $this->sharedTables ? $this->getTenant() . '|' . $id : $id;
67+
}
68+
6469
public function setTimeout(int $milliseconds, string $event = Database::EVENT_ALL): void
6570
{
6671
// No-op: nothing to time out in-memory
@@ -108,8 +113,7 @@ public function rollbackTransaction(): bool
108113
$this->data = $snapshot['data'];
109114
$this->permissions = $snapshot['permissions'];
110115
}
111-
$this->inTransaction = 0;
112-
$this->snapshots = [];
116+
$this->inTransaction--;
113117
return true;
114118
}
115119

@@ -245,7 +249,8 @@ public function updateAttribute(string $collection, string $id, string $type, in
245249

246250
$id = $this->filter($id);
247251
if (!empty($newKey) && $newKey !== $id) {
248-
return $this->renameAttribute($collection, $id, $newKey);
252+
$this->renameAttribute($collection, $id, $newKey);
253+
$id = $this->filter($newKey);
249254
}
250255

251256
$this->data[$key]['attributes'][$id] = [
@@ -271,6 +276,30 @@ public function deleteAttribute(string $collection, string $id): bool
271276
unset($document[$id]);
272277
}
273278
unset($document);
279+
280+
foreach ($this->data[$key]['indexes'] as &$index) {
281+
$attributes = $index['attributes'] ?? [];
282+
$filtered = [];
283+
$lengths = [];
284+
$orders = [];
285+
foreach ($attributes as $i => $attribute) {
286+
if ($this->filter($attribute) === $id) {
287+
continue;
288+
}
289+
$filtered[] = $attribute;
290+
if (isset($index['lengths'][$i])) {
291+
$lengths[] = $index['lengths'][$i];
292+
}
293+
if (isset($index['orders'][$i])) {
294+
$orders[] = $index['orders'][$i];
295+
}
296+
}
297+
$index['attributes'] = $filtered;
298+
$index['lengths'] = $lengths;
299+
$index['orders'] = $orders;
300+
}
301+
unset($index);
302+
274303
return true;
275304
}
276305

@@ -298,6 +327,18 @@ public function renameAttribute(string $collection, string $old, string $new): b
298327
}
299328
}
300329
unset($document);
330+
331+
foreach ($this->data[$key]['indexes'] as &$index) {
332+
$attributes = $index['attributes'] ?? [];
333+
foreach ($attributes as $i => $attribute) {
334+
if ($this->filter($attribute) === $old) {
335+
$attributes[$i] = $new;
336+
}
337+
}
338+
$index['attributes'] = $attributes;
339+
}
340+
unset($index);
341+
301342
return true;
302343
}
303344

@@ -346,6 +387,24 @@ public function createIndex(string $collection, string $id, string $type, array
346387
throw new DatabaseException('Fulltext indexes are not implemented in the Memory adapter');
347388
}
348389

390+
if ($type === Database::INDEX_UNIQUE && !empty($attributes)) {
391+
$seen = [];
392+
foreach ($this->data[$key]['documents'] as $row) {
393+
$signature = [];
394+
foreach ($attributes as $attribute) {
395+
$signature[] = $row[$this->mapAttribute($attribute)] ?? null;
396+
}
397+
if (\in_array(null, $signature, true)) {
398+
continue;
399+
}
400+
$hash = \json_encode($signature);
401+
if (isset($seen[$hash])) {
402+
throw new DatabaseException('Cannot create unique index: existing rows already contain duplicate values');
403+
}
404+
$seen[$hash] = true;
405+
}
406+
}
407+
349408
$id = $this->filter($id);
350409
$this->data[$key]['indexes'][$id] = [
351410
'type' => $type,
@@ -375,15 +434,11 @@ public function getDocument(Document $collection, string $id, array $queries = [
375434
return new Document([]);
376435
}
377436

378-
$doc = $this->data[$key]['documents'][$id] ?? null;
437+
$doc = $this->data[$key]['documents'][$this->documentKey($id)] ?? null;
379438
if ($doc === null) {
380439
return new Document([]);
381440
}
382441

383-
if ($this->sharedTables && ($doc['_tenant'] ?? null) !== $this->getTenant()) {
384-
return new Document([]);
385-
}
386-
387442
return new Document($this->rowToDocument($doc));
388443
}
389444

@@ -394,11 +449,9 @@ public function createDocument(Document $collection, Document $document): Docume
394449
throw new NotFoundException('Collection not found');
395450
}
396451

397-
if (isset($this->data[$key]['documents'][$document->getId()])) {
398-
$existing = $this->data[$key]['documents'][$document->getId()];
399-
if (!$this->sharedTables || ($existing['_tenant'] ?? null) === $this->getTenant()) {
400-
throw new DuplicateException('Document already exists');
401-
}
452+
$docKey = $this->documentKey($document->getId());
453+
if (isset($this->data[$key]['documents'][$docKey])) {
454+
throw new DuplicateException('Document already exists');
402455
}
403456

404457
$this->enforceUniqueIndexes($key, $document, null);
@@ -417,7 +470,7 @@ public function createDocument(Document $collection, Document $document): Docume
417470
$row = $this->documentToRow($document);
418471
$row['_id'] = $sequence;
419472

420-
$this->data[$key]['documents'][$document->getId()] = $row;
473+
$this->data[$key]['documents'][$docKey] = $row;
421474
$this->writePermissions($key, $document);
422475

423476
$document['$sequence'] = (string) $sequence;
@@ -440,13 +493,15 @@ public function updateDocument(Document $collection, string $id, Document $docum
440493
throw new NotFoundException('Collection not found');
441494
}
442495

443-
$existing = $this->data[$key]['documents'][$id] ?? null;
496+
$oldKey = $this->documentKey($id);
497+
$existing = $this->data[$key]['documents'][$oldKey] ?? null;
444498
if ($existing === null) {
445499
throw new NotFoundException('Document not found');
446500
}
447501

448502
$newId = $document->getId();
449-
if ($newId !== $id && isset($this->data[$key]['documents'][$newId])) {
503+
$newKey = $this->documentKey($newId);
504+
if ($newId !== $id && isset($this->data[$key]['documents'][$newKey])) {
450505
throw new DuplicateException('Document already exists');
451506
}
452507

@@ -456,9 +511,9 @@ public function updateDocument(Document $collection, string $id, Document $docum
456511
$row['_id'] = $existing['_id'];
457512

458513
if ($newId !== $id) {
459-
unset($this->data[$key]['documents'][$id]);
514+
unset($this->data[$key]['documents'][$oldKey]);
460515
}
461-
$this->data[$key]['documents'][$newId] = $row;
516+
$this->data[$key]['documents'][$newKey] = $row;
462517

463518
if (!$skipPermissions) {
464519
// Remove any permissions keyed to the old uid and rewrite.
@@ -501,11 +556,21 @@ public function updateDocuments(Document $collection, Document $updates, array $
501556
$count = 0;
502557
foreach ($documents as $doc) {
503558
$uid = $doc->getId();
504-
if (!isset($this->data[$key]['documents'][$uid])) {
559+
$docKey = $this->documentKey($uid);
560+
if (!isset($this->data[$key]['documents'][$docKey])) {
505561
continue;
506562
}
507563

508-
$row = &$this->data[$key]['documents'][$uid];
564+
if (!empty($attrs)) {
565+
$merged = new Document(\array_merge(
566+
$this->rowToDocument($this->data[$key]['documents'][$docKey]),
567+
$attrs,
568+
['$id' => $uid]
569+
));
570+
$this->enforceUniqueIndexes($key, $merged, $uid);
571+
}
572+
573+
$row = &$this->data[$key]['documents'][$docKey];
509574
foreach ($attrs as $attribute => $value) {
510575
if (\is_array($value)) {
511576
$value = \json_encode($value);
@@ -556,7 +621,7 @@ public function getSequences(string $collection, array $documents): array
556621
}
557622

558623
foreach ($documents as $index => $doc) {
559-
$existing = $this->data[$key]['documents'][$doc->getId()] ?? null;
624+
$existing = $this->data[$key]['documents'][$this->documentKey($doc->getId())] ?? null;
560625
if ($existing !== null) {
561626
$documents[$index]->setAttribute('$sequence', (string) $existing['_id']);
562627
}
@@ -571,11 +636,12 @@ public function deleteDocument(string $collection, string $id): bool
571636
return false;
572637
}
573638

574-
if (!isset($this->data[$key]['documents'][$id])) {
639+
$docKey = $this->documentKey($id);
640+
if (!isset($this->data[$key]['documents'][$docKey])) {
575641
return false;
576642
}
577643

578-
unset($this->data[$key]['documents'][$id]);
644+
unset($this->data[$key]['documents'][$docKey]);
579645
$this->permissions[$key] = \array_values(\array_filter(
580646
$this->permissions[$key] ?? [],
581647
fn (array $p) => $p['document'] !== $id
@@ -596,18 +662,23 @@ public function deleteDocuments(string $collection, array $sequences, array $per
596662
}
597663

598664
$count = 0;
665+
$deletedIds = [];
599666
foreach ($this->data[$key]['documents'] as $uid => $row) {
600667
if (isset($seqSet[(string) ($row['_id'] ?? '')])) {
668+
$deletedIds[(string) ($row['_uid'] ?? $uid)] = true;
601669
unset($this->data[$key]['documents'][$uid]);
602670
$count++;
603671
}
604672
}
605673

606-
if (!empty($permissionIds)) {
607-
$permSet = \array_flip(\array_map('strval', $permissionIds));
674+
$permSet = !empty($permissionIds)
675+
? \array_flip(\array_map('strval', $permissionIds))
676+
: [];
677+
678+
if (!empty($deletedIds) || !empty($permSet)) {
608679
$this->permissions[$key] = \array_values(\array_filter(
609680
$this->permissions[$key] ?? [],
610-
fn (array $p) => !isset($permSet[$p['document']])
681+
fn (array $p) => !isset($deletedIds[$p['document']]) && !isset($permSet[$p['document']])
611682
));
612683
}
613684

@@ -701,12 +772,13 @@ public function sum(Document $collection, string $attribute, array $queries = []
701772
public function increaseDocumentAttribute(string $collection, string $id, string $attribute, int|float $value, string $updatedAt, int|float|null $min = null, int|float|null $max = null): bool
702773
{
703774
$key = $this->key($collection);
704-
if (!isset($this->data[$key]['documents'][$id])) {
775+
$docKey = $this->documentKey($id);
776+
if (!isset($this->data[$key]['documents'][$docKey])) {
705777
throw new NotFoundException('Document not found');
706778
}
707779

708780
$column = $this->filter($attribute);
709-
$current = $this->data[$key]['documents'][$id][$column] ?? 0;
781+
$current = $this->data[$key]['documents'][$docKey][$column] ?? 0;
710782
$current = is_numeric($current) ? $current + 0 : 0;
711783
$next = $current + $value;
712784

@@ -717,8 +789,8 @@ public function increaseDocumentAttribute(string $collection, string $id, string
717789
return false;
718790
}
719791

720-
$this->data[$key]['documents'][$id][$column] = $next;
721-
$this->data[$key]['documents'][$id]['_updatedAt'] = $updatedAt;
792+
$this->data[$key]['documents'][$docKey][$column] = $next;
793+
$this->data[$key]['documents'][$docKey]['_updatedAt'] = $updatedAt;
722794
return true;
723795
}
724796

@@ -1195,6 +1267,13 @@ protected function rowToDocument(array $row): array
11951267
$document['$permissions'] = \is_string($value) ? (\json_decode($value, true) ?? []) : ($value ?? []);
11961268
break;
11971269
default:
1270+
if (\is_string($value) && $value !== '' && ($value[0] === '[' || $value[0] === '{')) {
1271+
$decoded = \json_decode($value, true);
1272+
if (\is_array($decoded)) {
1273+
$document[$key] = $decoded;
1274+
break;
1275+
}
1276+
}
11981277
$document[$key] = $value;
11991278
}
12001279
}
@@ -1557,11 +1636,16 @@ protected function enforceUniqueIndexes(string $key, Document $document, ?string
15571636
$signature[] = \is_array($docValue) ? \json_encode($docValue) : $docValue;
15581637
}
15591638

1560-
foreach ($this->data[$key]['documents'] as $uid => $row) {
1561-
if ($previousId !== null && $uid === $previousId) {
1639+
if (\in_array(null, $signature, true)) {
1640+
continue;
1641+
}
1642+
1643+
foreach ($this->data[$key]['documents'] as $row) {
1644+
$rowUid = (string) ($row['_uid'] ?? '');
1645+
if ($previousId !== null && $rowUid === $previousId) {
15621646
continue;
15631647
}
1564-
if ($uid === $document->getId() && $previousId === null) {
1648+
if ($rowUid === $document->getId() && $previousId === null) {
15651649
continue;
15661650
}
15671651
if ($this->sharedTables && ($row['_tenant'] ?? null) !== $this->getTenant()) {

0 commit comments

Comments
 (0)