diff --git a/api/app/Services/Sync/AbstractCursorReader.php b/api/app/Services/Sync/AbstractCursorReader.php new file mode 100644 index 0000000..d0da63f --- /dev/null +++ b/api/app/Services/Sync/AbstractCursorReader.php @@ -0,0 +1,120 @@ + $since filter + * - orderBy updated_at_ms ASC + * - pageSize+1 fetch with hasMore detection + * - max updated_at_ms cursor computation + * - [rows, hasMore, maxUpdatedMs] tuple packaging + */ +abstract class AbstractCursorReader implements RecordReader +{ + /** + * Return the Eloquent model class-string for this entity. + * + * @return class-string + */ + abstract protected function modelClass(): string; + + /** + * Apply user-ownership scoping to the base query. + * + * For direct-ownership entities (user_id column on the model), constrain + * by user_id. For parent-owned entities, use whereIn against the user's + * deck IDs: $query->whereIn('deck_id', $user->decks()->select('id')). + * + * @param Builder $query The base query for the entity's model. + * @param User $user Authenticated user whose records are being fetched. + * @return Builder The scoped query; must be returned (fluent-style or new instance). + */ + abstract protected function scopeForUser(Builder $query, User $user): Builder; + + /** + * Project a single model into the wire-format row array. + * + * @param Model $model Hydrated model instance. + * @return array Wire-format row to include in the response. + */ + abstract protected function projectRow(Model $model): array; + + /** + * Fetch records for the user changed after $since, up to $pageSize rows. + * + * Pagination contract: queries $pageSize + 1 rows. If the result set exceeds + * $pageSize, the extra row signals that more data exists (hasMore=true) and is + * discarded before projection. The returned maxUpdatedMs is the highest + * updated_at_ms in the page — the client sends this back as `since` on the + * next pull to advance the cursor. + * + * @param User $user Owner of the records; results are scoped via scopeForUser(). + * @param int $since Millisecond timestamp lower-bound (exclusive). + * @param int $pageSize Maximum number of rows to include in the returned page. + * @return array{0: list>, 1: bool, 2: int} + * [rows, hasMore, maxUpdatedMs] + */ + public function read(User $user, int $since, int $pageSize): array + { + $modelClass = $this->modelClass(); + + $rows = $this->scopeForUser($modelClass::query(), $user) + ->where('updated_at_ms', '>', $since) + ->orderBy('updated_at_ms') + ->limit($pageSize + 1) + ->get(); + + $hasMore = $rows->count() > $pageSize; + $page = $rows->take($pageSize); + $max = (int) ($page->max('updated_at_ms') ?? $since); + + return [ + $page->map(fn (Model $m) => $this->projectRow($m))->values()->all(), + $hasMore, + $max, + ]; + } +} diff --git a/api/app/Services/Sync/AbstractLwwUpserter.php b/api/app/Services/Sync/AbstractLwwUpserter.php new file mode 100644 index 0000000..8a4208c --- /dev/null +++ b/api/app/Services/Sync/AbstractLwwUpserter.php @@ -0,0 +1,191 @@ +id set explicitly before save() + * so HasUuids does not regenerate the client-supplied UUID. + * + * Not suitable for: + * - CardSubTopicUpserter — two-part ownership check (card's deck + sub_topic + * must be on the same deck) cannot be expressed via checkOwnership(). + * - ReviewUpserter — insert-only semantics; there is no LWW update path. + */ + +namespace App\Services\Sync; + +use App\Models\User; +use Illuminate\Database\Eloquent\Model; +use Illuminate\Support\Facades\DB; + +/** + * Template-method base class for LWW upserts with transactional row-locking + * and partial-update-preserving field application. + * + * Subclasses declare: + * - modelClass(): the Eloquent model (class-string) + * - checkOwnership(): return a failure reason or null to proceed + * - applyFields(): copy row fields onto the model using preserve() helpers + * + * The base class handles: + * - missing_id guard + * - ownership-reason short-circuit + * - DB transaction + lockForUpdate on the target row (TOCTOU fix) + * - stale LWW check (>=, so equal timestamps are also rejected for idempotency) + * - UUID preservation (new $modelClass; $model->id = $id) + * - deleted_at_ms with partial-update semantics + * - updated_at_ms assignment + */ +abstract class AbstractLwwUpserter implements RecordUpserter +{ + /** + * Return the Eloquent model class-string for this entity. + * + * @return class-string + */ + abstract protected function modelClass(): string; + + /** + * Check whether the authenticated user may write the record identified by $id. + * + * Called after the missing_id guard, before the transactional LWW block. + * Return a non-null reason string to reject immediately; return null to proceed. + * + * Note: this check does a pre-transaction SELECT. It is intentionally kept + * outside the transaction to avoid holding a lock on the ownership-parent row + * (e.g. the parent Deck) during the upsert of the child entity. For ownership + * checks, the risk of a TOCTOU race here is low (deck transfers do not occur + * in normal usage), and the inner lockForUpdate covers the entity row itself. + * + * @param User $user Authenticated user performing the push. + * @param string $id UUID of the record being upserted. + * @param array $row Full row payload from the client. + * @return string|null Rejection reason, or null to proceed. + */ + abstract protected function checkOwnership(User $user, string $id, array $row): ?string; + + /** + * Copy row fields onto the model. + * + * Called inside the DB transaction after the LWW check passes. The base + * class sets updated_at_ms and deleted_at_ms after this call returns, so + * subclasses must not set those fields here. + * + * Use $this->preserve($row, $existing, 'field_name') for nullable optional + * fields so that absent keys do not overwrite existing data. + * + * @param Model $model The model instance (new or fetched existing). + * @param User $user Authenticated user; needed for user_id on top-level entities. + * @param array $row Client row payload. + * @param Model|null $existing The pre-lock existing record, or null on create. + */ + abstract protected function applyFields(Model $model, User $user, array $row, ?Model $existing): void; + + /** + * Attempt to upsert the record using last-write-wins conflict resolution. + * + * Guard order: + * 1. Blank id → missing_id + * 2. Ownership check (subclass) → forbidden or custom reason + * 3. DB transaction: lockForUpdate on entity row, stale check, write + * + * @param User $user Authenticated owner. + * @param array $row Raw record payload from the client push request. + * @return UpsertResult Accepted (true) or rejected (false) with a reason string. + */ + public function upsert(User $user, array $row): UpsertResult + { + $id = (string) ($row['id'] ?? ''); + if ($id === '') { + return new UpsertResult(false, 'missing_id'); + } + + if ($reason = $this->checkOwnership($user, $id, $row)) { + return new UpsertResult(false, $reason); + } + + $incoming = (int) ($row['updated_at_ms'] ?? 0); + $modelClass = $this->modelClass(); + + return DB::transaction(function () use ($modelClass, $user, $id, $row, $incoming): UpsertResult { + /** @var Model|null $existing */ + $existing = $modelClass::query()->where('id', $id)->lockForUpdate()->first(); + + // LWW: reject stale or equal timestamps. Equality is rejected so that + // retried pushes with the same clock value are idempotent (no double-write). + if ($existing && (int) $existing->getAttribute('updated_at_ms') >= $incoming) { + return new UpsertResult(false, 'stale'); + } + + // UUID preservation: explicitly set id before save() so HasUuids + // skips its own UUID generation (id is not in $fillable). + /** @var Model $model */ + $model = $existing ?? new $modelClass; + if (! $existing) { + // setAttribute avoids PHPStan's property.notFound on the base Model class. + $model->setAttribute('id', $id); + } + + $this->applyFields($model, $user, $row, $existing); + + // Base class owns these two fields so subclasses cannot forget them. + $model->setAttribute('updated_at_ms', $incoming); + $model->setAttribute('deleted_at_ms', $this->preserve($row, $existing, 'deleted_at_ms', castInt: true)); + + $model->save(); + + return new UpsertResult(true); + }); + } + + /** + * Partial-update-preserving field resolver. + * + * Returns the EXISTING model attribute when $key is absent from $row, + * preventing absent optional fields from silently overwriting stored data. + * When $key is present (even explicitly null), the incoming value wins. + * + * @param array $row Client row payload. + * @param Model|null $existing Pre-lock model, or null on create. + * @param string $key The field name to resolve. + * @param bool $castInt When true, coerce non-null incoming values to int. + * @return mixed Resolved value to assign to the model attribute. + */ + protected function preserve(array $row, ?Model $existing, string $key, bool $castInt = false): mixed + { + if (! array_key_exists($key, $row)) { + // Key absent from payload — preserve whatever is already stored. + return $existing?->getAttribute($key); + } + + $v = $row[$key]; + if ($v === null) { + return null; + } + + return $castInt ? (int) $v : $v; + } +} diff --git a/api/app/Services/Sync/Entities/CardReader.php b/api/app/Services/Sync/Entities/CardReader.php index a217920..a5202f5 100644 --- a/api/app/Services/Sync/Entities/CardReader.php +++ b/api/app/Services/Sync/Entities/CardReader.php @@ -13,17 +13,16 @@ * Dependencies: * - App\Models\Card (Eloquent model, HasUuids) * - App\Models\User (ownership scoping via decks() relation) - * - App\Services\Sync\RecordReader (interface contract) + * - App\Services\Sync\AbstractCursorReader (base class; handles pagination, + * cursor computation, and the [rows, hasMore, maxUpdatedMs] tuple) * * Key concepts: * - Ownership: Card has no direct user_id column. Ownership is enforced * by constraining deck_id to the set of deck IDs owned by the user, resolved * via a subquery on $user->decks()->select('id'). This is why the User model * must expose a decks() HasMany relation. - * - Pagination: queries pageSize + 1 rows; if the result exceeds pageSize, - * hasMore=true is returned and the extra row is discarded before mapping. - * - nextSince: the maximum updated_at_ms in the returned page is passed back - * to the client as the cursor for the next pull request. + * - Pagination: one-extra-row trick; see AbstractCursorReader. + * - nextSince: max updated_at_ms in page → client's next `since`. * - Ordered by updated_at_ms ASC so cursored pagination is stable. */ @@ -31,68 +30,69 @@ use App\Models\Card; use App\Models\User; -use App\Services\Sync\RecordReader; +use App\Services\Sync\AbstractCursorReader; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\Model; /** * Reads Card records accessible to the authenticated user updated after a given timestamp. * - * Implements the RecordReader contract for the "cards" entity key. - * Ownership is enforced via a subquery on the user's owned deck IDs. + * Implements the RecordReader contract for the "cards" entity key via + * AbstractCursorReader. Ownership is enforced via a whereIn subquery on the + * user's owned deck IDs. */ -final class CardReader implements RecordReader +final class CardReader extends AbstractCursorReader { /** - * Fetch Card rows for the user changed after $since, up to $pageSize rows. - * - * Pagination contract: queries $pageSize + 1 rows. If the result set exceeds - * $pageSize, the extra row signals that more data exists (hasMore=true) and is - * discarded before serialisation. The returned maxUpdatedMs is the highest - * updated_at_ms in the page — the client sends this back as `since` on the - * next pull to advance the cursor. + * @return class-string + */ + protected function modelClass(): string + { + return Card::class; + } + + /** + * Scope the query to cards belonging to decks owned by the user. * - * Ownership is enforced by restricting deck_id to the subquery - * $user->decks()->select('id'), which returns only decks owned by the user. + * Uses a whereIn subquery on $user->decks()->select('id') so that ownership + * is enforced without a JOIN — the User model must expose a decks() HasMany. * - * @param User $user Owner of the records; results are scoped to this user's decks. - * @param int $since Millisecond timestamp lower-bound (exclusive); only rows updated after this are returned. - * @param int $pageSize Maximum number of rows to include in the returned page. - * @return array{0: list>, 1: bool, 2: int} - * [rows, hasMore, maxUpdatedMs] + * @param Builder $query Base query for the Card model. + * @param User $user Authenticated user. + * @return Builder Scoped query. */ - public function read(User $user, int $since, int $pageSize): array + protected function scopeForUser(Builder $query, User $user): Builder { - $rows = Card::query() - ->whereIn('deck_id', $user->decks()->select('id')) - ->where('updated_at_ms', '>', $since) - ->orderBy('updated_at_ms') - ->limit($pageSize + 1) - ->get(); + return $query->whereIn('deck_id', $user->decks()->select('id')); + } - $hasMore = $rows->count() > $pageSize; - $page = $rows->take($pageSize); - $max = (int) ($page->max('updated_at_ms') ?? $since); + /** + * Project a Card model into the wire-format row. + * + * @param Model $model Hydrated Card instance. + * @return array Wire-format row. + */ + protected function projectRow(Model $model): array + { + assert($model instanceof Card); return [ - $page->map(fn (Card $card) => [ - 'id' => $card->id, - 'deck_id' => $card->deck_id, - 'front_text' => $card->front_text, - 'back_text' => $card->back_text, - 'front_image_asset_id' => $card->front_image_asset_id, - 'back_image_asset_id' => $card->back_image_asset_id, - 'position' => $card->position, - 'stability' => $card->stability, - 'difficulty' => $card->difficulty, - 'state' => $card->state, - 'last_reviewed_at_ms' => $card->last_reviewed_at_ms, - 'due_at_ms' => $card->due_at_ms, - 'lapses' => $card->lapses, - 'reps' => $card->reps, - 'updated_at_ms' => $card->updated_at_ms, - 'deleted_at_ms' => $card->deleted_at_ms, - ])->values()->all(), - $hasMore, - $max, + 'id' => $model->id, + 'deck_id' => $model->deck_id, + 'front_text' => $model->front_text, + 'back_text' => $model->back_text, + 'front_image_asset_id' => $model->front_image_asset_id, + 'back_image_asset_id' => $model->back_image_asset_id, + 'position' => $model->position, + 'stability' => $model->stability, + 'difficulty' => $model->difficulty, + 'state' => $model->state, + 'last_reviewed_at_ms' => $model->last_reviewed_at_ms, + 'due_at_ms' => $model->due_at_ms, + 'lapses' => $model->lapses, + 'reps' => $model->reps, + 'updated_at_ms' => $model->updated_at_ms, + 'deleted_at_ms' => $model->deleted_at_ms, ]; } } diff --git a/api/app/Services/Sync/Entities/CardSubTopicReader.php b/api/app/Services/Sync/Entities/CardSubTopicReader.php index ff27e2a..2d05ccd 100644 --- a/api/app/Services/Sync/Entities/CardSubTopicReader.php +++ b/api/app/Services/Sync/Entities/CardSubTopicReader.php @@ -26,6 +26,15 @@ * - nextSince: the maximum updated_at_ms in the returned page is passed back * to the client as the cursor for the next pull request. * - Ordered by updated_at_ms ASC so cursored pagination is stable. + * + * Does NOT extend AbstractCursorReader: + * AbstractCursorReader's scopeForUser() hook is designed for direct ownership + * (user_id on entity) or single-level parent ownership (whereIn deck_id). + * CardSubTopicReader needs a two-level nested whereHas traversal to reach the + * user via card → deck. Expressing this through scopeForUser() would work + * mechanically, but the contract's documentation and typical usage imply a + * flat or single-level scope. A standalone implementation is clearer and avoids + * misleading future developers about the expected scope shape. */ namespace App\Services\Sync\Entities; diff --git a/api/app/Services/Sync/Entities/CardSubTopicUpserter.php b/api/app/Services/Sync/Entities/CardSubTopicUpserter.php index d83b407..bcdced8 100644 --- a/api/app/Services/Sync/Entities/CardSubTopicUpserter.php +++ b/api/app/Services/Sync/Entities/CardSubTopicUpserter.php @@ -26,6 +26,15 @@ * (1) card must exist and its deck must be owned by the authenticated user. * (2) sub-topic must exist and its deck_id must match the card's deck_id. * - Stateless: no constructor dependencies; safe to resolve per-request via app(). + * + * Does NOT extend AbstractLwwUpserter: + * The checkOwnership() hook in AbstractLwwUpserter supports a single ownership + * predicate. CardSubTopicUpserter requires a two-part check — (1) card's deck + * must be owned by the user AND (2) the sub-topic must belong to the same deck + * as the card. The second condition is a cross-entity structural invariant, not + * a simple ownership guard, and cannot be expressed through the single-hook contract. + * Forcing it into the base class would require awkward workarounds that obscure + * the intent; a standalone implementation is simpler and clearer. */ namespace App\Services\Sync\Entities; diff --git a/api/app/Services/Sync/Entities/CardUpserter.php b/api/app/Services/Sync/Entities/CardUpserter.php index 9b03600..02f7f6e 100644 --- a/api/app/Services/Sync/Entities/CardUpserter.php +++ b/api/app/Services/Sync/Entities/CardUpserter.php @@ -15,14 +15,17 @@ * - App\Models\Card (Eloquent model, HasUuids) * - App\Models\Deck (parent entity; ownership resolved via deck->user_id) * - App\Models\User (ownership scoping) - * - App\Services\Sync\RecordUpserter (interface contract) - * - App\Services\Sync\UpsertResult (return value object) + * - App\Services\Sync\AbstractLwwUpserter (base class; handles transaction, + * LWW check, UUID preservation, partial-update semantics) * * Key concepts: * - LWW rule: incoming row is rejected when existing updated_at_ms >= incoming updated_at_ms. * - Forbidden: the referenced deck must exist and be owned by the authenticated user. * Unlike top-level entities (Topic, Deck), there is no user_id column on cards; * ownership is enforced via the deck's user_id. + * - Partial updates: front_image_asset_id, back_image_asset_id, stability, + * difficulty, last_reviewed_at_ms, and due_at_ms use preserve() so absent keys + * do not overwrite existing values with null. * - Stateless: no constructor dependencies; safe to resolve per-request via app(). */ @@ -31,78 +34,82 @@ use App\Models\Card; use App\Models\Deck; use App\Models\User; -use App\Services\Sync\RecordUpserter; -use App\Services\Sync\UpsertResult; +use App\Services\Sync\AbstractLwwUpserter; +use Illuminate\Database\Eloquent\Model; /** * Upserts a single Card record for the authenticated user. * - * Implements the RecordUpserter contract for the "cards" entity key. - * Ownership is enforced via the parent deck's user_id — the cards table - * has no direct user_id column. + * Implements the RecordUpserter contract for the "cards" entity key via + * AbstractLwwUpserter. Ownership is enforced via the parent deck's user_id — + * the cards table has no direct user_id column. */ -final class CardUpserter implements RecordUpserter +final class CardUpserter extends AbstractLwwUpserter { /** - * Attempt to upsert a Card row using last-write-wins conflict resolution. - * - * LWW rule: if an existing row's updated_at_ms is >= the incoming value, - * the update is considered stale and rejected with reason "stale". This - * ensures a slower client cannot overwrite newer server state. + * @return class-string + */ + protected function modelClass(): string + { + return Card::class; + } + + /** + * Reject if deck_id is missing or the referenced deck is not owned by the user. * - * Ownership check: the referenced deck_id must belong to the authenticated - * user. If the deck does not exist or belongs to another user, the row is - * rejected with reason "forbidden". + * Also returns 'missing_id' when deck_id is absent, since a Card without + * a parent deck is structurally invalid. * - * @param User $user Authenticated owner; used to verify deck ownership. - * @param array $row Raw record payload from the client push request. - * @return UpsertResult Accepted (true) or rejected (false) with a reason string. + * @param User $user Authenticated user. + * @param string $id Card UUID from the client row. + * @param array $row Full client row payload (must contain deck_id). + * @return string|null 'missing_id', 'forbidden', or null to proceed. */ - public function upsert(User $user, array $row): UpsertResult + protected function checkOwnership(User $user, string $id, array $row): ?string { - $id = (string) ($row['id'] ?? ''); $deckId = (string) ($row['deck_id'] ?? ''); - $incoming = (int) ($row['updated_at_ms'] ?? 0); - - if ($id === '' || $deckId === '') { - return new UpsertResult(false, 'missing_id'); + if ($deckId === '') { + return 'missing_id'; } $deck = Deck::find($deckId); - if (! $deck || $deck->user_id !== $user->id) { - return new UpsertResult(false, 'forbidden'); - } - $existing = Card::find($id); - if ($existing && $existing->updated_at_ms >= $incoming) { - return new UpsertResult(false, 'stale'); - } + return (! $deck || $deck->user_id !== $user->id) ? 'forbidden' : null; + } + + /** + * Map client row fields onto the Card model. + * + * front_image_asset_id, back_image_asset_id, stability, difficulty, + * last_reviewed_at_ms, and due_at_ms use preserve() so that absent keys + * do not overwrite existing values with null (partial-update semantics). + * + * TODO(1.15): validate asset ownership and add FK constraint after assets table exists. + * + * @param Model $model Card model instance (new or fetched). + * @param User $user Authenticated owner (unused here; ownership is via deck). + * @param array $row Client row payload. + * @param Model|null $existing Pre-lock existing record, or null on create. + */ + protected function applyFields(Model $model, User $user, array $row, ?Model $existing): void + { + assert($model instanceof Card); - // Use firstOrNew + explicit id assignment to avoid HasUuids overwriting the - // client-supplied UUID during mass-assignment (id is not in $fillable, so - // updateOrCreate's where-clause id would not reach setUniqueIds' empty check). - $card = Card::firstOrNew(['id' => $id]); - $card->id = $id; - $card->fill([ - 'deck_id' => $deckId, - 'front_text' => (string) ($row['front_text'] ?? ''), - 'back_text' => (string) ($row['back_text'] ?? ''), - // TODO(1.15): validate asset ownership and add FK constraint after assets table exists. - 'front_image_asset_id' => $row['front_image_asset_id'] ?? null, - 'back_image_asset_id' => $row['back_image_asset_id'] ?? null, - 'position' => (int) ($row['position'] ?? 0), - 'stability' => isset($row['stability']) ? (float) $row['stability'] : null, - 'difficulty' => isset($row['difficulty']) ? (float) $row['difficulty'] : null, - 'state' => (string) ($row['state'] ?? 'new'), - 'last_reviewed_at_ms' => isset($row['last_reviewed_at_ms']) ? (int) $row['last_reviewed_at_ms'] : null, - 'due_at_ms' => isset($row['due_at_ms']) ? (int) $row['due_at_ms'] : null, - 'lapses' => (int) ($row['lapses'] ?? 0), - 'reps' => (int) ($row['reps'] ?? 0), - 'updated_at_ms' => $incoming, - 'deleted_at_ms' => isset($row['deleted_at_ms']) ? (int) $row['deleted_at_ms'] : null, - ]); - $card->save(); + $model->deck_id = (string) ($row['deck_id'] ?? ''); + $model->front_text = (string) ($row['front_text'] ?? ''); + $model->back_text = (string) ($row['back_text'] ?? ''); + $model->front_image_asset_id = $this->preserve($row, $existing, 'front_image_asset_id'); + $model->back_image_asset_id = $this->preserve($row, $existing, 'back_image_asset_id'); + $model->position = (int) ($row['position'] ?? 0); + $stability = $this->preserve($row, $existing, 'stability'); + $model->stability = $stability !== null ? (float) $stability : null; - return new UpsertResult(true); + $difficulty = $this->preserve($row, $existing, 'difficulty'); + $model->difficulty = $difficulty !== null ? (float) $difficulty : null; + $model->state = (string) ($row['state'] ?? 'new'); + $model->last_reviewed_at_ms = $this->preserve($row, $existing, 'last_reviewed_at_ms', castInt: true); + $model->due_at_ms = $this->preserve($row, $existing, 'due_at_ms', castInt: true); + $model->lapses = (int) ($row['lapses'] ?? 0); + $model->reps = (int) ($row['reps'] ?? 0); } } diff --git a/api/app/Services/Sync/Entities/DeckReader.php b/api/app/Services/Sync/Entities/DeckReader.php index 92665a8..0e81f9f 100644 --- a/api/app/Services/Sync/Entities/DeckReader.php +++ b/api/app/Services/Sync/Entities/DeckReader.php @@ -13,13 +13,13 @@ * Dependencies: * - App\Models\Deck (Eloquent model, HasUuids) * - App\Models\User (ownership scoping) - * - App\Services\Sync\RecordReader (interface contract) + * - App\Services\Sync\AbstractCursorReader (base class; handles pagination, + * cursor computation, and the [rows, hasMore, maxUpdatedMs] tuple) * * Key concepts: - * - Pagination: queries pageSize + 1 rows; if the result exceeds pageSize, - * hasMore=true is returned and the extra row is discarded before mapping. - * - nextSince: the maximum updated_at_ms in the returned page is passed back - * to the client as the cursor for the next pull request. + * - Ownership: scoped by user_id directly on the decks table. + * - Pagination: one-extra-row trick; see AbstractCursorReader. + * - nextSince: max updated_at_ms in page → client's next `since`. * - Ordered by updated_at_ms ASC so cursored pagination is stable. */ @@ -27,58 +27,60 @@ use App\Models\Deck; use App\Models\User; -use App\Services\Sync\RecordReader; +use App\Services\Sync\AbstractCursorReader; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\Model; /** * Reads Deck records for the authenticated user updated after a given timestamp. * - * Implements the RecordReader contract for the "decks" entity key. + * Implements the RecordReader contract for the "decks" entity key via + * AbstractCursorReader. Only entity-specific concerns are declared here: + * model class, ownership scope, and row projection. */ -final class DeckReader implements RecordReader +final class DeckReader extends AbstractCursorReader { /** - * Fetch Deck rows for the user changed after $since, up to $pageSize rows. - * - * Pagination contract: queries $pageSize + 1 rows. If the result set exceeds - * $pageSize, the extra row signals that more data exists (hasMore=true) and is - * discarded before serialisation. The returned maxUpdatedMs is the highest - * updated_at_ms in the page — the client sends this back as `since` on the - * next pull to advance the cursor. + * @return class-string + */ + protected function modelClass(): string + { + return Deck::class; + } + + /** + * Scope the query to decks owned by the authenticated user. * - * @param User $user Owner of the records; results are scoped to this user. - * @param int $since Millisecond timestamp lower-bound (exclusive); only rows updated after this are returned. - * @param int $pageSize Maximum number of rows to include in the returned page. - * @return array{0: list>, 1: bool, 2: int} - * [rows, hasMore, maxUpdatedMs] + * @param Builder $query Base query for the Deck model. + * @param User $user Authenticated user. + * @return Builder Scoped query. */ - public function read(User $user, int $since, int $pageSize): array + protected function scopeForUser(Builder $query, User $user): Builder { - $rows = Deck::query() - ->where('user_id', $user->id) - ->where('updated_at_ms', '>', $since) - ->orderBy('updated_at_ms') - ->limit($pageSize + 1) - ->get(); + return $query->where('user_id', $user->id); + } - $hasMore = $rows->count() > $pageSize; - $page = $rows->take($pageSize); - $max = (int) ($page->max('updated_at_ms') ?? $since); + /** + * Project a Deck model into the wire-format row. + * + * @param Model $model Hydrated Deck instance. + * @return array Wire-format row. + */ + protected function projectRow(Model $model): array + { + assert($model instanceof Deck); return [ - $page->map(fn (Deck $d) => [ - 'id' => $d->id, - 'topic_id' => $d->topic_id, - 'title' => $d->title, - 'description' => $d->description, - 'accent_color' => $d->accent_color, - 'default_study_mode' => $d->default_study_mode, - 'card_count' => $d->card_count, - 'last_studied_at_ms' => $d->last_studied_at_ms, - 'updated_at_ms' => $d->updated_at_ms, - 'deleted_at_ms' => $d->deleted_at_ms, - ])->values()->all(), - $hasMore, - $max, + 'id' => $model->id, + 'topic_id' => $model->topic_id, + 'title' => $model->title, + 'description' => $model->description, + 'accent_color' => $model->accent_color, + 'default_study_mode' => $model->default_study_mode, + 'card_count' => $model->card_count, + 'last_studied_at_ms' => $model->last_studied_at_ms, + 'updated_at_ms' => $model->updated_at_ms, + 'deleted_at_ms' => $model->deleted_at_ms, ]; } } diff --git a/api/app/Services/Sync/Entities/DeckUpserter.php b/api/app/Services/Sync/Entities/DeckUpserter.php index a074340..4964370 100644 --- a/api/app/Services/Sync/Entities/DeckUpserter.php +++ b/api/app/Services/Sync/Entities/DeckUpserter.php @@ -13,12 +13,14 @@ * Dependencies: * - App\Models\Deck (Eloquent model, HasUuids) * - App\Models\User (ownership scoping) - * - App\Services\Sync\RecordUpserter (interface contract) - * - App\Services\Sync\UpsertResult (return value object) + * - App\Services\Sync\AbstractLwwUpserter (base class; handles transaction, + * LWW check, UUID preservation, partial-update semantics) * * Key concepts: * - LWW rule: incoming row is rejected when existing updated_at_ms >= incoming updated_at_ms. * - Forbidden: a row whose UUID exists but belongs to a different user is rejected. + * - Partial updates: topic_id, description, and last_studied_at_ms use preserve() + * so absent keys do not overwrite existing values with null. * - Stateless: no constructor dependencies; safe to resolve per-request via app(). */ @@ -26,65 +28,64 @@ use App\Models\Deck; use App\Models\User; -use App\Services\Sync\RecordUpserter; -use App\Services\Sync\UpsertResult; +use App\Services\Sync\AbstractLwwUpserter; +use Illuminate\Database\Eloquent\Model; /** * Upserts a single Deck record for the authenticated user. * - * Implements the RecordUpserter contract for the "decks" entity key. + * Implements the RecordUpserter contract for the "decks" entity key via + * AbstractLwwUpserter. Only entity-specific concerns are declared here: + * model class, ownership check, and field mapping. */ -final class DeckUpserter implements RecordUpserter +final class DeckUpserter extends AbstractLwwUpserter { /** - * Attempt to upsert a Deck row using last-write-wins conflict resolution. - * - * LWW rule: if an existing row's updated_at_ms is >= the incoming value, - * the update is considered stale and rejected with reason "stale". This - * ensures a slower client cannot overwrite newer server state. - * - * @param User $user Authenticated owner; used to scope creation and enforce access. - * @param array $row Raw record payload from the client push request. - * @return UpsertResult Accepted (true) or rejected (false) with a reason string. + * @return class-string */ - public function upsert(User $user, array $row): UpsertResult + protected function modelClass(): string { - $id = (string) ($row['id'] ?? ''); - $incoming = (int) ($row['updated_at_ms'] ?? 0); - - if ($id === '') { - return new UpsertResult(false, 'missing_id'); - } + return Deck::class; + } + /** + * Reject if a deck with this id exists and belongs to a different user. + * + * @param User $user Authenticated user. + * @param string $id Deck UUID from the client row. + * @param array $row Full client row payload. + * @return string|null 'forbidden' if ownership fails; null to proceed. + */ + protected function checkOwnership(User $user, string $id, array $row): ?string + { $existing = Deck::find($id); - if ($existing && $existing->user_id !== $user->id) { - return new UpsertResult(false, 'forbidden'); - } - - if ($existing && $existing->updated_at_ms >= $incoming) { - return new UpsertResult(false, 'stale'); - } + return $existing && $existing->user_id !== $user->id ? 'forbidden' : null; + } - // Use firstOrNew + explicit id assignment to avoid HasUuids overwriting the - // client-supplied UUID during mass-assignment (id is not in $fillable, so - // updateOrCreate's where-clause id would not reach setUniqueIds' empty check). - $deck = Deck::firstOrNew(['id' => $id]); - $deck->id = $id; - $deck->fill([ - 'user_id' => $user->id, - 'topic_id' => $row['topic_id'] ?? null, - 'title' => (string) ($row['title'] ?? ''), - 'description' => $row['description'] ?? null, - 'accent_color' => (string) ($row['accent_color'] ?? 'amber'), - 'default_study_mode' => (string) ($row['default_study_mode'] ?? 'smart'), - 'card_count' => (int) ($row['card_count'] ?? 0), - 'last_studied_at_ms' => isset($row['last_studied_at_ms']) ? (int) $row['last_studied_at_ms'] : null, - 'updated_at_ms' => $incoming, - 'deleted_at_ms' => isset($row['deleted_at_ms']) ? (int) $row['deleted_at_ms'] : null, - ]); - $deck->save(); + /** + * Map client row fields onto the Deck model. + * + * topic_id, description, and last_studied_at_ms use preserve() so that + * a push omitting those keys does not overwrite existing values with null + * (partial-update semantics). + * + * @param Model $model Deck model instance (new or fetched). + * @param User $user Authenticated owner. + * @param array $row Client row payload. + * @param Model|null $existing Pre-lock existing record, or null on create. + */ + protected function applyFields(Model $model, User $user, array $row, ?Model $existing): void + { + assert($model instanceof Deck); - return new UpsertResult(true); + $model->user_id = $user->id; + $model->topic_id = $this->preserve($row, $existing, 'topic_id'); + $model->title = (string) ($row['title'] ?? ''); + $model->description = $this->preserve($row, $existing, 'description'); + $model->accent_color = (string) ($row['accent_color'] ?? 'amber'); + $model->default_study_mode = (string) ($row['default_study_mode'] ?? 'smart'); + $model->card_count = (int) ($row['card_count'] ?? 0); + $model->last_studied_at_ms = $this->preserve($row, $existing, 'last_studied_at_ms', castInt: true); } } diff --git a/api/app/Services/Sync/Entities/ReviewReader.php b/api/app/Services/Sync/Entities/ReviewReader.php index 2053fff..2cf0449 100644 --- a/api/app/Services/Sync/Entities/ReviewReader.php +++ b/api/app/Services/Sync/Entities/ReviewReader.php @@ -24,6 +24,13 @@ * - nextSince: the maximum updated_at_ms in the returned page is passed back * to the client as the cursor for the next pull request. * - Ordered by updated_at_ms ASC so cursored pagination is stable. + * + * Does NOT extend AbstractCursorReader: + * Review is append-only and carries significantly more fields than any other + * entity (state_before, state_after as JSON, scheduler_version, etc.). While + * structurally compatible with the base class, keeping it standalone preserves + * the semantic distinction between mutable LWW entities and immutable review + * events, and avoids conflating the two reader categories for future maintainers. */ namespace App\Services\Sync\Entities; diff --git a/api/app/Services/Sync/Entities/ReviewUpserter.php b/api/app/Services/Sync/Entities/ReviewUpserter.php index 0dbfa8a..c4d2c18 100644 --- a/api/app/Services/Sync/Entities/ReviewUpserter.php +++ b/api/app/Services/Sync/Entities/ReviewUpserter.php @@ -28,6 +28,13 @@ * - UUID preservation: id is not in $fillable, so we set $review->id explicitly * before save() to prevent HasUuids from regenerating the client-supplied UUID. * - Stateless: no constructor dependencies; safe to resolve per-request via app(). + * + * Does NOT extend AbstractLwwUpserter: + * AbstractLwwUpserter implements LWW (last-write-wins) update semantics with + * a stale-check and an update path. Reviews are append-only — there is no update + * path, no stale check, and duplicate detection uses "exists?" rather than + * timestamp comparison. Inheriting the base class would require overriding the + * entire upsert() method, defeating the purpose of the base class entirely. */ namespace App\Services\Sync\Entities; diff --git a/api/app/Services/Sync/Entities/SessionReader.php b/api/app/Services/Sync/Entities/SessionReader.php index 4adb7cb..60bb7a1 100644 --- a/api/app/Services/Sync/Entities/SessionReader.php +++ b/api/app/Services/Sync/Entities/SessionReader.php @@ -13,13 +13,13 @@ * Dependencies: * - App\Models\Session (Eloquent model, HasUuids, table=study_sessions) * - App\Models\User (ownership scoping) - * - App\Services\Sync\RecordReader (interface contract) + * - App\Services\Sync\AbstractCursorReader (base class; handles pagination, + * cursor computation, and the [rows, hasMore, maxUpdatedMs] tuple) * * Key concepts: - * - Pagination: queries pageSize + 1 rows; if the result exceeds pageSize, - * hasMore=true is returned and the extra row is discarded before mapping. - * - nextSince: the maximum updated_at_ms in the returned page is passed back - * to the client as the cursor for the next pull request. + * - Ownership: scoped by user_id directly on the study_sessions table. + * - Pagination: one-extra-row trick; see AbstractCursorReader. + * - nextSince: max updated_at_ms in page → client's next `since`. * - Ordered by updated_at_ms ASC so cursored pagination is stable. */ @@ -27,59 +27,61 @@ use App\Models\Session; use App\Models\User; -use App\Services\Sync\RecordReader; +use App\Services\Sync\AbstractCursorReader; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\Model; /** * Reads Session records for the authenticated user updated after a given timestamp. * - * Implements the RecordReader contract for the "sessions" entity key. + * Implements the RecordReader contract for the "sessions" entity key via + * AbstractCursorReader. Only entity-specific concerns are declared here: + * model class, ownership scope, and row projection. */ -final class SessionReader implements RecordReader +final class SessionReader extends AbstractCursorReader { /** - * Fetch Session rows for the user changed after $since, up to $pageSize rows. - * - * Pagination contract: queries $pageSize + 1 rows. If the result set exceeds - * $pageSize, the extra row signals that more data exists (hasMore=true) and is - * discarded before serialisation. The returned maxUpdatedMs is the highest - * updated_at_ms in the page — the client sends this back as `since` on the - * next pull to advance the cursor. + * @return class-string + */ + protected function modelClass(): string + { + return Session::class; + } + + /** + * Scope the query to sessions owned by the authenticated user. * - * @param User $user Owner of the records; results are scoped to this user. - * @param int $since Millisecond timestamp lower-bound (exclusive); only rows updated after this are returned. - * @param int $pageSize Maximum number of rows to include in the returned page. - * @return array{0: list>, 1: bool, 2: int} - * [rows, hasMore, maxUpdatedMs] + * @param Builder $query Base query for the Session model. + * @param User $user Authenticated user. + * @return Builder Scoped query. */ - public function read(User $user, int $since, int $pageSize): array + protected function scopeForUser(Builder $query, User $user): Builder { - $rows = Session::query() - ->where('user_id', $user->id) - ->where('updated_at_ms', '>', $since) - ->orderBy('updated_at_ms') - ->limit($pageSize + 1) - ->get(); + return $query->where('user_id', $user->id); + } - $hasMore = $rows->count() > $pageSize; - $page = $rows->take($pageSize); - $max = (int) ($page->max('updated_at_ms') ?? $since); + /** + * Project a Session model into the wire-format row. + * + * @param Model $model Hydrated Session instance. + * @return array Wire-format row. + */ + protected function projectRow(Model $model): array + { + assert($model instanceof Session); return [ - $page->map(fn (Session $s) => [ - 'id' => $s->id, - 'user_id' => $s->user_id, - 'deck_id' => $s->deck_id, - 'mode' => $s->mode, - 'started_at_ms' => $s->started_at_ms, - 'ended_at_ms' => $s->ended_at_ms, - 'cards_reviewed' => $s->cards_reviewed, - 'accuracy_pct' => $s->accuracy_pct, - 'mastery_delta' => $s->mastery_delta, - 'updated_at_ms' => $s->updated_at_ms, - 'deleted_at_ms' => $s->deleted_at_ms, - ])->values()->all(), - $hasMore, - $max, + 'id' => $model->id, + 'user_id' => $model->user_id, + 'deck_id' => $model->deck_id, + 'mode' => $model->mode, + 'started_at_ms' => $model->started_at_ms, + 'ended_at_ms' => $model->ended_at_ms, + 'cards_reviewed' => $model->cards_reviewed, + 'accuracy_pct' => $model->accuracy_pct, + 'mastery_delta' => $model->mastery_delta, + 'updated_at_ms' => $model->updated_at_ms, + 'deleted_at_ms' => $model->deleted_at_ms, ]; } } diff --git a/api/app/Services/Sync/Entities/SessionUpserter.php b/api/app/Services/Sync/Entities/SessionUpserter.php index a24841f..07f8d44 100644 --- a/api/app/Services/Sync/Entities/SessionUpserter.php +++ b/api/app/Services/Sync/Entities/SessionUpserter.php @@ -15,15 +15,17 @@ * - App\Models\Deck (ownership check via deck->user_id) * - App\Models\Session (Eloquent model, HasUuids, table=study_sessions) * - App\Models\User (authenticated owner) - * - App\Services\Sync\RecordUpserter (interface contract) - * - App\Services\Sync\UpsertResult (return value object) + * - App\Services\Sync\AbstractLwwUpserter (base class; handles transaction, + * LWW check, UUID preservation, partial-update semantics) * * Key concepts: * - LWW rule: incoming row is rejected when existing updated_at_ms >= incoming updated_at_ms. * - Ownership: the referenced deck must exist and its user_id must match the * authenticated user. Missing or foreign decks yield a "forbidden" result. - * - UUID preservation: id is not in $fillable; we use firstOrNew + explicit - * $model->id assignment so HasUuids does not regenerate the client-supplied UUID. + * - Partial updates: ended_at_ms uses preserve() so an absent key does not + * overwrite an existing value with null. + * - UUID preservation: id is not in $fillable; the base class sets $model->id + * explicitly before save() so HasUuids does not regenerate it. * - Stateless: no constructor dependencies; safe to resolve per-request via app(). */ @@ -32,75 +34,71 @@ use App\Models\Deck; use App\Models\Session; use App\Models\User; -use App\Services\Sync\RecordUpserter; -use App\Services\Sync\UpsertResult; +use App\Services\Sync\AbstractLwwUpserter; +use Illuminate\Database\Eloquent\Model; /** * Upserts a single Session record for the authenticated user. * - * Implements the RecordUpserter contract for the "sessions" entity key. - * Ownership is enforced by verifying the referenced deck belongs to the user. + * Implements the RecordUpserter contract for the "sessions" entity key via + * AbstractLwwUpserter. Ownership is enforced by verifying the referenced + * deck belongs to the authenticated user. */ -final class SessionUpserter implements RecordUpserter +final class SessionUpserter extends AbstractLwwUpserter { /** - * Attempt to upsert a Session row using last-write-wins conflict resolution. - * - * Guard order: - * 1. Missing id or deck_id → missing_id - * 2. Deck not found or deck.user_id != user.id → forbidden - * 3. Stale LWW check (existing updated_at_ms >= incoming) → stale - * 4. firstOrNew + explicit id + fill + save + * @return class-string + */ + protected function modelClass(): string + { + return Session::class; + } + + /** + * Reject if deck_id is missing or the referenced deck is not owned by the user. * - * UUID preservation: id is not in $fillable. We assign $session->id - * explicitly before save() to prevent HasUuids from regenerating it. + * Also returns 'missing_id' when deck_id is absent, since a Session without + * a parent deck is structurally invalid. * - * @param User $user Authenticated owner; used to verify deck ownership. - * @param array $row Raw record payload from the client push request. - * @return UpsertResult Accepted (true) or rejected (false) with a reason string. + * @param User $user Authenticated user. + * @param string $id Session UUID from the client row. + * @param array $row Full client row payload (must contain deck_id). + * @return string|null 'missing_id', 'forbidden', or null to proceed. */ - public function upsert(User $user, array $row): UpsertResult + protected function checkOwnership(User $user, string $id, array $row): ?string { - $id = (string) ($row['id'] ?? ''); $deckId = (string) ($row['deck_id'] ?? ''); - $incoming = (int) ($row['updated_at_ms'] ?? 0); - - if ($id === '' || $deckId === '') { - return new UpsertResult(false, 'missing_id'); + if ($deckId === '') { + return 'missing_id'; } - // Ownership: the deck must exist and belong to the authenticated user. $deck = Deck::find($deckId); - if (! $deck || $deck->user_id !== $user->id) { - return new UpsertResult(false, 'forbidden'); - } - $existing = Session::find($id); - - // LWW: reject stale writes regardless of whether the row was just found. - if ($existing && $existing->updated_at_ms >= $incoming) { - return new UpsertResult(false, 'stale'); - } + return (! $deck || $deck->user_id !== $user->id) ? 'forbidden' : null; + } - // Use firstOrNew + explicit id assignment to avoid HasUuids overwriting the - // client-supplied UUID (id is not in $fillable, so updateOrCreate's where-clause - // id would not reach setUniqueIds' empty check). - $session = Session::firstOrNew(['id' => $id]); - $session->id = $id; - $session->fill([ - 'user_id' => $user->id, - 'deck_id' => $deckId, - 'mode' => (string) ($row['mode'] ?? 'smart'), - 'started_at_ms' => (int) ($row['started_at_ms'] ?? 0), - 'ended_at_ms' => isset($row['ended_at_ms']) ? (int) $row['ended_at_ms'] : null, - 'cards_reviewed' => (int) ($row['cards_reviewed'] ?? 0), - 'accuracy_pct' => (float) ($row['accuracy_pct'] ?? 0), - 'mastery_delta' => (float) ($row['mastery_delta'] ?? 0), - 'updated_at_ms' => $incoming, - 'deleted_at_ms' => isset($row['deleted_at_ms']) ? (int) $row['deleted_at_ms'] : null, - ]); - $session->save(); + /** + * Map client row fields onto the Session model. + * + * ended_at_ms uses preserve() so that a push omitting the key does not + * overwrite an existing value with null (partial-update semantics). + * + * @param Model $model Session model instance (new or fetched). + * @param User $user Authenticated owner; set as user_id on the session. + * @param array $row Client row payload. + * @param Model|null $existing Pre-lock existing record, or null on create. + */ + protected function applyFields(Model $model, User $user, array $row, ?Model $existing): void + { + assert($model instanceof Session); - return new UpsertResult(true); + $model->user_id = $user->id; + $model->deck_id = (string) ($row['deck_id'] ?? ''); + $model->mode = (string) ($row['mode'] ?? 'smart'); + $model->started_at_ms = (int) ($row['started_at_ms'] ?? 0); + $model->ended_at_ms = $this->preserve($row, $existing, 'ended_at_ms', castInt: true); + $model->cards_reviewed = (int) ($row['cards_reviewed'] ?? 0); + $model->accuracy_pct = (float) ($row['accuracy_pct'] ?? 0); + $model->mastery_delta = (float) ($row['mastery_delta'] ?? 0); } } diff --git a/api/app/Services/Sync/Entities/SubTopicReader.php b/api/app/Services/Sync/Entities/SubTopicReader.php index 14b7b0d..3b30d91 100644 --- a/api/app/Services/Sync/Entities/SubTopicReader.php +++ b/api/app/Services/Sync/Entities/SubTopicReader.php @@ -13,17 +13,16 @@ * Dependencies: * - App\Models\SubTopic (Eloquent model, HasUuids) * - App\Models\User (ownership scoping via decks() relation) - * - App\Services\Sync\RecordReader (interface contract) + * - App\Services\Sync\AbstractCursorReader (base class; handles pagination, + * cursor computation, and the [rows, hasMore, maxUpdatedMs] tuple) * * Key concepts: * - Ownership: SubTopic has no direct user_id column. Ownership is enforced * by constraining deck_id to the set of deck IDs owned by the user, resolved * via a subquery on $user->decks()->select('id'). This is why the User model * must expose a decks() HasMany relation. - * - Pagination: queries pageSize + 1 rows; if the result exceeds pageSize, - * hasMore=true is returned and the extra row is discarded before mapping. - * - nextSince: the maximum updated_at_ms in the returned page is passed back - * to the client as the cursor for the next pull request. + * - Pagination: one-extra-row trick; see AbstractCursorReader. + * - nextSince: max updated_at_ms in page → client's next `since`. * - Ordered by updated_at_ms ASC so cursored pagination is stable. */ @@ -31,59 +30,60 @@ use App\Models\SubTopic; use App\Models\User; -use App\Services\Sync\RecordReader; +use App\Services\Sync\AbstractCursorReader; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\Model; /** * Reads SubTopic records accessible to the authenticated user updated after a given timestamp. * - * Implements the RecordReader contract for the "sub_topics" entity key. - * Ownership is enforced via a subquery on the user's owned deck IDs. + * Implements the RecordReader contract for the "sub_topics" entity key via + * AbstractCursorReader. Ownership is enforced via a whereIn subquery on the + * user's owned deck IDs. */ -final class SubTopicReader implements RecordReader +final class SubTopicReader extends AbstractCursorReader { /** - * Fetch SubTopic rows for the user changed after $since, up to $pageSize rows. - * - * Pagination contract: queries $pageSize + 1 rows. If the result set exceeds - * $pageSize, the extra row signals that more data exists (hasMore=true) and is - * discarded before serialisation. The returned maxUpdatedMs is the highest - * updated_at_ms in the page — the client sends this back as `since` on the - * next pull to advance the cursor. + * @return class-string + */ + protected function modelClass(): string + { + return SubTopic::class; + } + + /** + * Scope the query to sub-topics belonging to decks owned by the user. * - * Ownership is enforced by restricting deck_id to the subquery - * $user->decks()->select('id'), which returns only decks owned by the user. + * Uses a whereIn subquery on $user->decks()->select('id') so that ownership + * is enforced without a JOIN — the User model must expose a decks() HasMany. * - * @param User $user Owner of the records; results are scoped to this user's decks. - * @param int $since Millisecond timestamp lower-bound (exclusive); only rows updated after this are returned. - * @param int $pageSize Maximum number of rows to include in the returned page. - * @return array{0: list>, 1: bool, 2: int} - * [rows, hasMore, maxUpdatedMs] + * @param Builder $query Base query for the SubTopic model. + * @param User $user Authenticated user. + * @return Builder Scoped query. */ - public function read(User $user, int $since, int $pageSize): array + protected function scopeForUser(Builder $query, User $user): Builder { - $rows = SubTopic::query() - ->whereIn('deck_id', $user->decks()->select('id')) - ->where('updated_at_ms', '>', $since) - ->orderBy('updated_at_ms') - ->limit($pageSize + 1) - ->get(); + return $query->whereIn('deck_id', $user->decks()->select('id')); + } - $hasMore = $rows->count() > $pageSize; - $page = $rows->take($pageSize); - $max = (int) ($page->max('updated_at_ms') ?? $since); + /** + * Project a SubTopic model into the wire-format row. + * + * @param Model $model Hydrated SubTopic instance. + * @return array Wire-format row. + */ + protected function projectRow(Model $model): array + { + assert($model instanceof SubTopic); return [ - $page->map(fn (SubTopic $st) => [ - 'id' => $st->id, - 'deck_id' => $st->deck_id, - 'name' => $st->name, - 'position' => $st->position, - 'color_hint' => $st->color_hint, - 'updated_at_ms' => $st->updated_at_ms, - 'deleted_at_ms' => $st->deleted_at_ms, - ])->values()->all(), - $hasMore, - $max, + 'id' => $model->id, + 'deck_id' => $model->deck_id, + 'name' => $model->name, + 'position' => $model->position, + 'color_hint' => $model->color_hint, + 'updated_at_ms' => $model->updated_at_ms, + 'deleted_at_ms' => $model->deleted_at_ms, ]; } } diff --git a/api/app/Services/Sync/Entities/SubTopicUpserter.php b/api/app/Services/Sync/Entities/SubTopicUpserter.php index ea0e566..aa2eaf8 100644 --- a/api/app/Services/Sync/Entities/SubTopicUpserter.php +++ b/api/app/Services/Sync/Entities/SubTopicUpserter.php @@ -12,17 +12,19 @@ * a direct user_id column on the sub_topics table. * * Dependencies: - * - App\Models\SubTopic (Eloquent model, HasUuids) * - App\Models\Deck (parent entity; ownership resolved via deck->user_id) + * - App\Models\SubTopic (Eloquent model, HasUuids) * - App\Models\User (ownership scoping) - * - App\Services\Sync\RecordUpserter (interface contract) - * - App\Services\Sync\UpsertResult (return value object) + * - App\Services\Sync\AbstractLwwUpserter (base class; handles transaction, + * LWW check, UUID preservation, partial-update semantics) * * Key concepts: * - LWW rule: incoming row is rejected when existing updated_at_ms >= incoming updated_at_ms. * - Forbidden: the referenced deck must exist and be owned by the authenticated user. * Unlike top-level entities (Topic, Deck), there is no user_id column on sub_topics; * ownership is enforced via the deck's user_id. + * - Partial updates: color_hint uses preserve() so absent keys do not overwrite + * existing values with null. * - Stateless: no constructor dependencies; safe to resolve per-request via app(). */ @@ -31,68 +33,67 @@ use App\Models\Deck; use App\Models\SubTopic; use App\Models\User; -use App\Services\Sync\RecordUpserter; -use App\Services\Sync\UpsertResult; +use App\Services\Sync\AbstractLwwUpserter; +use Illuminate\Database\Eloquent\Model; /** * Upserts a single SubTopic record for the authenticated user. * - * Implements the RecordUpserter contract for the "sub_topics" entity key. - * Ownership is enforced via the parent deck's user_id — the sub_topics table - * has no direct user_id column. + * Implements the RecordUpserter contract for the "sub_topics" entity key via + * AbstractLwwUpserter. Ownership is enforced via the parent deck's user_id — + * the sub_topics table has no direct user_id column. */ -final class SubTopicUpserter implements RecordUpserter +final class SubTopicUpserter extends AbstractLwwUpserter { /** - * Attempt to upsert a SubTopic row using last-write-wins conflict resolution. - * - * LWW rule: if an existing row's updated_at_ms is >= the incoming value, - * the update is considered stale and rejected with reason "stale". This - * ensures a slower client cannot overwrite newer server state. + * @return class-string + */ + protected function modelClass(): string + { + return SubTopic::class; + } + + /** + * Reject if deck_id is missing or the referenced deck is not owned by the user. * - * Ownership check: the referenced deck_id must belong to the authenticated - * user. If the deck does not exist or belongs to another user, the row is - * rejected with reason "forbidden". + * Also returns 'missing_id' when deck_id is absent, since a SubTopic without + * a parent deck is structurally invalid. * - * @param User $user Authenticated owner; used to verify deck ownership. - * @param array $row Raw record payload from the client push request. - * @return UpsertResult Accepted (true) or rejected (false) with a reason string. + * @param User $user Authenticated user. + * @param string $id SubTopic UUID from the client row. + * @param array $row Full client row payload (must contain deck_id). + * @return string|null 'missing_id', 'forbidden', or null to proceed. */ - public function upsert(User $user, array $row): UpsertResult + protected function checkOwnership(User $user, string $id, array $row): ?string { - $id = (string) ($row['id'] ?? ''); $deckId = (string) ($row['deck_id'] ?? ''); - $incoming = (int) ($row['updated_at_ms'] ?? 0); - - if ($id === '' || $deckId === '') { - return new UpsertResult(false, 'missing_id'); + if ($deckId === '') { + return 'missing_id'; } $deck = Deck::find($deckId); - if (! $deck || $deck->user_id !== $user->id) { - return new UpsertResult(false, 'forbidden'); - } - $existing = SubTopic::find($id); - if ($existing && $existing->updated_at_ms >= $incoming) { - return new UpsertResult(false, 'stale'); - } + return (! $deck || $deck->user_id !== $user->id) ? 'forbidden' : null; + } - // Use firstOrNew + explicit id assignment to avoid HasUuids overwriting the - // client-supplied UUID during mass-assignment (id is not in $fillable, so - // updateOrCreate's where-clause id would not reach setUniqueIds' empty check). - $subTopic = SubTopic::firstOrNew(['id' => $id]); - $subTopic->id = $id; - $subTopic->fill([ - 'deck_id' => $deckId, - 'name' => (string) ($row['name'] ?? ''), - 'position' => (int) ($row['position'] ?? 0), - 'color_hint' => $row['color_hint'] ?? null, - 'updated_at_ms' => $incoming, - 'deleted_at_ms' => isset($row['deleted_at_ms']) ? (int) $row['deleted_at_ms'] : null, - ]); - $subTopic->save(); + /** + * Map client row fields onto the SubTopic model. + * + * color_hint uses preserve() so that a push omitting the key does not + * overwrite an existing value with null (partial-update semantics). + * + * @param Model $model SubTopic model instance (new or fetched). + * @param User $user Authenticated owner (unused here; ownership is via deck). + * @param array $row Client row payload. + * @param Model|null $existing Pre-lock existing record, or null on create. + */ + protected function applyFields(Model $model, User $user, array $row, ?Model $existing): void + { + assert($model instanceof SubTopic); - return new UpsertResult(true); + $model->deck_id = (string) ($row['deck_id'] ?? ''); + $model->name = (string) ($row['name'] ?? ''); + $model->position = (int) ($row['position'] ?? 0); + $model->color_hint = $this->preserve($row, $existing, 'color_hint'); } } diff --git a/api/app/Services/Sync/Entities/TopicReader.php b/api/app/Services/Sync/Entities/TopicReader.php index 3f3cdba..8508996 100644 --- a/api/app/Services/Sync/Entities/TopicReader.php +++ b/api/app/Services/Sync/Entities/TopicReader.php @@ -13,13 +13,13 @@ * Dependencies: * - App\Models\Topic (Eloquent model, HasUuids) * - App\Models\User (ownership scoping) - * - App\Services\Sync\RecordReader (interface contract) + * - App\Services\Sync\AbstractCursorReader (base class; handles pagination, + * cursor computation, and the [rows, hasMore, maxUpdatedMs] tuple) * * Key concepts: - * - Pagination: queries pageSize + 1 rows; if the result exceeds pageSize, - * hasMore=true is returned and the extra row is discarded before mapping. - * - nextSince: the maximum updated_at_ms in the returned page is passed back - * to the client as the cursor for the next pull request. + * - Ownership: scoped by user_id directly on the topics table. + * - Pagination: one-extra-row trick; see AbstractCursorReader. + * - nextSince: max updated_at_ms in page → client's next `since`. * - Ordered by updated_at_ms ASC so cursored pagination is stable. */ @@ -27,53 +27,55 @@ use App\Models\Topic; use App\Models\User; -use App\Services\Sync\RecordReader; +use App\Services\Sync\AbstractCursorReader; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\Model; /** * Reads Topic records for the authenticated user updated after a given timestamp. * - * Implements the RecordReader contract for the "topics" entity key. + * Implements the RecordReader contract for the "topics" entity key via + * AbstractCursorReader. Only entity-specific concerns are declared here: + * model class, ownership scope, and row projection. */ -final class TopicReader implements RecordReader +final class TopicReader extends AbstractCursorReader { /** - * Fetch Topic rows for the user changed after $since, up to $pageSize rows. - * - * Pagination contract: queries $pageSize + 1 rows. If the result set exceeds - * $pageSize, the extra row signals that more data exists (hasMore=true) and is - * discarded before serialisation. The returned maxUpdatedMs is the highest - * updated_at_ms in the page — the client sends this back as `since` on the - * next pull to advance the cursor. + * @return class-string + */ + protected function modelClass(): string + { + return Topic::class; + } + + /** + * Scope the query to topics owned by the authenticated user. * - * @param User $user Owner of the records; results are scoped to this user. - * @param int $since Millisecond timestamp lower-bound (exclusive); only rows updated after this are returned. - * @param int $pageSize Maximum number of rows to include in the returned page. - * @return array{0: list>, 1: bool, 2: int} - * [rows, hasMore, maxUpdatedMs] + * @param Builder $query Base query for the Topic model. + * @param User $user Authenticated user. + * @return Builder Scoped query. */ - public function read(User $user, int $since, int $pageSize): array + protected function scopeForUser(Builder $query, User $user): Builder { - $rows = Topic::query() - ->where('user_id', $user->id) - ->where('updated_at_ms', '>', $since) - ->orderBy('updated_at_ms') - ->limit($pageSize + 1) - ->get(); + return $query->where('user_id', $user->id); + } - $hasMore = $rows->count() > $pageSize; - $page = $rows->take($pageSize); - $max = (int) ($page->max('updated_at_ms') ?? $since); + /** + * Project a Topic model into the wire-format row. + * + * @param Model $model Hydrated Topic instance. + * @return array Wire-format row. + */ + protected function projectRow(Model $model): array + { + assert($model instanceof Topic); return [ - $page->map(fn (Topic $t) => [ - 'id' => $t->id, - 'name' => $t->name, - 'color_hint' => $t->color_hint, - 'updated_at_ms' => $t->updated_at_ms, - 'deleted_at_ms' => $t->deleted_at_ms, - ])->values()->all(), - $hasMore, - $max, + 'id' => $model->id, + 'name' => $model->name, + 'color_hint' => $model->color_hint, + 'updated_at_ms' => $model->updated_at_ms, + 'deleted_at_ms' => $model->deleted_at_ms, ]; } } diff --git a/api/app/Services/Sync/Entities/TopicUpserter.php b/api/app/Services/Sync/Entities/TopicUpserter.php index e25dc51..63e8b74 100644 --- a/api/app/Services/Sync/Entities/TopicUpserter.php +++ b/api/app/Services/Sync/Entities/TopicUpserter.php @@ -13,12 +13,14 @@ * Dependencies: * - App\Models\Topic (Eloquent model, HasUuids) * - App\Models\User (ownership scoping) - * - App\Services\Sync\RecordUpserter (interface contract) - * - App\Services\Sync\UpsertResult (return value object) + * - App\Services\Sync\AbstractLwwUpserter (base class; handles transaction, + * LWW check, UUID preservation, partial-update semantics) * * Key concepts: * - LWW rule: incoming row is rejected when existing updated_at_ms >= incoming updated_at_ms. * - Forbidden: a row whose UUID exists but belongs to a different user is rejected. + * - Partial updates: color_hint uses preserve() so an absent key does not + * overwrite an existing value with null. * - Stateless: no constructor dependencies; safe to resolve per-request via app(). */ @@ -26,60 +28,58 @@ use App\Models\Topic; use App\Models\User; -use App\Services\Sync\RecordUpserter; -use App\Services\Sync\UpsertResult; +use App\Services\Sync\AbstractLwwUpserter; +use Illuminate\Database\Eloquent\Model; /** * Upserts a single Topic record for the authenticated user. * - * Implements the RecordUpserter contract for the "topics" entity key. + * Implements the RecordUpserter contract for the "topics" entity key via + * AbstractLwwUpserter. Only entity-specific concerns are declared here: + * model class, ownership check, and field mapping. */ -final class TopicUpserter implements RecordUpserter +final class TopicUpserter extends AbstractLwwUpserter { /** - * Attempt to upsert a Topic row using last-write-wins conflict resolution. - * - * LWW rule: if an existing row's updated_at_ms is >= the incoming value, - * the update is considered stale and rejected with reason "stale". This - * ensures a slower client cannot overwrite newer server state. - * - * @param User $user Authenticated owner; used to scope creation and enforce access. - * @param array $row Raw record payload from the client push request. - * @return UpsertResult Accepted (true) or rejected (false) with a reason string. + * @return class-string */ - public function upsert(User $user, array $row): UpsertResult + protected function modelClass(): string { - $id = (string) ($row['id'] ?? ''); - $incoming = (int) ($row['updated_at_ms'] ?? 0); - - if ($id === '') { - return new UpsertResult(false, 'missing_id'); - } + return Topic::class; + } + /** + * Reject if a topic with this id exists and belongs to a different user. + * + * @param User $user Authenticated user. + * @param string $id Topic UUID from the client row. + * @param array $row Full client row payload. + * @return string|null 'forbidden' if ownership fails; null to proceed. + */ + protected function checkOwnership(User $user, string $id, array $row): ?string + { $existing = Topic::find($id); - if ($existing && $existing->user_id !== $user->id) { - return new UpsertResult(false, 'forbidden'); - } - - if ($existing && $existing->updated_at_ms >= $incoming) { - return new UpsertResult(false, 'stale'); - } + return $existing && $existing->user_id !== $user->id ? 'forbidden' : null; + } - // Use firstOrNew + explicit id assignment to avoid HasUuids overwriting the - // client-supplied UUID during mass-assignment (id is not in $fillable, so - // updateOrCreate's where-clause id would not reach setUniqueIds' empty check). - $topic = Topic::firstOrNew(['id' => $id]); - $topic->id = $id; - $topic->fill([ - 'user_id' => $user->id, - 'name' => (string) ($row['name'] ?? ''), - 'color_hint' => $row['color_hint'] ?? null, - 'updated_at_ms' => $incoming, - 'deleted_at_ms' => isset($row['deleted_at_ms']) ? (int) $row['deleted_at_ms'] : null, - ]); - $topic->save(); + /** + * Map client row fields onto the Topic model. + * + * color_hint uses preserve() so that a push omitting the key does not + * overwrite an existing color with null (partial-update semantics). + * + * @param Model $model Topic model instance (new or fetched). + * @param User $user Authenticated owner. + * @param array $row Client row payload. + * @param Model|null $existing Pre-lock existing record, or null on create. + */ + protected function applyFields(Model $model, User $user, array $row, ?Model $existing): void + { + assert($model instanceof Topic); - return new UpsertResult(true); + $model->user_id = $user->id; + $model->name = (string) ($row['name'] ?? ''); + $model->color_hint = $this->preserve($row, $existing, 'color_hint'); } } diff --git a/api/tests/Feature/Sync/DeckSyncTest.php b/api/tests/Feature/Sync/DeckSyncTest.php index 780e483..021266d 100644 --- a/api/tests/Feature/Sync/DeckSyncTest.php +++ b/api/tests/Feature/Sync/DeckSyncTest.php @@ -59,6 +59,41 @@ expect($d->fresh()->deleted_at_ms)->toBe(2000); }); +test('partial push preserves omitted nullable fields', function () { + // A deck with an existing description and topic_id. + $u = User::factory()->create(); + $topic = Topic::factory()->for($u)->create(); + $d = Deck::factory()->for($u)->create([ + 'topic_id' => $topic->id, + 'description' => 'Original description', + 'last_studied_at_ms' => 9000, + 'updated_at_ms' => 1000, + ]); + $token = $u->createToken('t')->plainTextToken; + + // Push an update that omits description, topic_id, and last_studied_at_ms entirely. + // The existing values must be preserved — absent keys must not overwrite with null. + $this->withHeader('Authorization', "Bearer {$token}") + ->postJson('/api/v1/sync/push', [ + 'client_clock_ms' => 2000, + 'records' => ['decks' => [[ + 'id' => $d->id, + 'title' => 'Updated Title', + 'accent_color' => $d->accent_color, + 'default_study_mode' => 'smart', + 'card_count' => 0, + 'updated_at_ms' => 2000, + // description, topic_id, last_studied_at_ms intentionally omitted + ]]], + ])->assertOk()->assertJson(['accepted' => 1]); + + $fresh = $d->fresh(); + expect($fresh->title)->toBe('Updated Title') + ->and($fresh->description)->toBe('Original description') + ->and($fresh->topic_id)->toBe($topic->id) + ->and($fresh->last_studied_at_ms)->toBe(9000); +}); + test('pull returns decks since cursor', function () { $u = User::factory()->create(); Deck::factory()->for($u)->create(['title' => 'Old', 'updated_at_ms' => 100]); diff --git a/api/tests/Feature/Sync/TopicSyncTest.php b/api/tests/Feature/Sync/TopicSyncTest.php index 74aef2d..f64d5d0 100644 --- a/api/tests/Feature/Sync/TopicSyncTest.php +++ b/api/tests/Feature/Sync/TopicSyncTest.php @@ -41,6 +41,34 @@ expect($t->fresh()->name)->toBe('Current'); }); +test('partial push preserves omitted color_hint', function () { + // A topic with an existing color_hint value. + $u = User::factory()->create(); + $t = Topic::factory()->for($u)->create([ + 'name' => 'Biology', + 'color_hint' => 'green', + 'updated_at_ms' => 1000, + ]); + $token = $u->createToken('t')->plainTextToken; + + // Push an update that omits color_hint entirely. + // The existing 'green' value must be preserved — an absent key must not overwrite with null. + $this->withHeader('Authorization', "Bearer {$token}") + ->postJson('/api/v1/sync/push', [ + 'client_clock_ms' => 2000, + 'records' => ['topics' => [[ + 'id' => $t->id, + 'name' => 'Biology (renamed)', + 'updated_at_ms' => 2000, + // color_hint intentionally omitted + ]]], + ])->assertOk()->assertJson(['accepted' => 1]); + + $fresh = $t->fresh(); + expect($fresh->name)->toBe('Biology (renamed)') + ->and($fresh->color_hint)->toBe('green'); +}); + test('pull since=0 returns topics owned by user', function () { $u = User::factory()->create(); Topic::factory()->for($u)->create(['name' => 'Alpha', 'updated_at_ms' => 100]);