Skip to content

Commit 9946e3e

Browse files
claudeclaude-bot
authored andcommitted
refactor: address review feedback on recent push
- Strip leaked internal `@architect:T*` task-ownership markers from the Redis adapter; they were planning artifacts and add no value to readers. - Document::fromRow: collapse two `array_key_exists('$permissions', ...)` branches into one — the second `is_array` check was redundant given the first arm throws on non-array. - SQL::generateOrderBy: rename `$callerSuppliedOrder` to its positive counterpart `$shouldAutoOrderByRelevance` so the gate predicate reads without double-negation. - Redis::orderDocuments: docblock said "preserve usort transitivity" but the random branch never calls usort — explain the actual reason (non-deterministic comparators are undefined for usort). - Structure::$mergedAttributes: weaken the "Stable for the lifetime of this validator" comment — `readonly` blocks property reassignment but not inner Document mutation, so callers that mutate the held collection between `isValid()` calls would see a stale memo. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 92a30ed commit 9946e3e

4 files changed

Lines changed: 14 additions & 48 deletions

File tree

src/Database/Adapter/Redis.php

Lines changed: 3 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1310,8 +1310,6 @@ private function loadMetadataDocument(string $collection): ?Document
13101310
return $this->decode($payload);
13111311
}
13121312

1313-
// === @architect:T20 owns: schema + collection + attribute ops ===
1314-
13151313
public function create(string $name): bool
13161314
{
13171315
$name = $this->filter($name);
@@ -1975,14 +1973,6 @@ private function measureKey(string $key): int
19751973
}
19761974
}
19771975

1978-
// === @architect:T20 end ===
1979-
1980-
1981-
1982-
1983-
1984-
// === @architect:T30 owns: document CRUD + bulk + increase ===
1985-
19861976
public function getDocument(Document $collection, string $id, array $queries = [], bool $forUpdate = false): Document
19871977
{
19881978
$col = $this->filter($collection->getId());
@@ -2629,14 +2619,6 @@ public function increaseDocumentAttribute(
26292619
});
26302620
}
26312621

2632-
// === @architect:T30 end ===
2633-
2634-
2635-
2636-
2637-
2638-
// === @architect:T40 owns: indexes + queries + counts ===
2639-
26402622
public function createIndex(string $collection, string $id, string $type, array $attributes, array $lengths, array $orders, array $indexAttributeTypes = [], array $collation = [], int $ttl = 1): bool
26412623
{
26422624
$collection = $this->filter($collection);
@@ -3356,8 +3338,9 @@ private function matchesDocumentObject(mixed $value, Query $query): bool
33563338
}
33573339

33583340
/**
3359-
* Stable ordering across Documents. Random short-circuits via shuffle to
3360-
* preserve usort transitivity; absent attributes fall back to $sequence.
3341+
* Stable ordering across Documents. Random short-circuits to shuffle()
3342+
* because a non-deterministic comparator passed to usort is undefined;
3343+
* absent attributes fall back to $sequence.
33613344
*
33623345
* @param array<int, Document> $documents
33633346
* @param array<int, string> $orderAttributes
@@ -3807,14 +3790,6 @@ private function projectDocument(Document $document, array $selections): Documen
38073790
return new Document($projected);
38083791
}
38093792

3810-
// === @architect:T40 end ===
3811-
3812-
3813-
3814-
3815-
3816-
// === @architect:T50 owns: permissions + relationships ===
3817-
38183793
public function createRelationship(string $collection, string $relatedCollection, string $type, bool $twoWay = false, string $id = '', string $twoWayKey = ''): bool
38193794
{
38203795
// Redis stores documents as flexible JSON blobs, so the relationship
@@ -3948,14 +3923,6 @@ public function deleteRelationship(string $collection, string $relatedCollection
39483923
return true;
39493924
}
39503925

3951-
// === @architect:T50 end ===
3952-
3953-
3954-
3955-
3956-
3957-
// === @architect:T56 owns: transactions + journal ===
3958-
39593926
public function startTransaction(): bool
39603927
{
39613928
$this->journalStack[] = [];
@@ -3988,8 +3955,6 @@ public function rollbackTransaction(): bool
39883955
return true;
39893956
}
39903957

3991-
// === @architect:T56 end ===
3992-
39933958
/**
39943959
* Resolve any Operator-typed attributes against the existing document
39953960
* before persisting. Mirrors Memory::applyOperators — non-operator

src/Database/Adapter/SQL.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1440,12 +1440,12 @@ public function find(Document $collection, array $queries = [], ?int $limit = 25
14401440
// relevance. Anything else (multiple entries, or a leading attribute
14411441
// other than $sequence) means the caller has an explicit order and
14421442
// relevance ordering would silently override it.
1443-
$callerSuppliedOrder = ! (
1443+
$shouldAutoOrderByRelevance = (
14441444
count($orderAttributes) === 0
14451445
|| (count($orderAttributes) === 1 && $orderAttributes[0] === '$sequence')
14461446
);
14471447

1448-
if (! empty($searchQueries) && ! $callerSuppliedOrder) {
1448+
if (! empty($searchQueries) && $shouldAutoOrderByRelevance) {
14491449
$builder->select(['*']);
14501450
foreach ($searchQueries as $searchQuery) {
14511451
$relevanceRaw = $this->getSearchRelevanceRaw($searchQuery, $alias);

src/Database/Document.php

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -99,11 +99,10 @@ public static function fromRow(array $row): self
9999
throw new StructureException('$id must be of type string');
100100
}
101101

102-
if (array_key_exists('$permissions', $row) && ! is_array($row['$permissions'])) {
103-
throw new StructureException('$permissions must be of type array');
104-
}
105-
106-
if (array_key_exists('$permissions', $row) && is_array($row['$permissions'])) {
102+
if (array_key_exists('$permissions', $row)) {
103+
if (! \is_array($row['$permissions'])) {
104+
throw new StructureException('$permissions must be of type array');
105+
}
107106
$row['$permissions'] = \array_values(\array_unique($row['$permissions']));
108107
}
109108

src/Database/Validator/Structure.php

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,9 +114,11 @@ class Structure extends Validator
114114
/**
115115
* Lazily-built merged attribute list (internal + collection).
116116
*
117-
* Stable for the lifetime of this validator: $attributes is constant and
118-
* $collection is readonly, so the merge result never changes after the
119-
* first call.
117+
* Cached for the lifetime of this validator. `$collection` is `readonly`
118+
* (the property cannot be reassigned), but its inner Document state is
119+
* not deep-frozen — callers that mutate `$collection->setAttribute(
120+
* 'attributes', ...)` between `isValid()` calls would see a stale memo.
121+
* Construct a fresh validator if the underlying schema may change.
120122
*
121123
* @var array<array<string, mixed>>|null
122124
*/

0 commit comments

Comments
 (0)