From efc73a37790be5c5522021b0bccc1ef66eccf180 Mon Sep 17 00:00:00 2001 From: "Payalrvs0310@gmail.com" Date: Mon, 10 Aug 2026 12:53:09 +0530 Subject: [PATCH 1/2] fix: batch list_people's per-person queries into one Signed-off-by: Payalrvs0310@gmail.com --- backend/src/find_api/routers/people.py | 45 +++++++++++------------ backend/tests/test_people.py | 51 ++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 23 deletions(-) diff --git a/backend/src/find_api/routers/people.py b/backend/src/find_api/routers/people.py index b9b3c39a..93acd4aa 100644 --- a/backend/src/find_api/routers/people.py +++ b/backend/src/find_api/routers/people.py @@ -4,9 +4,8 @@ from fastapi import APIRouter, Depends, HTTPException from sqlalchemy.orm import Session -from sqlalchemy import func from pydantic import BaseModel, ConfigDict -from typing import List, Optional +from typing import Dict, List, Optional from find_api.core.database import get_db from find_api.core.config import settings @@ -79,29 +78,29 @@ def list_people( persons = db.query(Person).order_by(Person.created_at.desc()).all() + # One query for every (person, media) face pair instead of a count query + # and a sample query per person. face_count and sample_media_ids are both + # derived from it below. + faces_query = ( + db.query(Face.person_id, Face.media_id) + .join(Media, Media.id == Face.media_id) + .filter(Media.is_hidden.is_(False)) + ) + if scope_user_id is not None: + faces_query = faces_query.filter(Media.uploader_user_id == scope_user_id) + + face_counts: Dict[int, int] = {} + sample_media_ids_by_person: Dict[int, List[int]] = {} + for person_id, media_id in faces_query.all(): + face_counts[person_id] = face_counts.get(person_id, 0) + 1 + samples = sample_media_ids_by_person.setdefault(person_id, []) + if media_id not in samples and len(samples) < 4: + samples.append(media_id) + result = [] for person in persons: - # Count how many faces belong to this person - count_query = ( - db.query(func.count(Face.id)) - .join(Media, Media.id == Face.media_id) - .filter(Face.person_id == person.id, Media.is_hidden.is_(False)) - ) - if scope_user_id is not None: - count_query = count_query.filter(Media.uploader_user_id == scope_user_id) - face_count = count_query.scalar() - - # Get up to 4 sample media IDs for thumbnail preview - sample_query = ( - db.query(Face.media_id) - .join(Media, Media.id == Face.media_id) - .filter(Face.person_id == person.id) - .filter(Media.is_hidden.is_(False)) - ) - if scope_user_id is not None: - sample_query = sample_query.filter(Media.uploader_user_id == scope_user_id) - sample_faces = sample_query.distinct().limit(4).all() - sample_media_ids = [f.media_id for f in sample_faces] + face_count = face_counts.get(person.id, 0) + sample_media_ids = sample_media_ids_by_person.get(person.id, []) # Skip groups with no visible faces. face_count/sample_media_ids are # already scoped to the caller in shared mode, so this also hides # person groups the user has none of their own media in. diff --git a/backend/tests/test_people.py b/backend/tests/test_people.py index a79ad8ca..eddc68b7 100644 --- a/backend/tests/test_people.py +++ b/backend/tests/test_people.py @@ -108,3 +108,54 @@ def test_people_images_omit_hidden_media_faces(client, db): media_ids = [item["media_id"] for item in body["images"]] assert hidden_media.id not in media_ids + + +def test_people_list_counts_and_samples_are_not_mixed_between_people(client, db): + person_a, media_1 = _seed_person_group(db, name="A") + person_b, media_3 = _seed_person_group(db, name="B") + + # A second media for person A, plus a second face of A's in media_1 — + # face_count should total all faces (3), sample_media_ids only the + # 2 distinct media. + media_2 = Media( + file_hash=hashlib.sha256("a-second".encode()).hexdigest(), + minio_key="images/test/a-second.jpg", + filename="a-second.jpg", + content_type="image/jpeg", + file_size=1024, + status="indexed", + width=800, + height=600, + is_hidden=False, + vault_state="visible", + created_at=datetime.now(timezone.utc), + ) + db.add(media_2) + db.commit() + db.refresh(media_2) + + db.add_all( + [ + Face( + media_id=media_1.id, + person_id=person_a.id, + bounding_box={"x1": 20, "y1": 20, "x2": 30, "y2": 30}, + confidence=0.9, + ), + Face( + media_id=media_2.id, + person_id=person_a.id, + bounding_box={"x1": 0, "y1": 0, "x2": 10, "y2": 10}, + confidence=0.9, + ), + ] + ) + db.commit() + + body = client.get("/api/people").json() + by_id = {item["id"]: item for item in body} + + assert by_id[person_a.id]["face_count"] == 3 + assert set(by_id[person_a.id]["sample_media_ids"]) == {media_1.id, media_2.id} + assert by_id[person_b.id]["face_count"] == 1 + assert by_id[person_b.id]["sample_media_ids"] == [media_3.id] From 981d0a9eadb3d587eba1dcd96aaf6d2c9aa7fe02 Mon Sep 17 00:00:00 2001 From: Abhash Chakraborty <80592559+Abhash-Chakraborty@users.noreply.github.com> Date: Tue, 11 Aug 2026 23:02:43 +0530 Subject: [PATCH 2/2] perf: aggregate the people list in SQL rather than in Python MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The batching removed the 1 + 2N round trips, but replaced them with a single unbounded read: every visible (person_id, media_id) face pair in the library was fetched and folded in Python. That is constant in *queries* while being linear in *rows transferred*, so a large library trades one scaling cliff for another on the same endpoint the issue set out to make fast. Both aggregates now run in the database: - face_count is a GROUP BY over the scoped face set. - sample_media_ids uses ROW_NUMBER() OVER (PARTITION BY person_id), so the database returns at most SAMPLE_MEDIA_LIMIT rows per person instead of all of them. Ordering by media_id also makes the chosen thumbnail stable — the original `.distinct().limit(4)` had no ORDER BY and could return a different sample on each request. The shared-mode scope now lives in one helper used by both, so the count can never include rows the samples exclude. Adds a test asserting the query count is *constant*, not merely smaller: it seeds 3 people, then 23, and requires the same number of SELECTs. Against the original code it reports 7 -> 47. Response-shape assertions alone would not have caught the N+1, since the data was already correct — only slow. --- backend/src/find_api/routers/people.py | 75 ++++++++++++++++++++------ backend/tests/test_people.py | 40 ++++++++++++++ 2 files changed, 100 insertions(+), 15 deletions(-) diff --git a/backend/src/find_api/routers/people.py b/backend/src/find_api/routers/people.py index 93acd4aa..9b7ce7de 100644 --- a/backend/src/find_api/routers/people.py +++ b/backend/src/find_api/routers/people.py @@ -3,6 +3,7 @@ """ from fastapi import APIRouter, Depends, HTTPException +from sqlalchemy import func from sqlalchemy.orm import Session from pydantic import BaseModel, ConfigDict from typing import Dict, List, Optional @@ -21,6 +22,9 @@ logger = logging.getLogger(__name__) router = APIRouter() +# Thumbnails shown per person group on the People page. +SAMPLE_MEDIA_LIMIT = 4 + # ─── Pydantic schemas (what the API returns) ────────────────────────────────── @@ -78,24 +82,65 @@ def list_people( persons = db.query(Person).order_by(Person.created_at.desc()).all() - # One query for every (person, media) face pair instead of a count query - # and a sample query per person. face_count and sample_media_ids are both - # derived from it below. - faces_query = ( - db.query(Face.person_id, Face.media_id) - .join(Media, Media.id == Face.media_id) - .filter(Media.is_hidden.is_(False)) + def visible_faces(): + """Faces the caller may see, with the shared-mode scope applied. + + Both aggregates below must filter identically — a count that counts + rows the samples exclude would report a person the page cannot show. + """ + query = db.query( + Face.person_id.label("person_id"), + Face.media_id.label("media_id"), + Face.id.label("face_id"), + ).join(Media, Media.id == Face.media_id) + query = query.filter(Media.is_hidden.is_(False)) + if scope_user_id is not None: + query = query.filter(Media.uploader_user_id == scope_user_id) + return query + + # Two aggregate queries, not one row per face. Deriving these in Python + # would mean reading every face in the library into memory on every request + # — constant queries, but unbounded transfer, which is the same scaling + # cliff in a different place. + counted = visible_faces().subquery() + face_counts: Dict[int, int] = dict( + db.query(counted.c.person_id, func.count(counted.c.face_id)) + .group_by(counted.c.person_id) + .all() ) - if scope_user_id is not None: - faces_query = faces_query.filter(Media.uploader_user_id == scope_user_id) - face_counts: Dict[int, int] = {} + # Distinct media per person, then the first few of each, ranked in SQL so + # the database returns at most SAMPLE_MEDIA_LIMIT rows per person instead + # of every match. Ordering by media_id keeps the chosen thumbnail stable + # across requests; the previous `.distinct().limit(4)` had no ORDER BY and + # so could return a different sample each time. + distinct_pairs = ( + visible_faces() + .with_entities( + Face.person_id.label("person_id"), Face.media_id.label("media_id") + ) + .distinct() + .subquery() + ) + ranked = db.query( + distinct_pairs.c.person_id, + distinct_pairs.c.media_id, + func.row_number() + .over( + partition_by=distinct_pairs.c.person_id, + order_by=distinct_pairs.c.media_id, + ) + .label("rank"), + ).subquery() + sample_media_ids_by_person: Dict[int, List[int]] = {} - for person_id, media_id in faces_query.all(): - face_counts[person_id] = face_counts.get(person_id, 0) + 1 - samples = sample_media_ids_by_person.setdefault(person_id, []) - if media_id not in samples and len(samples) < 4: - samples.append(media_id) + for person_id, media_id in ( + db.query(ranked.c.person_id, ranked.c.media_id) + .filter(ranked.c.rank <= SAMPLE_MEDIA_LIMIT) + .order_by(ranked.c.person_id, ranked.c.media_id) + .all() + ): + sample_media_ids_by_person.setdefault(person_id, []).append(media_id) result = [] for person in persons: diff --git a/backend/tests/test_people.py b/backend/tests/test_people.py index eddc68b7..2501d95e 100644 --- a/backend/tests/test_people.py +++ b/backend/tests/test_people.py @@ -159,3 +159,43 @@ def test_people_list_counts_and_samples_are_not_mixed_between_people(client, db) assert set(by_id[person_a.id]["sample_media_ids"]) == {media_1.id, media_2.id} assert by_id[person_b.id]["face_count"] == 1 assert by_id[person_b.id]["sample_media_ids"] == [media_3.id] + + +def test_people_list_query_count_does_not_grow_with_people(client, db): + """The point of the batching: cost must not scale with N. + + Asserting the response shape alone would not have caught the original + 1 + 2N pattern, since it returned correct data — it was only slow. This + fails if anyone reintroduces a per-person query. + """ + from sqlalchemy import event + + engine = db.get_bind() + + def count_selects_for(batch: str, person_count: int) -> int: + # Names are hashed into file_hash, which is unique, so each batch needs + # its own prefix rather than restarting the counter. + for index in range(person_count): + _seed_person_group(db, name=f"{batch}{index}") + + statements: list[str] = [] + + def record(conn, cursor, statement, params, context, executemany): + if statement.lstrip().upper().startswith("SELECT"): + statements.append(statement) + + event.listen(engine, "before_cursor_execute", record) + try: + assert client.get("/api/people").status_code == 200 + finally: + event.remove(engine, "before_cursor_execute", record) + return len(statements) + + few = count_selects_for("Few", 3) + many = count_selects_for("Many", 20) + + # Constant, not merely "fewer": 23 people must cost the same as 3. + assert few == many, ( + f"query count grew with the number of people: {few} -> {many}; " + "a per-person query has been reintroduced" + )