From 627c5e43bc85d994ff75dc1befbda692cc7ce269 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 8 Oct 2026 14:37:19 -0400 Subject: [PATCH] DRY pass #2, batch 4, F12: one note fetch, one tag-name match, one push-landed reply (#5372) notes.helpers._fetch_note is the parse-id, not-purged, gated select that _get_owned, _get_visible and _get_editable each wrote out, and note_visible is the viewer's visibility predicate that five note reads spelled in full. labeling.named is the case-insensitive live-name match that tags, labels (create and rename) and the sync push each wrote; how two tag names compare is now said in one place (#5385 will change it there). sync._landed is the flush, read-back-the-revision and reply that four push paths ended with. The REST routes' commit-and-serialise tails stay: each is two lines, and whether a route refreshes the row first differs by route. Co-Authored-By: Claude Opus 5.5 --- src/inkwell/labeling.py | 9 ++++++- src/inkwell/labels.py | 7 +++--- src/inkwell/notes/__init__.py | 8 +++---- src/inkwell/notes/helpers.py | 45 +++++++++++++++-------------------- src/inkwell/notes/tags.py | 4 ++-- src/inkwell/sync.py | 34 +++++++++++++------------- 6 files changed, 52 insertions(+), 55 deletions(-) diff --git a/src/inkwell/labeling.py b/src/inkwell/labeling.py index aa48fa4..9b9b5a6 100644 --- a/src/inkwell/labeling.py +++ b/src/inkwell/labeling.py @@ -10,7 +10,7 @@ from __future__ import annotations from datetime import datetime, timezone from sqlalchemy import delete as sa_delete -from sqlalchemy import select +from sqlalchemy import func, select from .models.label import Label, NoteLabel from .models.note import Note @@ -23,6 +23,13 @@ def live(owner_id): return (Label.owner_id == owner_id) & Label.purged_at.is_(None) +def named(owner_id, name: str): + """`owner_id`'s live tag called `name`, in any case. Names are unique among an + owner's live tags without regard to case, because every client's store is; this + is the one place that says how two names compare.""" + return live(owner_id) & (func.lower(Label.name) == name.lower()) + + async def tombstone_label(db, label: Label, edited_at: datetime | None = None) -> None: """Delete a tag: detach it from every note and mark the row purged. diff --git a/src/inkwell/labels.py b/src/inkwell/labels.py index 5e091ed..53ac91d 100644 --- a/src/inkwell/labels.py +++ b/src/inkwell/labels.py @@ -6,7 +6,7 @@ from sqlalchemy import func, select from .auth import login_required from .colors import normalize_color from .db import session_scope -from .labeling import live, tombstone_label +from .labeling import live, named, tombstone_label from .models.label import Label, NoteLabel from .responses import json_error, not_found, parse_uuid from .serialize import serialize_label @@ -95,7 +95,7 @@ async def create_label(): # the path that MINTED the "Groceries" beside "groceries" pair that no synced # client can hold, since their `labels` index is unique on `lower(name)`. existing = await db.scalar( - select(Label).where(live(g.user_id), func.lower(Label.name) == name.lower()) + select(Label).where(named(g.user_id, name)) ) if existing is not None: return jsonify(_serialize_label(existing)), 200 @@ -129,8 +129,7 @@ async def update_label(label_id: str): # one be made is letting a pull fail later on a phone. clash = await db.scalar( select(Label).where( - live(g.user_id), - func.lower(Label.name) == name.lower(), + named(g.user_id, name), Label.id != label.id, ) ) diff --git a/src/inkwell/notes/__init__.py b/src/inkwell/notes/__init__.py index 264713a..aea60f9 100644 --- a/src/inkwell/notes/__init__.py +++ b/src/inkwell/notes/__init__.py @@ -19,7 +19,6 @@ from datetime import datetime, timedelta, timezone from quart import Response, g, jsonify, request, send_file from sqlalchemy import func, literal_column, select -from ..acl import visible_to_user from ..auth import login_required from ..colors import normalize_color from ..common import coerce_bool, iso, parse_dt @@ -52,6 +51,7 @@ from .helpers import ( apply_filter, derive_display_title, is_empty_note, + note_visible, store_attachment, top_position, parse_list_items, @@ -121,9 +121,7 @@ async def list_notes(): async with session_scope() as db: # Through the viewer's own pin, archive and place: on a note shared with them # those are theirs, not the owner's (#5176). - stmt = join_state(select(Note), g.user_id).where( - visible_to_user("note", Note.owner_id, Note.id, g.user_id) - ) + stmt = join_state(select(Note), g.user_id).where(note_visible()) stmt = apply_filter(stmt, filter_name, archived_for(g.user_id)) # Trash is the owner's: a note its owner trashed leaves a recipient's board # rather than turning up in their Trash, where they could do nothing with it. @@ -392,7 +390,7 @@ async def reorder_notes(): await db.scalars( select(Note).where( Note.id.in_(parsed), - visible_to_user("note", Note.owner_id, Note.id, g.user_id), + note_visible(), Note.purged_at.is_(None), ) ) diff --git a/src/inkwell/notes/helpers.py b/src/inkwell/notes/helpers.py index 12d998f..c8bb5d8 100644 --- a/src/inkwell/notes/helpers.py +++ b/src/inkwell/notes/helpers.py @@ -103,6 +103,22 @@ async def top_position(db, owner_id) -> int: ) +def note_visible(permission: str | None = None): + """Predicate: the current user may see the note, as its owner or through a share + (at `permission`, when one is given). Every note read scoped to a viewer asks + through here.""" + return visible_to_user("note", Note.owner_id, Note.id, g.user_id, permission=permission) + + +async def _fetch_note(db, note_id: str, gate) -> Note | None: + """The one shape of a per-note fetch: a well-formed id, a note that isn't purged, + and `gate` saying who may have it. None for anything else, so the route 404s.""" + nid = parse_uuid(note_id) + if nid is None: + return None + return await db.scalar(select(Note).where(Note.id == nid, gate, Note.purged_at.is_(None))) + + async def _get_owned(db, note_id: str) -> Note | None: """Fetch a note the current user OWNS, for every change only an owner may make: trash, delete, labels, reminders, attachments, previews, history and sharing @@ -116,44 +132,21 @@ async def _get_owned(db, note_id: str) -> Note | None: editing or restoring one 404s. The sync push path looks rows up directly rather than through here, which is what still lets a client re-create an id it owns. """ - nid = parse_uuid(note_id) - if nid is None: - return None - return await db.scalar( - select(Note).where(Note.id == nid, Note.owner_id == g.user_id, Note.purged_at.is_(None)) - ) + return await _fetch_note(db, note_id, Note.owner_id == g.user_id) async def _get_visible(db, note_id: str) -> Note | None: """Fetch a note the current user can see: theirs, or shared with them at any level. What a recipient may do to it from here is their own pin, archive and place (#5176); `_get_editable` and `_get_owned` gate the rest.""" - nid = parse_uuid(note_id) - if nid is None: - return None - return await db.scalar( - select(Note).where( - Note.id == nid, - visible_to_user("note", Note.owner_id, Note.id, g.user_id), - Note.purged_at.is_(None), - ) - ) + return await _fetch_note(db, note_id, note_visible()) async def _get_editable(db, note_id: str) -> Note | None: """Fetch a note the current user may change the TEXT of: its body and its checklist. The owner, or someone it is shared with at `edit`; a view share gets None, and so a 404, like a stranger (#5174).""" - nid = parse_uuid(note_id) - if nid is None: - return None - return await db.scalar( - select(Note).where( - Note.id == nid, - visible_to_user("note", Note.owner_id, Note.id, g.user_id, permission=EDIT), - Note.purged_at.is_(None), - ) - ) + return await _fetch_note(db, note_id, note_visible(EDIT)) def _slugify(text: str) -> str: diff --git a/src/inkwell/notes/tags.py b/src/inkwell/notes/tags.py index edc046e..1c3f815 100644 --- a/src/inkwell/notes/tags.py +++ b/src/inkwell/notes/tags.py @@ -18,7 +18,7 @@ import re from sqlalchemy import func, select -from ..labeling import live +from ..labeling import named from ..models.label import Label, NoteLabel from ..models.note import Note from .helpers import derive_display_title @@ -124,7 +124,7 @@ async def _find_or_create_label(db, owner_id, name: str): """Owner's live label id for `name` (case-insensitive match), creating it if absent. A deleted tag's tombstone never matches: tagging a note with it again makes a new tag.""" existing = await db.scalar( - select(Label.id).where(live(owner_id), func.lower(Label.name) == name.lower()) + select(Label.id).where(named(owner_id, name)) ) if existing is not None: return existing diff --git a/src/inkwell/sync.py b/src/inkwell/sync.py index 3f72a5f..396febf 100644 --- a/src/inkwell/sync.py +++ b/src/inkwell/sync.py @@ -21,11 +21,10 @@ from quart import Blueprint, g, jsonify, request from sqlalchemy import delete as sa_delete from sqlalchemy import func, select -from .acl import visible_to_user from .auth import login_required from .common import iso, parse_dt from .db import session_scope -from .labeling import live, reconcile_manual_labels, resolve_owned_label_ids, tombstone_label +from .labeling import live, named, reconcile_manual_labels, resolve_owned_label_ids, tombstone_label from .models.label import Label, NoteLabel from .models.note import Note from .models.note_attachment import NoteAttachment @@ -38,7 +37,7 @@ from .notes import ( normalize_recurrence, ) from .notes.body import write_body -from .notes.helpers import _get_editable, _get_visible, store_attachment +from .notes.helpers import _get_editable, _get_visible, note_visible, store_attachment from .responses import json_error, not_found, parse_uuid from .retention import purge_note from .serialize import serialize_label_sync @@ -140,7 +139,7 @@ async def changes(): # notes it would treat as its own: pinning them, labelling them, pushing them. with_shares = request.args.get("shares") == "1" if with_shares: - visible = visible_to_user("note", Note.owner_id, Note.id, g.user_id) + visible = note_visible() else: visible = Note.owner_id == g.user_id async with session_scope() as db: @@ -268,6 +267,14 @@ def _result(rid, entity: str, status: str, **extra) -> dict: return {"id": rid, "entity": entity, "status": status, **extra} +async def _landed(db, row, entity: str, status: str) -> dict: + """The reply for a push that changed `row`: flush it, read back the revision the + trigger gave it, and report that, so the client's cursor never runs ahead of it.""" + await db.flush() + await db.refresh(row, ["sync_revision"]) + return _result(str(row.id), entity, status, sync_revision=row.sync_revision) + + async def _apply_note(db, ch: dict, previews: list[tuple[uuid.UUID, str]]) -> dict: """Apply one pushed note. A note whose text changed is appended to `previews` as (id, final body), for the caller to queue link previews once the batch commits.""" @@ -288,9 +295,7 @@ async def _apply_note(db, ch: dict, previews: list[tuple[uuid.UUID, str]]) -> di if not client_wins(edited_at, note.updated_at): return _result(str(nid), "note", "kept", sync_revision=note.sync_revision) await purge_note(db, note, edited_at) - await db.flush() - await db.refresh(note, ["sync_revision"]) - return _result(str(nid), "note", "applied", sync_revision=note.sync_revision) + return await _landed(db, note, "note", "applied") creating = note is None if creating: @@ -321,9 +326,7 @@ async def _apply_note(db, ch: dict, previews: list[tuple[uuid.UUID, str]]) -> di if edited_at is not None: note.updated_at = edited_at await _apply_note_manual_labels(db, note, ch) - await db.flush() - await db.refresh(note, ["sync_revision"]) - return _result(str(nid), "note", "created" if creating else "applied", sync_revision=note.sync_revision) + return await _landed(db, note, "note", "created" if creating else "applied") async def _apply_shared_note(db, note: Note, ch: dict, edited_at, previews: list[tuple[uuid.UUID, str]]) -> dict: @@ -374,7 +377,7 @@ async def _apply_shared_note(db, note: Note, ch: dict, edited_at, previews: list join_state(select(revision_for()).select_from(Note), g.user_id).where(Note.id == note.id) ) status = "kept" if kept and not applied else "applied" - return {"id": nid, "entity": "note", "status": status, "sync_revision": revision} + return _result(nid, "note", status, sync_revision=revision) async def _apply_label(db, ch: dict) -> dict: @@ -396,8 +399,7 @@ async def _apply_label(db, ch: dict) -> dict: if not client_wins(edited_at, label.updated_at): return _result(str(lid), "label", "kept", sync_revision=label.sync_revision) await tombstone_label(db, label, edited_at) - await db.refresh(label, ["sync_revision"]) - return _result(str(lid), "label", "applied", sync_revision=label.sync_revision) + return await _landed(db, label, "label", "applied") name = (ch.get("name") or "").strip() creating = label is None @@ -407,7 +409,7 @@ async def _apply_label(db, ch: dict) -> dict: # id can't be an insert. A tombstone holds no name (0040), so it never clashes. if name: clash = await db.scalar( - select(Label.id).where(live(g.user_id), func.lower(Label.name) == name.lower(), Label.id != lid) + select(Label.id).where(named(g.user_id, name), Label.id != lid) ) if clash is not None: return _result(str(lid), "label", "rejected", error="name in use") @@ -425,9 +427,7 @@ async def _apply_label(db, ch: dict) -> dict: label.color = normalize_color(ch.get("color")) if edited_at is not None: label.updated_at = edited_at - await db.flush() - await db.refresh(label, ["sync_revision"]) - return _result(str(lid), "label", "created" if creating else "applied", sync_revision=label.sync_revision) + return await _landed(db, label, "label", "created" if creating else "applied") # Entities a push may only DELETE. Each is a note's child, so deleting one bumps its