Skip to content

Commit 4f03607

Browse files
github-actions[bot]claude-bot
github-actions[bot]
authored and
claude-bot
committed
fix: address review findings — auth parity, MariaDB timeout, relationship chunking, validator cache
- MariaDB: apply session timeout eagerly in setTimeout/clearTimeout so direct $stmt->execute() paths inherit it, mirroring the MySQL fix; prevents stale timeouts leaking across pool checkouts. - Relationships hook: chunk batched updateDocuments/deleteDocuments calls by RELATION_QUERY_CHUNK_SIZE so a one-to-many or many-to-many diff with thousands of removed peers stays within the validator's maxQueryValues ceiling. - Database::setMaxQueryValues: clear the DocumentsValidator cache when the limit changes; the cache key encodes the old value. - Traits/Documents (find): always restore previous Authorization status in the inlined skip toggle so a nested setStatus inside the try block cannot leak past the scope. - Memory adapter: decodeObjectValue returns null on JSON parse miss (parity with decodeArrayValue); prevents scalar haystacks reaching jsonContains. - MemoryTest::freshDatabase: use $this->testDatabase to honor the per-process isolation token. - SQL adapter: clarify spatialCacheKey() comment to match code (tenant intentionally excluded; schema is shared across tenants).
1 parent 9946e3e commit 4f03607

7 files changed

Lines changed: 91 additions & 48 deletions

File tree

‎src/Database/Adapter/MariaDB.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -816,9 +816,25 @@ public function setTimeout(int $milliseconds, Event $event = Event::All): void
816816
throw new DatabaseException('Timeout must be greater than 0');
817817
}
818818

819+
// Apply eagerly so direct $stmt->execute() paths (e.g. exists()) inherit
820+
// the new timeout even before the next $this->execute() runs. Lazy
821+
// application leaked stale timeouts across pool checkouts under paratest;
822+
// mirrors the MySQL adapter's eager-apply fix for the same shape of bug.
823+
$seconds = $milliseconds / 1000.0;
824+
$this->getPDO()->exec('SET max_statement_time = ' . $seconds);
825+
$this->appliedMaxStatementTime = $seconds;
826+
819827
parent::setTimeout($milliseconds, $event);
820828
}
821829

830+
public function clearTimeout(Event $event = Event::All): void
831+
{
832+
$this->getPDO()->exec('SET max_statement_time = 0');
833+
$this->appliedMaxStatementTime = 0.0;
834+
835+
parent::clearTimeout($event);
836+
}
837+
822838
/**
823839
* Size of POINT spatial type
824840
*/

‎src/Database/Adapter/Memory.php‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3150,22 +3150,18 @@ protected function matchesObject(mixed $value, Query $query): bool
31503150
throw new DatabaseException('Query method '.$method->value.' not supported for object attributes');
31513151
}
31523152

3153-
protected function decodeObjectValue(mixed $value): mixed
3153+
protected function decodeObjectValue(mixed $value): ?array
31543154
{
3155-
if ($value === null) {
3156-
return null;
3157-
}
31583155
if (\is_array($value)) {
31593156
return $value;
31603157
}
31613158
if (\is_string($value) && $value !== '' && ($value[0] === '{' || $value[0] === '[')) {
31623159
$decoded = \json_decode($value, true);
3163-
if (\is_array($decoded)) {
3164-
return $decoded;
3165-
}
3160+
3161+
return \is_array($decoded) ? $decoded : null;
31663162
}
31673163

3168-
return $value;
3164+
return null;
31693165
}
31703166

