diff --git a/backend/app/feedquery.py b/backend/app/feedquery.py index 6e6d8714..aa84af0e 100644 --- a/backend/app/feedquery.py +++ b/backend/app/feedquery.py @@ -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: diff --git a/backend/app/routes/saved_views.py b/backend/app/routes/saved_views.py index c400cd0a..c4691d73 100644 --- a/backend/app/routes/saved_views.py +++ b/backend/app/routes/saved_views.py @@ -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( diff --git a/backend/app/sync/autotag.py b/backend/app/sync/autotag.py index 5ce0ba07..7f3aa788 100644 --- a/backend/app/sync/autotag.py +++ b/backend/app/sync/autotag.py @@ -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() diff --git a/backend/tests/test_autotag_orphans.py b/backend/tests/test_autotag_orphans.py index ef2b2759..a9eb5605 100644 --- a/backend/tests/test_autotag_orphans.py +++ b/backend/tests/test_autotag_orphans.py @@ -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 diff --git a/backend/tests/test_feed_views.py b/backend/tests/test_feed_views.py index 9b647160..ec8d8c78 100644 --- a/backend/tests/test_feed_views.py +++ b/backend/tests/test_feed_views.py @@ -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 diff --git a/backend/tests/test_saved_views_source.py b/backend/tests/test_saved_views_source.py index f7e078e3..a45938b5 100644 --- a/backend/tests/test_saved_views_source.py +++ b/backend/tests/test_saved_views_source.py @@ -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