diff --git a/alembic/versions/0040_label_live_names.py b/alembic/versions/0040_label_live_names.py new file mode 100644 index 0000000..f358b41 --- /dev/null +++ b/alembic/versions/0040_label_live_names.py @@ -0,0 +1,41 @@ +"""labels: a tag's name is unique among LIVE tags only + +Revision ID: 0040 +Revises: 0039 +Create Date: 2026-10-08 + +Deleting a tag now leaves a tombstone row (`purged_at` set) so the change feed can +tell linked devices about it (#5382). Before, the web deleted the row outright and +devices never heard. A tombstone must not hold its name: creating `#grocery` again +after deleting it has to make a live tag, on the web and from a device alike. So +`(owner_id, name)` stays unique only where `purged_at IS NULL`. + +## Downgrade + +Restores the plain unique constraint. Tombstones are deleted first, since one may +share its name with a live tag; devices that have not pulled since keep the tag. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0040" +down_revision = "0039" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.drop_constraint("uq_labels_owner_name", "labels", type_="unique") + op.create_index( + "uq_labels_owner_name_live", + "labels", + ["owner_id", "name"], + unique=True, + postgresql_where=sa.text("purged_at IS NULL"), + ) + + +def downgrade() -> None: + op.drop_index("uq_labels_owner_name_live", table_name="labels") + op.execute("DELETE FROM labels WHERE purged_at IS NOT NULL") + op.create_unique_constraint("uq_labels_owner_name", "labels", ["owner_id", "name"]) diff --git a/src/inkwell/labeling.py b/src/inkwell/labeling.py index 094dc05..aa48fa4 100644 --- a/src/inkwell/labeling.py +++ b/src/inkwell/labeling.py @@ -2,24 +2,49 @@ labels, leave the tag-sourced ones alone" logic was duplicated line-for-line between the labels-picker API (notes.set_note_labels) and sync push (sync._apply_note_manual_labels). Single home so both stay in lockstep. via_tag=True rows track the body #tags and are -governed by _lift_and_reconcile_tags — this function never touches them.""" +governed by _lift_and_reconcile_tags — this function never touches them. + +Also the one definition of a LIVE tag and of deleting one (#5382).""" from __future__ import annotations +from datetime import datetime, timezone + +from sqlalchemy import delete as sa_delete from sqlalchemy import select from .models.label import Label, NoteLabel from .models.note import Note +def live(owner_id): + """`owner_id`'s tags that exist: not tombstoned. Every read of the catalog goes + through this. A tombstone is kept only for the change feed, and one that leaked + into a listing, a name match or a picker would bring a deleted tag back.""" + return (Label.owner_id == owner_id) & Label.purged_at.is_(None) + + +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. + + The ONE way a tag is deleted, from the web (delete, merge) and from a device's + push alike. The row stays, with a new `sync_revision`, so the change feed tells + every linked device. A hard delete sends nothing, and the devices keep the tag + forever. Dropping the links bumps each note too (the 0015 parent trigger). + `edited_at` is a device's edit time, kept as the row's LWW clock.""" + await db.execute(sa_delete(NoteLabel).where(NoteLabel.label_id == label.id)) + label.purged_at = datetime.now(timezone.utc) + if edited_at is not None: + label.updated_at = edited_at + await db.flush() + + async def resolve_owned_label_ids(db, label_ids, owner_id) -> set: - """Of `label_ids` (an iterable of UUIDs), the subset actually owned by `owner_id`. - Callers parse/validate the raw ids first; this just enforces ownership.""" + """Of `label_ids` (an iterable of UUIDs), the subset actually owned by `owner_id` + and live. Callers parse/validate the raw ids first; this just enforces ownership.""" ids = list(label_ids) if not ids: return set() - return set( - (await db.scalars(select(Label.id).where(Label.owner_id == owner_id, Label.id.in_(ids)))).all() - ) + return set((await db.scalars(select(Label.id).where(live(owner_id), Label.id.in_(ids)))).all()) async def reconcile_manual_labels(db, note: Note, owned_label_ids: set) -> None: diff --git a/src/inkwell/labels.py b/src/inkwell/labels.py index 645d06d..5e091ed 100644 --- a/src/inkwell/labels.py +++ b/src/inkwell/labels.py @@ -6,6 +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 .models.label import Label, NoteLabel from .responses import json_error, not_found, parse_uuid from .serialize import serialize_label @@ -28,7 +29,7 @@ async def _label_note_count(db, label_id) -> int: async def _merge_into(db, source: Label, target: Label) -> None: - """Move every note tagged with `source` onto `target`, then delete `source`. + """Move every note tagged with `source` onto `target`, then tombstone `source`. Shared by the explicit `/merge` route and by a rename that lands on a name another tag already holds — those are the same operation, and having one body @@ -53,22 +54,21 @@ async def _merge_into(db, source: Label, target: Label) -> None: for note_id, via_tag in by_note.items(): if note_id not in target_notes: db.add(NoteLabel(note_id=note_id, label_id=target.id, via_tag=via_tag)) - await db.delete(source) - await db.flush() + await tombstone_label(db, source) async def _get_owned_label(db, label_id: str) -> Label | None: lid = parse_uuid(label_id) if lid is None: return None - return await db.scalar(select(Label).where(Label.id == lid, Label.owner_id == g.user_id)) + return await db.scalar(select(Label).where(Label.id == lid, live(g.user_id))) @bp.get("") @login_required async def list_labels(): async with session_scope() as db: - labels = (await db.scalars(select(Label).where(Label.owner_id == g.user_id).order_by(Label.name))).all() + labels = (await db.scalars(select(Label).where(live(g.user_id)).order_by(Label.name))).all() # One grouped query for all usage counts (0 for labels attached to nothing). counts = dict( ( @@ -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(Label.owner_id == g.user_id, func.lower(Label.name) == name.lower()) + select(Label).where(live(g.user_id), func.lower(Label.name) == name.lower()) ) if existing is not None: return jsonify(_serialize_label(existing)), 200 @@ -129,7 +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( - Label.owner_id == g.user_id, + live(g.user_id), func.lower(Label.name) == name.lower(), Label.id != label.id, ) @@ -162,7 +162,7 @@ async def delete_label(label_id: str): label = await _get_owned_label(db, label_id) if label is None: return not_found() - await db.delete(label) # note_labels rows cascade + await tombstone_label(db, label) await db.commit() return jsonify({"ok": True}) @@ -171,7 +171,7 @@ async def delete_label(label_id: str): @login_required async def merge_label(label_id: str): """Merge `label_id` (source) INTO the label given by body {"into": }: move - every note tagged with the source onto the target, then delete the source. Both + every note tagged with the source onto the target, then tombstone the source. Both must be owned by the caller. Note-body `#tags` are NOT rewritten, so a note whose body still literally contains the source #tag will re-mint that label on its next edit — retire a tag by editing it out of the text (a known, documented nuance).""" diff --git a/src/inkwell/models/label.py b/src/inkwell/models/label.py index f57d408..6425d41 100644 --- a/src/inkwell/models/label.py +++ b/src/inkwell/models/label.py @@ -3,7 +3,7 @@ from __future__ import annotations import uuid from datetime import datetime -from sqlalchemy import BigInteger, Boolean, DateTime, ForeignKey, Text, UniqueConstraint, func +from sqlalchemy import BigInteger, Boolean, DateTime, ForeignKey, Index, Text, func, text from sqlalchemy.dialects.postgresql import UUID from sqlalchemy.orm import Mapped, mapped_column @@ -12,7 +12,17 @@ from . import Base class Label(Base): __tablename__ = "labels" - __table_args__ = (UniqueConstraint("owner_id", "name", name="uq_labels_owner_name"),) + # A name is unique among LIVE tags only: a tombstone keeps its row for the change + # feed but gives its name back, so `#grocery` can be made again (#5382, 0040). + __table_args__ = ( + Index( + "uq_labels_owner_name_live", + "owner_id", + "name", + unique=True, + postgresql_where=text("purged_at IS NULL"), + ), + ) id: Mapped[uuid.UUID] = mapped_column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) owner_id: Mapped[uuid.UUID] = mapped_column( @@ -29,7 +39,8 @@ class Label(Base): # Sync (M8): monotonic per-row revision (from sync_revision_seq via DB trigger) so a # label rename/recolor/merge/delete propagates to native clients independently of notes. sync_revision: Mapped[int | None] = mapped_column(BigInteger(), nullable=True) - # Hard-delete tombstone (content-less) so a deleted label is removed on clients. + # Tombstone: set means deleted. The row stays so the change feed can carry the + # delete to devices; every read of the catalog skips it (`labeling.live`). purged_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) diff --git a/src/inkwell/notes/__init__.py b/src/inkwell/notes/__init__.py index 9248bc9..c913bf7 100644 --- a/src/inkwell/notes/__init__.py +++ b/src/inkwell/notes/__init__.py @@ -25,7 +25,7 @@ from ..colors import normalize_color from ..common import coerce_bool, iso, parse_dt from ..config import Config from ..db import session_scope -from ..labeling import reconcile_manual_labels, resolve_owned_label_ids +from ..labeling import live, reconcile_manual_labels, resolve_owned_label_ids from ..models.label import Label, NoteLabel from ..models.note import Note from ..models.note_attachment import NoteAttachment @@ -245,7 +245,7 @@ async def export_notes(): for a in att_rows: att_by_note.setdefault(a.note_id, []).append(a) all_labels = ( - await db.scalars(select(Label).where(Label.owner_id == g.user_id).order_by(Label.name)) + await db.scalars(select(Label).where(live(g.user_id)).order_by(Label.name)) ).all() payload: dict = { diff --git a/src/inkwell/notes/tags.py b/src/inkwell/notes/tags.py index 5cff004..edc046e 100644 --- a/src/inkwell/notes/tags.py +++ b/src/inkwell/notes/tags.py @@ -18,6 +18,7 @@ import re from sqlalchemy import func, select +from ..labeling import live from ..models.label import Label, NoteLabel from ..models.note import Note from .helpers import derive_display_title @@ -120,9 +121,10 @@ def split_body_tags(body: str | None) -> tuple[list[str], list[str], str]: async def _find_or_create_label(db, owner_id, name: str): - """Owner's label id for `name` (case-insensitive match), creating it if absent.""" + """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(Label.owner_id == owner_id, func.lower(Label.name) == name.lower()) + select(Label.id).where(live(owner_id), func.lower(Label.name) == name.lower()) ) if existing is not None: return existing diff --git a/src/inkwell/sync.py b/src/inkwell/sync.py index 23e4b6b..f6d23bb 100644 --- a/src/inkwell/sync.py +++ b/src/inkwell/sync.py @@ -25,7 +25,7 @@ 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 reconcile_manual_labels, resolve_owned_label_ids +from .labeling import live, 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 @@ -395,11 +395,7 @@ async def _apply_label(db, ch: dict) -> dict: return _result(str(lid), "label", "noop") if not client_wins(edited_at, label.updated_at): return _result(str(lid), "label", "kept", sync_revision=label.sync_revision) - await db.execute(sa_delete(NoteLabel).where(NoteLabel.label_id == label.id)) - label.purged_at = datetime.now(timezone.utc) - if edited_at is not None: - label.updated_at = edited_at - await db.flush() + await tombstone_label(db, label, edited_at) await db.refresh(label, ["sync_revision"]) return _result(str(lid), "label", "applied", sync_revision=label.sync_revision) @@ -407,12 +403,11 @@ async def _apply_label(db, ch: dict) -> dict: creating = label is None if not creating and not client_wins(edited_at, label.updated_at): return _result(str(lid), "label", "kept", sync_revision=label.sync_revision) - # Names are unique per owner — a same-name clash on a DIFFERENT id can't be an insert. + # Names are unique among an owner's LIVE tags — a same-name clash on a DIFFERENT + # 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( - Label.owner_id == g.user_id, func.lower(Label.name) == name.lower(), Label.id != lid - ) + select(Label.id).where(live(g.user_id), func.lower(Label.name) == name.lower(), Label.id != lid) ) if clash is not None: return _result(str(lid), "label", "rejected", error="name in use") diff --git a/tests/test_integration.py b/tests/test_integration.py index 85653b3..caa3a88 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -646,6 +646,91 @@ async def test_creating_a_tag_that_differs_only_in_case_returns_the_existing_one assert len(listing) == 1 +# --- A deleted tag is a tombstone on every path (#5382) --------------------------- +# +# The web deleted the row outright, so the change feed never carried the delete and +# devices kept the tag. A device's delete left a tombstone the web still listed and +# still matched by name. One definition now: `labeling.tombstone_label`, and every +# read of the catalog goes through `labeling.live`. + + +async def _label_in_feed(app_client, lid: str) -> dict | None: + feed = await (await app_client.get("/api/sync/changes?since=0")).get_json() + return next((lb for lb in feed["labels"] if lb["id"] == lid), None) + + +async def _push_label(app_client, lid: str, op: str, edited_at: str, name: str = "grocery") -> dict: + change = {"entity": "label", "id": lid, "op": op, "name": name, "edited_at": edited_at} + resp = await app_client.post("/api/sync/push", json={"changes": [change]}) + return (await resp.get_json())["results"][0] + + +async def test_a_tag_deleted_on_the_web_reaches_devices_as_a_tombstone(app_client, db): + await _signed_in(app_client, "webdelete") + tag = await (await app_client.post("/api/labels", json={"name": "grocery"})).get_json() + note = await (await app_client.post("/api/notes", json={"body": "milk"})).get_json() + await app_client.put(f"/api/notes/{note['id']}/labels", json={"label_ids": [tag["id"]]}) + + assert (await app_client.delete(f"/api/labels/{tag['id']}")).status_code == 200 + + row = await _label_in_feed(app_client, tag["id"]) + assert row is not None and row["purged_at"] is not None, "the feed carries the delete" + assert (await (await app_client.get(f"/api/notes/{note['id']}")).get_json())["labels"] == [] + assert (await (await app_client.get("/api/labels")).get_json())["labels"] == [] + assert (await app_client.delete(f"/api/labels/{tag['id']}")).status_code == 404, "gone, not hidden" + + +async def test_a_merge_tombstones_the_tag_it_folds_away(app_client, db): + await _signed_in(app_client, "mergedelete") + source = await (await app_client.post("/api/labels", json={"name": "groceries"})).get_json() + target = await (await app_client.post("/api/labels", json={"name": "shopping"})).get_json() + + resp = await app_client.post(f"/api/labels/{source['id']}/merge", json={"into": target["id"]}) + assert resp.status_code == 200 + + row = await _label_in_feed(app_client, source["id"]) + assert row is not None and row["purged_at"] is not None + listing = (await (await app_client.get("/api/labels")).get_json())["labels"] + assert [lb["id"] for lb in listing] == [target["id"]] + + +async def test_a_tag_deleted_on_a_device_is_gone_from_the_web_and_its_name_is_free(app_client, db): + await _signed_in(app_client, "devicedelete") + lid = str(uuid.uuid4()) + assert (await _push_label(app_client, lid, "upsert", "2026-01-01T00:00:00Z"))["status"] == "created" + assert (await _push_label(app_client, lid, "delete", "2026-01-02T00:00:00Z"))["status"] == "applied" + + assert (await (await app_client.get("/api/labels")).get_json())["labels"] == [] + + # The web makes the name again: a NEW live tag, not the tombstone handed back. + again = await app_client.post("/api/labels", json={"name": "Grocery"}) + assert again.status_code == 201 + assert (await again.get_json())["id"] != lid + + # A body #tag finds that live one, not the tombstone. + note = await (await app_client.post("/api/notes", json={"body": "milk #grocery"})).get_json() + note = await (await app_client.get(f"/api/notes/{note['id']}")).get_json() + assert [lb["id"] for lb in note["labels"]] == [(await again.get_json())["id"]] + + # A picker naming the tombstone attaches nothing. + await app_client.put(f"/api/notes/{note['id']}/labels", json={"label_ids": [lid]}) + labels = (await (await app_client.get(f"/api/notes/{note['id']}")).get_json())["labels"] + assert lid not in [lb["id"] for lb in labels] + + +async def test_a_device_can_make_a_deleted_tags_name_again(app_client, db): + """The tombstone gives its name back (0040), so another device's fresh tag of + the same name is created rather than refused as "name in use".""" + await _signed_in(app_client, "devicerename") + old, new = str(uuid.uuid4()), str(uuid.uuid4()) + await _push_label(app_client, old, "upsert", "2026-01-01T00:00:00Z") + await _push_label(app_client, old, "delete", "2026-01-02T00:00:00Z") + + assert (await _push_label(app_client, new, "upsert", "2026-01-03T00:00:00Z"))["status"] == "created" + listing = (await (await app_client.get("/api/labels")).get_json())["labels"] + assert [lb["id"] for lb in listing] == [new] + + # --- One save path for a note's text (#5164) --------------------------------- # # A body edit is a sequence: keep a revision, rename the note, lift #tags, commit,