fix(feed): a malformed saved view cannot break the autotag cleanup, and a foreign private tag reads as missing
E32 review round 1 (B5, B6).
- The autotag orphan cleanup reads every user's feed views. A view whose tag pick was not a list
("tags": 5) raised for everyone's job, True passed as tag id 1, and ~66k ids pushed the NOT IN
past psycopg's 65535 bind parameters. The views are now read defensively and the kept ids
filtered in Python; the saved-views route holds filters.tags to a list of at most 500 ids, on
create and on update.
- tags_by_category resolved any tag id, so another user's private tag landed in its real category
and the feed summary answered differently than for an id that does not exist, which told the
caller which foreign ids exist. It is owner-scoped now (pre-existing; S2's missing-tag branch
made the difference visible).
This commit is contained in:
@@ -113,15 +113,19 @@ def _tag_visible(user: User):
|
||||
MISSING_TAG_CATEGORY = "missing"
|
||||
|
||||
|
||||
def tags_by_category(db: Session, tag_ids: list[int]) -> dict[str, list[int]]:
|
||||
def tags_by_category(db: Session, user: User, tag_ids: list[int]) -> dict[str, list[int]]:
|
||||
"""Group tag ids by their category (language/topic/other). The single source for the category split
|
||||
shared by the feed's tag WHERE (filtered_query) and the facet/count summary (feed_summary), so the
|
||||
two can't disagree on which selected tag belongs to which category. An unknown id lands in
|
||||
MISSING_TAG_CATEGORY."""
|
||||
MISSING_TAG_CATEGORY — and so does another user's private tag: resolving it to its real category
|
||||
told the caller which foreign ids exist (E32 review)."""
|
||||
if not tag_ids:
|
||||
return {} # skip the pointless `WHERE id IN ()` round-trip on the no-tag-filter hot path
|
||||
out: dict[str, list[int]] = {}
|
||||
for tid, cat in db.execute(select(Tag.id, Tag.category).where(Tag.id.in_(tag_ids))).all():
|
||||
visible = or_(Tag.user_id.is_(None), Tag.user_id == user.id)
|
||||
for tid, cat in db.execute(
|
||||
select(Tag.id, Tag.category).where(Tag.id.in_(tag_ids), visible)
|
||||
).all():
|
||||
out.setdefault(cat, []).append(tid)
|
||||
known = {tid for ids in out.values() for tid in ids}
|
||||
if missing := [tid for tid in tag_ids if tid not in known]:
|
||||
@@ -315,7 +319,7 @@ def filtered_query(
|
||||
# ALL its selected tags. ⚠️ feed_summary.passes() reimplements this exact rule in Python for the
|
||||
# sidebar count/facets — keep the two in lockstep (equivalence-tested by
|
||||
# test_feed_summary_matches_count_plus_facets).
|
||||
by_category = tags_by_category(db, tags)
|
||||
by_category = tags_by_category(db, user, tags)
|
||||
# Facet counting drops the category being counted so its own chips don't zero each
|
||||
# other out (standard drill-down faceting: a category's count ignores its own
|
||||
# selections but still honours the other categories' filters).
|
||||
@@ -406,7 +410,7 @@ def feed_summary(db: Session, user: User, **params) -> dict:
|
||||
return {"count": 0, "facets": {}}
|
||||
|
||||
# channel -> its visible tag ids, plus each tag's category (for grouping the facet output).
|
||||
visible = or_(ChannelTag.user_id.is_(None), ChannelTag.user_id == user.id)
|
||||
visible = _tag_visible(user)
|
||||
channel_tags: dict[str, set[int]] = {}
|
||||
tag_category: dict[int, str] = {}
|
||||
for cid, tid, cat in db.execute(
|
||||
@@ -421,7 +425,7 @@ def feed_summary(db: Session, user: User, **params) -> dict:
|
||||
# base channel carries still filters everything out for its category — matching the SQL `IN (empty)`
|
||||
# behaviour).
|
||||
selected_by_cat: dict[str, set[int]] = {
|
||||
cat: set(ids) for cat, ids in tags_by_category(db, params.get("tags") or []).items()
|
||||
cat: set(ids) for cat, ids in tags_by_category(db, user, params.get("tags") or []).items()
|
||||
}
|
||||
|
||||
def passes(ch_tags: set[int], exclude_cat: str | None) -> bool:
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
from fastapi import APIRouter, Depends
|
||||
from pydantic import BaseModel
|
||||
from pydantic import BaseModel, field_validator
|
||||
from sqlalchemy import func, select, update
|
||||
from sqlalchemy.exc import IntegrityError
|
||||
from sqlalchemy.orm import Session
|
||||
@@ -53,11 +53,30 @@ def list_views(
|
||||
return [_serialize(v) for v in rows]
|
||||
|
||||
|
||||
# A feed view's tag pick is read by more than the feed: the autotag job keeps every tag a view names
|
||||
# (sync/autotag.py). Held here to the shape the feed sends — a list of tag ids, at most this many — so
|
||||
# a hand-made body (`"tags": 5`, or 60k ids) can't break that job for every user (E32 review).
|
||||
_MAX_VIEW_TAGS = 500
|
||||
|
||||
|
||||
def _check_filters(filters: dict | None) -> dict | None:
|
||||
tags = (filters or {}).get("tags")
|
||||
if tags is None:
|
||||
return filters
|
||||
if not isinstance(tags, list) or len(tags) > _MAX_VIEW_TAGS:
|
||||
raise ValueError(f"filters.tags must be a list of at most {_MAX_VIEW_TAGS} tag ids")
|
||||
if any(type(t) is not int for t in tags): # `type`, not isinstance: True is not a tag id
|
||||
raise ValueError("filters.tags must hold tag ids")
|
||||
return filters
|
||||
|
||||
|
||||
class CreateViewIn(BaseModel):
|
||||
name: str
|
||||
filters: dict
|
||||
source: str = "feed"
|
||||
|
||||
_filters = field_validator("filters")(_check_filters)
|
||||
|
||||
|
||||
@router.post("")
|
||||
def create_view(
|
||||
@@ -98,6 +117,8 @@ class UpdateViewIn(BaseModel):
|
||||
filters: dict | None = None
|
||||
is_default: bool | None = None
|
||||
|
||||
_filters = field_validator("filters")(_check_filters)
|
||||
|
||||
|
||||
@router.patch("/{view_id}")
|
||||
def update_view(
|
||||
|
||||
+21
-14
@@ -288,28 +288,35 @@ def run_autotag_all(db: Session, only_missing: bool = False) -> dict:
|
||||
return {"channels_tagged": tagged, "orphan_tags_removed": removed}
|
||||
|
||||
|
||||
def _view_tag_ids(filters: object) -> list[int]:
|
||||
"""The tag ids a stored view's filters name; anything else in that slot names none."""
|
||||
tags = filters.get("tags") if isinstance(filters, dict) else None
|
||||
if not isinstance(tags, list):
|
||||
return []
|
||||
return [t for t in tags if type(t) is int] # `type`, not isinstance: True is not a tag id
|
||||
|
||||
|
||||
def _cleanup_orphan_system_tags(db: Session) -> int:
|
||||
"""Delete system tags that no longer link to any channel (e.g. after a mapping change) — but not
|
||||
one a saved feed view filters on. Views hold tag IDS, and a deleted tag comes back under a new id
|
||||
when a channel earns it again, so the view's filter would point at nothing; the feed drops an
|
||||
unknown id, and the view showed the whole feed under its old name (E32 S2)."""
|
||||
unknown id, and the view showed the whole feed under its old name (E32 S2).
|
||||
|
||||
The views are read defensively, and the kept ids are filtered here rather than in SQL: a view
|
||||
stored before its route checked the shape (or written around it) must not raise for everyone's
|
||||
job, nor push a `NOT IN` past the driver's bind-parameter limit (E32 review)."""
|
||||
in_views = {
|
||||
t
|
||||
for (filters,) in db.execute(select(SavedView.filters).where(SavedView.source == "feed"))
|
||||
for t in ((filters or {}).get("tags") or [])
|
||||
if isinstance(t, int)
|
||||
for t in _view_tag_ids(filters)
|
||||
}
|
||||
orphans = (
|
||||
db.execute(
|
||||
select(Tag).where(
|
||||
Tag.user_id.is_(None),
|
||||
~exists().where(ChannelTag.tag_id == Tag.id),
|
||||
Tag.id.notin_(in_views),
|
||||
)
|
||||
)
|
||||
.scalars()
|
||||
.all()
|
||||
)
|
||||
orphans = [
|
||||
tag
|
||||
for tag in db.execute(
|
||||
select(Tag).where(Tag.user_id.is_(None), ~exists().where(ChannelTag.tag_id == Tag.id))
|
||||
).scalars()
|
||||
if tag.id not in in_views
|
||||
]
|
||||
for tag in orphans:
|
||||
db.delete(tag)
|
||||
db.commit()
|
||||
|
||||
@@ -34,3 +34,31 @@ def test_an_orphan_system_tag_goes_but_one_a_saved_view_filters_on_stays(db, mak
|
||||
db.expire_all()
|
||||
names = set(db.execute(select(Tag.name)).scalars())
|
||||
assert names == {"Gaming", "Music", "Mine"} # only the unreferenced system orphan went
|
||||
|
||||
|
||||
def test_a_malformed_saved_view_cannot_break_the_cleanup(db, make_user):
|
||||
"""A view stored around the route's shape check (or before it) must not raise for everyone's job
|
||||
— `"tags": 5` raised TypeError — and `True` is not tag id 1 (E32 review)."""
|
||||
user = make_user()
|
||||
orphan = Tag(user_id=None, name="Cooking", category="topic")
|
||||
db.add(orphan)
|
||||
db.flush()
|
||||
for i, filters in enumerate(
|
||||
({"tags": 5}, {"tags": [True]}, ["not", "a", "dict"], {"tags": None})
|
||||
):
|
||||
db.add(SavedView(user_id=user.id, source="feed", name=f"bad{i}", filters=filters))
|
||||
db.commit()
|
||||
assert autotag._cleanup_orphan_system_tags(db) == 1
|
||||
db.expire_all()
|
||||
assert db.execute(select(Tag).where(Tag.name == "Cooking")).first() is None
|
||||
|
||||
|
||||
def test_a_view_kept_tag_list_larger_than_a_bind_parameter_limit_still_cleans_up(db, make_user):
|
||||
user = make_user()
|
||||
orphan = Tag(user_id=None, name="Cooking", category="topic")
|
||||
db.add(orphan)
|
||||
db.flush()
|
||||
huge = list(range(10_000_000, 10_070_000)) # past psycopg's 65535 bind parameters
|
||||
db.add(SavedView(user_id=user.id, source="feed", name="huge", filters={"tags": huge}))
|
||||
db.commit()
|
||||
assert autotag._cleanup_orphan_system_tags(db) == 1
|
||||
|
||||
@@ -12,6 +12,8 @@ from datetime import datetime, timezone
|
||||
|
||||
import pytest
|
||||
|
||||
from app import feedquery
|
||||
|
||||
from app.models import Channel, ChannelTag, Subscription, Tag, Video, VideoState
|
||||
|
||||
|
||||
@@ -192,6 +194,26 @@ def test_a_deleted_tags_id_filters_to_nothing_not_to_everything(db, as_user, mak
|
||||
assert r.json()["items"] == [], qs
|
||||
|
||||
|
||||
def test_another_users_private_tag_answers_like_one_that_does_not_exist(db, as_user, make_user):
|
||||
"""The category split resolved any tag id, so a foreign PRIVATE tag landed in its real category
|
||||
and the summary's facets differed from a non-existent id's: an oracle for which foreign ids exist
|
||||
(E32 review). Both must now read as "missing"."""
|
||||
user, tag_ids = _seed_tagged(db, make_user)
|
||||
stranger = make_user(email="stranger@example.com")
|
||||
private = Tag(user_id=stranger.id, name="Secret", category="language")
|
||||
db.add(private)
|
||||
db.commit()
|
||||
assert feedquery.tags_by_category(db, user, [tag_ids["L1"], private.id]) == {
|
||||
"language": [tag_ids["L1"]],
|
||||
feedquery.MISSING_TAG_CATEGORY: [private.id],
|
||||
}
|
||||
client = as_user(user)
|
||||
foreign = client.get(f"/api/feed/summary?show=all&tags={tag_ids['T1']}&tags={private.id}")
|
||||
absent = client.get(f"/api/feed/summary?show=all&tags={tag_ids['T1']}&tags=987654")
|
||||
assert foreign.status_code == absent.status_code == 200
|
||||
assert foreign.json() == absent.json()
|
||||
|
||||
|
||||
def test_untagged_filter_keeps_only_channels_the_user_has_not_tagged(db, as_user, make_user):
|
||||
"""The "No tag" filter (item B): `untagged=true` keeps only channels with NO tag the USER personally
|
||||
attached (no user-owned ChannelTag). System/auto tags (user_id NULL) don't count, and another user's
|
||||
|
||||
@@ -67,3 +67,18 @@ def test_duplicate_name_within_a_source_conflicts(db, monkeypatch, override_dep)
|
||||
b = c.post("/api/saved-views", json={"name": "Dup", "filters": {}, "source": "plex"})
|
||||
assert a.status_code == 200
|
||||
assert b.status_code == 409 # same (user, source, name)
|
||||
|
||||
|
||||
def test_a_feed_views_tag_pick_must_be_a_short_list_of_ids(db, monkeypatch, override_dep):
|
||||
"""The autotag job reads every view's tag pick (E32 review): the route holds it to the shape the
|
||||
feed sends, on create and on update."""
|
||||
user = _user(db)
|
||||
c = _client(db, user, monkeypatch, override_dep)
|
||||
for bad in (5, "1", [1, "2"], [True], list(range(501))):
|
||||
r = c.post("/api/saved-views", json={"name": "Bad", "filters": {"tags": bad}})
|
||||
assert r.status_code == 422, (bad, r.text)
|
||||
ok = c.post("/api/saved-views", json={"name": "Ok", "filters": {"tags": [1, 2]}})
|
||||
assert ok.status_code == 200, ok.text
|
||||
vid = ok.json()["id"]
|
||||
assert c.patch(f"/api/saved-views/{vid}", json={"filters": {"tags": 5}}).status_code == 422
|
||||
assert c.patch(f"/api/saved-views/{vid}", json={"filters": {"tags": []}}).status_code == 200
|
||||
|
||||
Reference in New Issue
Block a user