31713167
/**

‎src/Database/Adapter/SQL.php‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3620,9 +3620,11 @@ protected function invalidateSpatialAttributesCache(string $collectionId): void
36203620
}
36213621

36223622
/**
3623-
* Compose a cache key scoped to the current database/namespace/tenant so
3624-
* that Pool sibling adapters reused across tenants never collide on a
3625-
* shared collection id.
3623+
* Compose a cache key scoped to the current database and namespace so
3624+
* that Pool sibling adapters reused across schemas never collide on a
3625+
* shared collection id. Tenant is intentionally excluded: collection
3626+
* schema (and therefore the spatial-attribute set) is shared across
3627+
* tenants under shared tables.
36263628
*/
36273629
private function spatialCacheKey(string $collectionId): string
36283630
{

‎src/Database/Database.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1098,6 +1098,13 @@ public function isMigrating(): bool
10981098
*/
10991099
public function setMaxQueryValues(int $max): self
11001100
{
1101+
if ($this->maxQueryValues !== $max) {
1102+
// Validator cache key encodes maxQueryValues; entries built under
1103+
// the previous limit must be discarded so subsequent validation
1104+
// honors the new ceiling.
1105+
$this->documentsValidatorCache = [];
1106+
}
1107+
11011108
$this->maxQueryValues = $max;
11021109

11031110
return $this;

‎src/Database/Hook/Relationships.php‎

Lines changed: 53 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -482,11 +482,16 @@ public function afterDocumentUpdate(Document $collection, Document $old, Documen
482482
$removedDocuments = \array_values(\array_diff($oldIds, $newIds));
483483

484484
if (! empty($removedDocuments)) {
485-
$this->db->getAuthorization()->skip(fn () => $this->db->skipRelationships(fn () => $this->db->updateDocuments(
486-
$relatedCollection->getId(),
487-
new Document([$twoWayKey => null]),
488-
[Query::equal('$id', $removedDocuments)],
489-
)));
485+
// Chunk to honor the validator's maxQueryValues cap; without
486+
// this a relationship update with thousands of removed
487+
// children would throw QueryException.
488+
foreach (\array_chunk($removedDocuments, Database::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
489+
$this->db->getAuthorization()->skip(fn () => $this->db->skipRelationships(fn () => $this->db->updateDocuments(
490+
$relatedCollection->getId(),
491+
new Document([$twoWayKey => null]),
492+
[Query::equal('$id', $chunk)],
493+
)));
494+
}
490495
}
491496

492497
$stringRelations = [];
@@ -502,21 +507,28 @@ public function afterDocumentUpdate(Document $collection, Document $old, Documen
502507
}
503508

504509
if (! empty($stringRelations)) {
505-
$existing = $this->db->skipRelationships(
506-
fn () => $this->db->find($relatedCollection->getId(), [
507-
Query::select(['$id']),
508-
Query::equal('$id', $stringRelations),
509-
Query::limit(\count($stringRelations)),
510-
])
511-
);
510+
$existingIds = [];
511+
foreach (\array_chunk($stringRelations, Database::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
512+
$existing = $this->db->skipRelationships(
513+
fn () => $this->db->find($relatedCollection->getId(), [
514+
Query::select(['$id']),
515+
Query::equal('$id', $chunk),
516+
Query::limit(\count($chunk)),
517+
])
518+
);
519+
foreach ($existing as $doc) {
520+
$existingIds[] = $doc->getId();
521+
}
522+
}
512523

513-
if (! empty($existing)) {
514-
$existingIds = \array_map(fn (Document $doc) => $doc->getId(), $existing);
515-
$this->db->skipRelationships(fn () => $this->db->updateDocuments(
516-
$relatedCollection->getId(),
517-
new Document([$twoWayKey => $document->getId()]),
518-
[Query::equal('$id', $existingIds)],
519-
));
524+
if (! empty($existingIds)) {
525+
foreach (\array_chunk($existingIds, Database::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
526+
$this->db->skipRelationships(fn () => $this->db->updateDocuments(
527+
$relatedCollection->getId(),
528+
new Document([$twoWayKey => $document->getId()]),
529+
[Query::equal('$id', $chunk)],
530+
));
531+
}
520532
}
521533
}
522534

@@ -620,19 +632,29 @@ public function afterDocumentUpdate(Document $collection, Document $old, Documen
620632
if (! empty($removedDocuments)) {
621633
$junction = $this->getJunctionCollection($collection, $relatedCollection, $side);
622634

623-
$junctions = $this->db->find($junction, [
624-
Query::select(['$id']),
625-
Query::equal($key, $removedDocuments),
626-
Query::equal($twoWayKey, [$document->getId()]),
627-
Query::limit(PHP_INT_MAX),
628-
]);
635+
// Chunk both the lookup and the delete so a many-to-many
636+
// diff with thousands of removed peers stays within the
637+
// validator's maxQueryValues ceiling.
638+
$junctionIds = [];
639+
foreach (\array_chunk($removedDocuments, Database::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
640+
$junctions = $this->db->find($junction, [
641+
Query::select(['$id']),
642+
Query::equal($key, $chunk),
643+
Query::equal($twoWayKey, [$document->getId()]),
644+
Query::limit(PHP_INT_MAX),
645+
]);
646+
foreach ($junctions as $junctionDoc) {
647+
$junctionIds[] = $junctionDoc->getId();
648+
}
649+
}
629650

630-
if (! empty($junctions)) {
631-
$junctionIds = \array_map(fn (Document $junctionDoc) => $junctionDoc->getId(), $junctions);
632-
$this->db->getAuthorization()->skip(fn () => $this->db->deleteDocuments(
633-
$junction,
634-
[Query::equal('$id', $junctionIds)],
635-
));
651+
if (! empty($junctionIds)) {
652+
foreach (\array_chunk($junctionIds, Database::RELATION_QUERY_CHUNK_SIZE) as $chunk) {
653+
$this->db->getAuthorization()->skip(fn () => $this->db->deleteDocuments(
654+
$junction,
655+
[Query::equal('$id', $chunk)],
656+
));
657+
}
636658
}
637659
}
638660

‎src/Database/Traits/Documents.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2337,8 +2337,10 @@ public function find(string $collection, array $queries = [], PermissionType $fo
23372337

23382338
if (! isset($results)) {
23392339
// Inline the auth-skip toggle to avoid the per-find Closure
2340-
// allocation that authorization->skip() requires. Still
2341-
// restores the original status on exception via try/finally.
2340+
// allocation that authorization->skip() requires. Mirrors
2341+
// Authorization::skip's restore semantics: the previous status
2342+
// is reapplied unconditionally so any nested toggle inside the
2343+
// try block cannot leak past this scope.
23422344
if ($skipAuth) {
23432345
$previousStatus = $this->authorization->getStatus();
23442346
$this->authorization->disable();
@@ -2355,9 +2357,7 @@ public function find(string $collection, array $queries = [], PermissionType $fo
23552357
$forPermission
23562358
);
23572359
} finally {
2358-
if ($previousStatus) {
2359-
$this->authorization->enable();
2360-
}
2360+
$this->authorization->setStatus($previousStatus);
23612361
}
23622362
} else {
23632363
$results = $this->adapter->find(

‎tests/e2e/Adapter/MemoryTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ private function freshDatabase(): Database
9595
$authorization = self::$authorization ?? throw new \RuntimeException('Authorization not initialised');
9696
$database
9797
->setAuthorization($authorization)
98-
->setDatabase('utopiaTests')
98+
->setDatabase($this->testDatabase)
9999
->setNamespace('memory_iso_' . uniqid());
100100
$database->create();
101101
return $database;

0 commit comments

Comments
 (0)