peter
1a1e9d9542
An Esc during a pointer drag is spent on the drag and leaves the selection. A wheel over the rail re-reads the target each frame, so a drop lands on the row under the pointer. After a bulk removal the focus goes to the next row left, and to the list's menu when the list is emptied. remove-bulk names the videos that went, and its Undo brings back only those. An Undo of a move keeps each row (and when it was saved), leaves out a video that moved on from the target, closes up its place, and says how many came back. Delete with every tick hidden removes nothing and says why. A list gone before the lock answers 404, and a body without its videos 422. One removal path now: the single row goes through the bulk one, the same toast and undo, which also retires the add-at-position route. railDrop reuses the store and scrollParent, the drag image the overlay and the z-index token, and the sensors keep their number. The count is read out from a region that outlives the band. Tests fill the gaps the review named, each seen failing.
290 lines
11 KiB
Python
290 lines
11 KiB
Python
"""Every request that reads a playlist's item positions and writes new ones locks the
|
|
playlist row first.
|
|
|
|
Without it two adds (two tabs; the rail and the page) both read the same `max(position)` and land
|
|
on one position, and a reorder racing an add collides with it — the list's order between the two
|
|
rows is then whatever the planner returns. A true two-connection race is not reproducible on
|
|
demand, so the test holds the mechanism: the parent row is taken `FOR UPDATE` by each such route.
|
|
A Plex reorder that omits a row (added in another tab meanwhile) must not collide with it either.
|
|
|
|
The lock also comes FIRST — before the request reads or writes a single item row. A remove
|
|
that deleted an item and then touched the playlist beside a reorder that went the other way was a
|
|
deadlock (a 500 for one of them), and an add that checked "already there?" before the lock let two
|
|
adds of one video both insert (a 500 on the unique key).
|
|
"""
|
|
|
|
import re
|
|
from datetime import datetime, timezone
|
|
|
|
import pytest
|
|
from sqlalchemy import event
|
|
|
|
from app.plex.models import PlexItem, PlexLibrary, PlexPlaylist, PlexPlaylistItem
|
|
from app.catalog.models import Channel, Video
|
|
from app.reader.models import Playlist, PlaylistItem
|
|
|
|
|
|
@pytest.fixture
|
|
def locks(db):
|
|
"""The tables each request took a row lock on (`SELECT … FROM <t> … FOR UPDATE`)."""
|
|
seen: list[str] = []
|
|
|
|
def _record(conn, cursor, statement, *_):
|
|
if "FOR UPDATE" in statement:
|
|
seen.extend(re.findall(r"FROM\s+(\w+)", statement)[:1])
|
|
|
|
engine = db.get_bind()
|
|
event.listen(engine, "before_cursor_execute", _record)
|
|
yield seen
|
|
event.remove(engine, "before_cursor_execute", _record)
|
|
|
|
|
|
@pytest.fixture
|
|
def statements(db):
|
|
"""Every statement a request ran, in order."""
|
|
seen: list[str] = []
|
|
|
|
def _record(conn, cursor, statement, *_):
|
|
seen.append(" ".join(statement.split()))
|
|
|
|
engine = db.get_bind()
|
|
event.listen(engine, "before_cursor_execute", _record)
|
|
yield seen
|
|
event.remove(engine, "before_cursor_execute", _record)
|
|
|
|
|
|
def _locked_before_items(seen: list[str], parent: str, items: str) -> str | None:
|
|
"""The first statement touching `items` must come after a `FOR UPDATE` of `parent`; returns
|
|
that lock statement (None when it came too late or never)."""
|
|
lock = None
|
|
for st in seen:
|
|
if lock is None and "FOR UPDATE" in st and re.search(rf"FROM {parent}\b", st):
|
|
lock = st
|
|
if re.search(rf"\b{items}\b", st):
|
|
return lock
|
|
return lock
|
|
|
|
|
|
def _yt(db, user):
|
|
db.add(Channel(id="UCtest", title="Test"))
|
|
now = datetime.now(timezone.utc)
|
|
for v in ("v1", "v2", "v3"):
|
|
db.add(Video(id=v, channel_id="UCtest", published_at=now))
|
|
pl = Playlist(user_id=user.id, name="L")
|
|
other = Playlist(user_id=user.id, name="M")
|
|
db.add_all([pl, other])
|
|
db.flush()
|
|
db.add(PlaylistItem(playlist_id=pl.id, video_id="v1", position=0))
|
|
db.commit()
|
|
return pl.id, other.id
|
|
|
|
|
|
def _plex(db, user):
|
|
lib = PlexLibrary(plex_key="p1", title="Movies", kind="movie", enabled=True)
|
|
db.add(lib)
|
|
db.flush()
|
|
items = [PlexItem(rating_key=rk, library_id=lib.id, kind="movie", title=rk) for rk in "abcd"]
|
|
db.add_all(items)
|
|
pl = PlexPlaylist(user_id=user.id, title="L")
|
|
db.add(pl)
|
|
db.flush()
|
|
for i, it in enumerate(items[:3]):
|
|
db.add(PlexPlaylistItem(playlist_id=pl.id, item_id=it.id, position=i))
|
|
db.commit()
|
|
return pl.id
|
|
|
|
|
|
YT = Playlist.__table__.name
|
|
PLEX = PlexPlaylist.__table__.name
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"call",
|
|
[
|
|
lambda c, pid, other: c.post(f"/api/playlists/{pid}/items", json={"video_id": "v2"}),
|
|
lambda c, pid, other: c.post("/api/playlists/watch-later", json={"video_id": "v2"}),
|
|
lambda c, pid, other: c.post(
|
|
f"/api/playlists/{pid}/items/move-bulk",
|
|
json={"video_ids": ["v1"], "target_playlist_id": other},
|
|
),
|
|
lambda c, pid, other: c.put(f"/api/playlists/{pid}/order", json={"video_ids": ["v1"]}),
|
|
lambda c, pid, other: c.post(
|
|
f"/api/playlists/{pid}/items/remove-bulk", json={"video_ids": ["v1"]}
|
|
),
|
|
lambda c, pid, other: c.post(
|
|
f"/api/playlists/{pid}/items/restore", json={"video_ids": ["v2"], "positions": [0]}
|
|
),
|
|
lambda c, pid, other: c.post(
|
|
f"/api/playlists/{pid}/items/undo-move",
|
|
json={"from_playlist_id": other, "moved": ["v2"], "positions": [0]},
|
|
),
|
|
],
|
|
ids=["add", "watch-later", "move", "reorder", "remove-bulk", "restore", "undo-move"],
|
|
)
|
|
def test_a_youtube_playlist_write_locks_the_playlist(db, make_user, as_user, locks, call):
|
|
user = make_user()
|
|
pid, other = _yt(db, user)
|
|
r = call(as_user(user), pid, other)
|
|
assert r.status_code == 200, r.text
|
|
assert YT in locks
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"call",
|
|
[
|
|
lambda c, pid: c.post(f"/api/plex/playlists/{pid}/items", json={"item_rating_key": "d"}),
|
|
lambda c, pid: c.post(
|
|
f"/api/plex/playlists/{pid}/items/bulk", json={"item_rating_keys": ["d"]}
|
|
),
|
|
lambda c, pid: c.post(
|
|
f"/api/plex/playlists/{pid}/items/bulk",
|
|
json={"item_rating_keys": ["d"], "positions": [0]},
|
|
),
|
|
lambda c, pid: c.put(
|
|
f"/api/plex/playlists/{pid}/order", json={"item_rating_keys": ["c", "b", "a"]}
|
|
),
|
|
],
|
|
ids=["add", "bulk", "bulk-at", "reorder"],
|
|
)
|
|
def test_a_plex_playlist_write_locks_the_playlist(db, make_user, as_user, locks, call):
|
|
user = make_user()
|
|
pid = _plex(db, user)
|
|
r = call(as_user(user), pid)
|
|
assert r.status_code == 200, r.text
|
|
assert PLEX in locks
|
|
|
|
|
|
def test_a_plex_reorder_that_omits_a_row_does_not_collide_with_it(db, make_user, as_user):
|
|
"""The row "a" was added in another tab after this page loaded its order ["c", "b"]: it keeps
|
|
its place after the given rows instead of sharing position 0 with "c"."""
|
|
user = make_user()
|
|
pid = _plex(db, user)
|
|
r = as_user(user).put(f"/api/plex/playlists/{pid}/order", json={"item_rating_keys": ["c", "b"]})
|
|
assert r.status_code == 200
|
|
db.expire_all()
|
|
rows = db.query(PlexPlaylistItem).filter_by(playlist_id=pid).all()
|
|
by_id = {it.id: it.rating_key for it in db.query(PlexItem).all()}
|
|
got = sorted((r.position, by_id[r.item_id]) for r in rows)
|
|
assert got == [(0, "c"), (1, "b"), (2, "a")]
|
|
|
|
|
|
YT_ITEMS = PlaylistItem.__table__.name
|
|
PLEX_ITEMS = PlexPlaylistItem.__table__.name
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"call",
|
|
[
|
|
lambda c, pid, other: c.post(f"/api/playlists/{pid}/items", json={"video_id": "v1"}),
|
|
lambda c, pid, other: c.delete(f"/api/playlists/{pid}/items/v1"),
|
|
lambda c, pid, other: c.post(
|
|
f"/api/playlists/{pid}/items/move-bulk",
|
|
json={"video_ids": ["v1"], "target_playlist_id": other},
|
|
),
|
|
lambda c, pid, other: c.put(f"/api/playlists/{pid}/order", json={"video_ids": ["v1"]}),
|
|
# the ORM cascade deletes the items before the list row
|
|
lambda c, pid, other: c.delete(f"/api/playlists/{pid}"),
|
|
lambda c, pid, other: c.post(
|
|
f"/api/playlists/{pid}/items/remove-bulk", json={"video_ids": ["v1"]}
|
|
),
|
|
lambda c, pid, other: c.post(
|
|
f"/api/playlists/{pid}/items/restore", json={"video_ids": ["v2"], "positions": [0]}
|
|
),
|
|
lambda c, pid, other: c.post(
|
|
f"/api/playlists/{pid}/items/undo-move",
|
|
json={"from_playlist_id": other, "moved": ["v2"], "positions": [0]},
|
|
),
|
|
],
|
|
ids=[
|
|
"add-present",
|
|
"remove",
|
|
"move",
|
|
"reorder",
|
|
"delete",
|
|
"remove-bulk",
|
|
"restore",
|
|
"undo-move",
|
|
],
|
|
)
|
|
def test_a_youtube_item_write_locks_the_playlist_before_any_item(
|
|
db, make_user, as_user, statements, call
|
|
):
|
|
user = make_user()
|
|
pid, other = _yt(db, user)
|
|
c = as_user(user)
|
|
statements.clear()
|
|
r = call(c, pid, other)
|
|
assert r.status_code == 200, r.text
|
|
assert _locked_before_items(statements, YT, YT_ITEMS), statements
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
("path", "body"),
|
|
[
|
|
("move-bulk", lambda other: {"video_ids": ["v1"], "target_playlist_id": other}),
|
|
("undo-move", lambda other: {"from_playlist_id": other, "moved": ["v2"], "positions": [0]}),
|
|
],
|
|
ids=["move", "undo"],
|
|
)
|
|
def test_a_move_locks_both_lists_in_one_ordered_statement(
|
|
db, make_user, as_user, statements, path, body
|
|
):
|
|
"""Two moves A→B and B→A each locking "its" list first would lock crosswise, and an Undo runs the
|
|
other way round from a move by its nature."""
|
|
user = make_user()
|
|
pid, other = _yt(db, user)
|
|
statements.clear()
|
|
r = as_user(user).post(f"/api/playlists/{pid}/items/{path}", json=body(other))
|
|
assert r.status_code == 200, r.text
|
|
lock = _locked_before_items(statements, YT, YT_ITEMS)
|
|
assert lock and "ORDER BY" in lock, lock
|
|
# BOTH lists in that one statement: one bound id per list in its IN (…) (a single-id lock
|
|
# renders the same IN and ORDER BY)
|
|
listed = re.search(r" IN \((.*?)\) ORDER BY", lock)
|
|
assert listed and len(re.findall(r"%\(\w+\)s", listed.group(1))) == 2, lock
|
|
|
|
|
|
def test_watch_later_add_and_remove_lock_it_before_any_item(db, make_user, as_user, statements):
|
|
user = make_user()
|
|
_yt(db, user)
|
|
c = as_user(user)
|
|
assert c.post("/api/playlists/watch-later", json={"video_id": "v2"}).status_code == 200
|
|
statements.clear()
|
|
assert c.post("/api/playlists/watch-later", json={"video_id": "v2"}).status_code == 200
|
|
assert _locked_before_items(statements, YT, YT_ITEMS), statements
|
|
statements.clear()
|
|
assert c.delete("/api/playlists/watch-later/v2").status_code == 200
|
|
assert _locked_before_items(statements, YT, YT_ITEMS), statements
|
|
|
|
|
|
def test_a_rename_locks_the_playlist(db, make_user, as_user, locks):
|
|
"""A rename moves `dirty`, which the scheduled mirror refresh re-reads under this lock."""
|
|
user = make_user()
|
|
pid, _ = _yt(db, user)
|
|
r = as_user(user).patch(f"/api/playlists/{pid}", json={"name": "New"})
|
|
assert r.status_code == 200, r.text
|
|
assert YT in locks
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"call",
|
|
[
|
|
lambda c, pid: c.delete(f"/api/plex/playlists/{pid}/items/a"),
|
|
lambda c, pid: c.post(
|
|
f"/api/plex/playlists/{pid}/items/remove-bulk", json={"item_rating_keys": ["a", "b"]}
|
|
),
|
|
lambda c, pid: c.post(f"/api/plex/playlists/{pid}/items", json={"item_rating_key": "a"}),
|
|
],
|
|
ids=["remove", "remove-bulk", "add-present"],
|
|
)
|
|
def test_a_plex_item_write_locks_the_playlist_before_any_item(
|
|
db, make_user, as_user, statements, call
|
|
):
|
|
user = make_user()
|
|
pid = _plex(db, user)
|
|
c = as_user(user)
|
|
statements.clear()
|
|
r = call(c, pid)
|
|
assert r.status_code == 200, r.text
|
|
assert _locked_before_items(statements, PLEX, PLEX_ITEMS), statements
|