feat(discovery): persist video duration_s instead of discarding it (ADR-028)
ADR-023's filterLowValue already fetches each candidate's duration via the cheap videos.list quota call to drop Shorts/live, then threw it away — the videos.duration_s column (migration 001) was never written. Carry it onto the kept domain.Video and have UpsertVideo persist it, COALESCE-preserving a known value so an unknown (0) re-upsert never clobbers it (the channel_title backfill stance, migration 014). This is the enabling change for length-aware burst selection. No new migration — the column already exists.
This commit is contained in:
@@ -46,15 +46,16 @@ func (s *Store) UpsertVideo(ctx context.Context, v domain.Video) (string, error)
|
|||||||
}
|
}
|
||||||
|
|
||||||
if err := tx.QueryRow(ctx,
|
if err := tx.QueryRow(ctx,
|
||||||
`INSERT INTO videos (user_id, provider, provider_video_id, title, url, published_at, channel_title)
|
`INSERT INTO videos (user_id, provider, provider_video_id, title, url, published_at, channel_title, duration_s)
|
||||||
VALUES ($1, $2, $3, $4, $5, $6, $7)
|
VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
|
||||||
ON CONFLICT (user_id, provider, provider_video_id) DO UPDATE SET
|
ON CONFLICT (user_id, provider, provider_video_id) DO UPDATE SET
|
||||||
title = EXCLUDED.title,
|
title = EXCLUDED.title,
|
||||||
url = EXCLUDED.url,
|
url = EXCLUDED.url,
|
||||||
published_at = EXCLUDED.published_at,
|
published_at = EXCLUDED.published_at,
|
||||||
channel_title = COALESCE(NULLIF(EXCLUDED.channel_title, ''), videos.channel_title)
|
channel_title = COALESCE(NULLIF(EXCLUDED.channel_title, ''), videos.channel_title),
|
||||||
|
duration_s = COALESCE(EXCLUDED.duration_s, videos.duration_s)
|
||||||
RETURNING id`,
|
RETURNING id`,
|
||||||
v.UserID, provider, v.ProviderVideoID, v.Title, v.URL, nullTime(v.PublishedAt), v.ChannelTitle,
|
v.UserID, provider, v.ProviderVideoID, v.Title, v.URL, nullTime(v.PublishedAt), v.ChannelTitle, nullDuration(v.DurationSeconds),
|
||||||
).Scan(&id); err != nil {
|
).Scan(&id); err != nil {
|
||||||
return fmt.Errorf("store: upsert video: %w", err)
|
return fmt.Errorf("store: upsert video: %w", err)
|
||||||
}
|
}
|
||||||
@@ -74,6 +75,17 @@ func nullTime(t time.Time) *time.Time {
|
|||||||
return &t
|
return &t
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// nullDuration maps an unknown duration (0) to SQL NULL so the upsert's
|
||||||
|
// COALESCE(EXCLUDED.duration_s, videos.duration_s) preserves a previously-known
|
||||||
|
// value instead of clobbering it with 0 (ADR-028; the channel_title backfill
|
||||||
|
// stance, migration 014).
|
||||||
|
func nullDuration(seconds int) *int {
|
||||||
|
if seconds <= 0 {
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
return &seconds
|
||||||
|
}
|
||||||
|
|
||||||
// NewestUnsummarizedVideoIDs returns up to limit of the user's videos that have
|
// NewestUnsummarizedVideoIDs returns up to limit of the user's videos that have
|
||||||
// no summary yet, newest first (published_at DESC, NULLS LAST). It caps the
|
// no summary yet, newest first (published_at DESC, NULLS LAST). It caps the
|
||||||
// connect-time onboarding burst (Feature 1) at a fixed count: the caller marks
|
// connect-time onboarding burst (Feature 1) at a fixed count: the caller marks
|
||||||
|
|||||||
@@ -52,6 +52,36 @@ func TestUpsertVideo_ReturnsStableID(t *testing.T) {
|
|||||||
require.Equal(t, 1, count, "must not duplicate the row")
|
require.Equal(t, 1, count, "must not duplicate the row")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestUpsertVideo_PersistsAndPreservesDuration(t *testing.T) {
|
||||||
|
ctx := context.Background()
|
||||||
|
s := newStore(t)
|
||||||
|
resetDB(t, rawPool(t))
|
||||||
|
|
||||||
|
// First upsert carries a known duration (ADR-028: discovery enriches it).
|
||||||
|
v := ytVideo(userA, "dur0000001x", "with duration")
|
||||||
|
v.DurationSeconds = 750
|
||||||
|
id, err := s.UpsertVideo(ctx, v)
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
p := rawPool(t)
|
||||||
|
readDuration := func() *int {
|
||||||
|
var d *int
|
||||||
|
require.NoError(t, p.QueryRow(ctx, `SELECT duration_s FROM videos WHERE id = $1`, id).Scan(&d))
|
||||||
|
return d
|
||||||
|
}
|
||||||
|
require.NotNil(t, readDuration())
|
||||||
|
require.Equal(t, 750, *readDuration(), "duration must persist")
|
||||||
|
|
||||||
|
// A later upsert that does NOT know the duration (0) must not clobber it —
|
||||||
|
// the channel_title backfill stance (migration 014): COALESCE-preserve.
|
||||||
|
v2 := ytVideo(userA, "dur0000001x", "title updated, duration unknown")
|
||||||
|
v2.DurationSeconds = 0
|
||||||
|
_, err = s.UpsertVideo(ctx, v2)
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.NotNil(t, readDuration(), "a 0/unknown re-upsert must not erase a known duration")
|
||||||
|
require.Equal(t, 750, *readDuration())
|
||||||
|
}
|
||||||
|
|
||||||
func TestUpsertVideo_IDMatchesSummaryDedup(t *testing.T) {
|
func TestUpsertVideo_IDMatchesSummaryDedup(t *testing.T) {
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
s := newStore(t)
|
s := newStore(t)
|
||||||
|
|||||||
@@ -302,6 +302,10 @@ func (a *Adapter) filterLowValue(ctx context.Context, client *http.Client, video
|
|||||||
if m.seconds > 0 && m.seconds < a.cfg.MinVideoSeconds {
|
if m.seconds > 0 && m.seconds < a.cfg.MinVideoSeconds {
|
||||||
continue // Short / sub-threshold clip
|
continue // Short / sub-threshold clip
|
||||||
}
|
}
|
||||||
|
// Carry the duration we already fetched onto the kept video so the store
|
||||||
|
// can persist it (ADR-028) — the burst's length-aware selection depends on
|
||||||
|
// it. Discarding it here was the gap the onboarding investigation found.
|
||||||
|
v.DurationSeconds = m.seconds
|
||||||
kept = append(kept, v)
|
kept = append(kept, v)
|
||||||
}
|
}
|
||||||
return kept
|
return kept
|
||||||
|
|||||||
@@ -224,6 +224,11 @@ func TestNewVideosFiltersShortsAndLive(t *testing.T) {
|
|||||||
if len(vids) != 1 || vids[0].ProviderVideoID != "long1" {
|
if len(vids) != 1 || vids[0].ProviderVideoID != "long1" {
|
||||||
t.Fatalf("expected only long1 to survive the filter, got %+v", vids)
|
t.Fatalf("expected only long1 to survive the filter, got %+v", vids)
|
||||||
}
|
}
|
||||||
|
// The duration fetched for the filter is carried onto the kept video so the
|
||||||
|
// store can persist it (ADR-028) instead of discarding it.
|
||||||
|
if vids[0].DurationSeconds != 750 {
|
||||||
|
t.Fatalf("kept video DurationSeconds = %d, want 750 (PT12M30S)", vids[0].DurationSeconds)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestNewVideosNoFilterWhenDisabled: MinVideoSeconds=0 keeps the pre-ADR-023
|
// TestNewVideosNoFilterWhenDisabled: MinVideoSeconds=0 keeps the pre-ADR-023
|
||||||
|
|||||||
@@ -77,6 +77,11 @@ type Video struct {
|
|||||||
URL string
|
URL string
|
||||||
PublishedAt time.Time
|
PublishedAt time.Time
|
||||||
SeenAt time.Time
|
SeenAt time.Time
|
||||||
|
// DurationSeconds is the video length in seconds, when known (fetched by the
|
||||||
|
// ADR-023 videos.list enrichment at discovery). 0 means unknown — the store
|
||||||
|
// preserves a previously-known value rather than overwriting it with 0, and
|
||||||
|
// the onboarding burst (ADR-028) treats unknown as degrade-open (kept).
|
||||||
|
DurationSeconds int
|
||||||
}
|
}
|
||||||
|
|
||||||
// Transcript is the text of a video (or a record that none was available).
|
// Transcript is the text of a video (or a record that none was available).
|
||||||
|
|||||||
Reference in New Issue
Block a user