refactor(sync): AbstractLwwUpserter + AbstractCursorReader + partial-update preservation

api/app/Services/Sync/AbstractCursorReader.php

@@ -0,0 +1,120 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * AbstractCursorReader — shared base for cursor-paginated sync pull readers.
+ *
+ * Purpose:
+ *   Extracts the rule-of-four duplication that exists across TopicReader,
+ *   DeckReader, SubTopicReader, CardReader, and SessionReader into a single
+ *   place. Subclasses declare only what is unique to the entity: the model
+ *   class, a user-ownership scope, and a row-projection closure.
+ *
+ * Dependencies:
+ *   - App\Models\User (ownership scoping)
+ *   - Illuminate\Database\Eloquent\Builder (query building)
+ *   - Illuminate\Database\Eloquent\Model (generic model operations)
+ *   - App\Services\Sync\RecordReader (implemented interface)
+ *
+ * Key concepts:
+ *   - Pagination: queries pageSize + 1 rows; the extra row signals hasMore
+ *     without a separate COUNT query (one-extra-row trick).
+ *   - Cursor: the maximum updated_at_ms in the returned page is passed back
+ *     to the caller as the next `since` value.
+ *   - Ordered by updated_at_ms ASC for stable cursor-based pagination.
+ *
+ * Not suitable for:
+ *   - CardSubTopicReader — uses nested whereHas traversal rather than a
+ *     direct whereIn subquery; the scopeForUser contract does not express that.
+ *   - ReviewReader — append-only semantics; structurally similar but
+ *     kept separate for clarity and future divergence.
+ */
+
+namespace App\Services\Sync;
+
+use App\Models\User;
+use Illuminate\Database\Eloquent\Builder;
+use Illuminate\Database\Eloquent\Model;
+
+/**
+ * Template-method base class for cursor-paginated sync pull readers.
+ *
+ * Subclasses declare:
+ *   - modelClass(): the Eloquent model class-string
+ *   - scopeForUser(): apply user-ownership constraints to the base query
+ *   - projectRow(): map a model instance to the wire-format row array
+ *
+ * The base class handles:
+ *   - updated_at_ms > $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<Model>
+     */
+    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<Model>  $query  The base query for the entity's model.
+     * @param  User  $user  Authenticated user whose records are being fetched.
+     * @return Builder<Model> 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<string, mixed> 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<array<string, mixed>>, 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,
+        ];
+    }
+}

api/app/Services/Sync/AbstractLwwUpserter.php

@@ -0,0 +1,191 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * AbstractLwwUpserter — shared base for last-write-wins upsert logic.
+ *
+ * Purpose:
+ *   Extracts the rule-of-four duplication that exists across TopicUpserter,
+ *   DeckUpserter, SubTopicUpserter, CardUpserter, and SessionUpserter into a
+ *   single place. Subclasses declare only what is unique to the entity:
+ *   the model class, an ownership check, and the field-mapping step.
+ *
+ * Dependencies:
+ *   - App\Models\User (ownership gating)
+ *   - Illuminate\Database\Eloquent\Model (generic model operations)
+ *   - Illuminate\Support\Facades\DB (transactional LWW write)
+ *   - App\Services\Sync\RecordUpserter (implemented interface)
+ *   - App\Services\Sync\UpsertResult (return value object)
+ *
+ * Key concepts:
+ *   - TOCTOU fix: the stale-check and write are wrapped in DB::transaction with
+ *     lockForUpdate(), so concurrent pushes of the same id are serialised at the
+ *     database level and LWW semantics are guaranteed.
+ *   - Partial-update preservation: the preserve() helper returns the EXISTING value
+ *     when a key is absent from $row, and returns the incoming value (even null)
+ *     when the key is present. This prevents absent optional fields from
+ *     silently overwriting existing data.
+ *   - UUID preservation: new models have $model->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<Model>)
+ *   - 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<Model>
+     */
+    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<string, mixed>  $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<string, mixed>  $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<string, mixed>  $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<string, mixed>  $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;
+    }
+}