From 80ea1918e367591133e7022a389c45a4aff784ea Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 16 Aug 2026 19:40:17 +0200 Subject: [PATCH] fix(downloads): derive title clamp from the column + real-Postgres regression Address review findings on the title-clamp: TITLE_MAX_LEN now reads the length straight from Notification.title so the clamp can't drift from the column a migration would change (single source of truth). Add a db-lane test that actually commits a >255-char title into Postgres (end-to-end proof the truncation is gone, not just the clamp logic) and one that asserts the constant tracks the column width. --- backend/app/notifications.py | 13 +++++++------ backend/tests/test_notifications.py | 22 +++++++++++++++++++++- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/backend/app/notifications.py b/backend/app/notifications.py index 10f0ad6b..27abded7 100644 --- a/backend/app/notifications.py +++ b/backend/app/notifications.py @@ -14,12 +14,13 @@ from app.models import Notification # rows are never pruned (the user hasn't seen them yet). READ_SOFT_CAP = 200 -# Hard limit of the Notification.title column (String(255)). Titles are only an English -# fallback — the client renders the translated message from `data` — so clamping an -# over-long one (e.g. a FB/IG reel whose "title" is the whole caption, hundreds of chars) -# is harmless to the UI and prevents a StringDataRightTruncation that would abort the -# caller's whole transaction (the batched download_gc run in particular). -TITLE_MAX_LEN = 255 +# Hard limit of the Notification.title column, read straight from the model so the clamp +# can never drift from the actual column width (a later migration is the single source of +# truth). Titles are only an English fallback — the client renders the translated message +# from `data` — so clamping an over-long one (e.g. a FB/IG reel whose "title" is the whole +# caption, hundreds of chars) is harmless to the UI and prevents a StringDataRightTruncation +# that would abort the caller's whole transaction (the batched download_gc run in particular). +TITLE_MAX_LEN = Notification.__table__.c.title.type.length def create_notification( diff --git a/backend/tests/test_notifications.py b/backend/tests/test_notifications.py index 6780eb2d..25018d32 100644 --- a/backend/tests/test_notifications.py +++ b/backend/tests/test_notifications.py @@ -5,7 +5,7 @@ clamp that string went straight into Notification.title (String(255)) and, at th download_gc commit, raised psycopg StringDataRightTruncation — aborting the entire GC run so no expiry deletion / LRU eviction / orphan reap ever ran. The clamp trims the title to the column limit centrally, defending all callers regardless of DB backend.""" -from app.models import Notification +from app.models import Notification, User from app.notifications import TITLE_MAX_LEN, create_notification @@ -53,3 +53,23 @@ def test_title_at_exactly_the_limit_is_untouched(): exact = "y" * TITLE_MAX_LEN notif = create_notification(_CaptureDB(), user_id=1, type="t", title=exact) assert notif.title == exact + + +def test_limit_tracks_the_actual_column_width(): + # The clamp is derived from the model, so it can't silently drift from the column a + # migration would change (which is exactly what re-introduces the truncation bug). + assert TITLE_MAX_LEN == Notification.__table__.c.title.type.length + + +def test_over_long_title_actually_commits_on_postgres(db): + # The stub tests above prove the clamp *logic*; this proves the real fix end-to-end — + # a caption-length title now COMMITS into varchar(TITLE_MAX_LEN) instead of raising + # StringDataRightTruncation. Uses the opt-in Postgres lane (skipped with no DB). + user = User(email="reel@example.com") + db.add(user) + db.flush() + notif = create_notification( + db, user_id=user.id, type="download_expiring", title="z" * 2000 + ) + db.refresh(notif) + assert len(notif.title) == TITLE_MAX_LEN # persisted, not truncated by the DB