From 94a280e89ad73fe2b994cb36bb5b2aa0208bd561 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Sat, 27 Jun 2026 18:51:29 +0000 Subject: [PATCH] CodeRabbit Generated Unit Tests: Add unit tests for PR changes --- internal/jellycompat/content_direct_test.go | 249 ++++++++++++++++++ internal/jellycompat/handlers_items_test.go | 180 ++++++++++++- internal/jellycompat/handlers_persons_test.go | 135 +++++++++- 3 files changed, 562 insertions(+), 2 deletions(-) diff --git a/internal/jellycompat/content_direct_test.go b/internal/jellycompat/content_direct_test.go index 8c3f85c8..64d49ef1 100644 --- a/internal/jellycompat/content_direct_test.go +++ b/internal/jellycompat/content_direct_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/url" "slices" + "strings" "testing" "time" @@ -460,6 +461,44 @@ func (s *stubBrowseSource) ListGenres(_ context.Context, _ catalog.BrowseFilters return nil, nil } +type recordingCatalogSearchProvider struct { + requests []catalog.CatalogSearchRequest + result *catalog.CatalogSearchResult +} + +func (p *recordingCatalogSearchProvider) Search(_ context.Context, req catalog.CatalogSearchRequest) (*catalog.CatalogSearchResult, error) { + p.requests = append(p.requests, req) + if p.result != nil { + return p.result, nil + } + return &catalog.CatalogSearchResult{}, nil +} + +type recordingItemAccessSource struct { + searchQueries []string + searchTypes [][]string + searchLimits []int + searchOffsets []int + items []*models.MediaItem + total int +} + +func (s *recordingItemAccessSource) EnsureAccessible(context.Context, string, catalog.AccessFilter) error { + return nil +} + +func (s *recordingItemAccessSource) Search(_ context.Context, query string, itemTypes []string, limit, offset int, _ catalog.AccessFilter) ([]*models.MediaItem, int, error) { + s.searchQueries = append(s.searchQueries, query) + s.searchTypes = append(s.searchTypes, append([]string(nil), itemTypes...)) + s.searchLimits = append(s.searchLimits, limit) + s.searchOffsets = append(s.searchOffsets, offset) + return append([]*models.MediaItem(nil), s.items...), s.total, nil +} + +func (s *recordingItemAccessSource) GetByIDs(context.Context, []string) ([]*models.MediaItem, error) { + return nil, nil +} + // newDirectContentServiceForTest builds a directContentService with stubbed // catalog dependencies. Useful for behavioral tests that don't need real // Postgres state. @@ -470,6 +509,216 @@ func newDirectContentServiceForTest(browse browseSource, provider userstore.User } } +func TestSearchItemsUsesCatalogSearchProviderWithCompatScope(t *testing.T) { + libraryID := 7 + provider := &recordingCatalogSearchProvider{ + result: &catalog.CatalogSearchResult{ + Items: []*models.MediaItem{{ + ContentID: "movie-1", + Type: "movie", + Title: "Dune", + }}, + Total: 12, + HasMore: true, + }, + } + svc := &directContentService{ + searchProvider: provider, + accessFilter: func(context.Context, int, string) catalog.AccessFilter { + return catalog.AccessFilter{ + AllowedLibraryIDs: []int{1, 2}, + ExcludedMediaTypes: []string{"ebook"}, + MaxContentRating: "PG-13", + } + }, + } + + result, err := svc.SearchItems(context.Background(), &Session{ + StreamAppUserID: 22, + ProfileID: "profile-1", + }, SearchItemsOptions{ + Query: "dune", + Limit: 5, + Offset: 10, + LibraryID: &libraryID, + SkipTotal: true, + }) + if err != nil { + t.Fatalf("SearchItems error: %v", err) + } + if len(provider.requests) != 1 { + t.Fatalf("provider requests = %d, want 1", len(provider.requests)) + } + req := provider.requests[0] + if req.Query != "dune" || req.Limit != 5 || req.Offset != 10 || !req.SkipTotal { + t.Fatalf("provider request shape = %#v", req) + } + if want := []string{"movie", "series", "episode"}; !slices.Equal(req.ItemTypes, want) { + t.Fatalf("ItemTypes = %#v, want %#v", req.ItemTypes, want) + } + if req.Access.PresentationLibraryID == nil || *req.Access.PresentationLibraryID != libraryID { + t.Fatalf("PresentationLibraryID = %#v, want %d", req.Access.PresentationLibraryID, libraryID) + } + for _, mediaType := range []string{"ebook", "audiobook", "podcast"} { + if !slices.Contains(req.Access.ExcludedMediaTypes, mediaType) { + t.Fatalf("ExcludedMediaTypes = %#v, missing %q", req.Access.ExcludedMediaTypes, mediaType) + } + } + if result.Total != 12 || !result.HasMore || len(result.Items) != 1 || result.Items[0].Title != "Dune" { + t.Fatalf("result = %#v", result) + } +} + +func TestSearchItemsFallsBackToItemRepoWhenProviderMissing(t *testing.T) { + itemRepo := &recordingItemAccessSource{ + items: []*models.MediaItem{{ + ContentID: "movie-1", + Type: "movie", + Title: "Fallback", + }}, + total: 3, + } + svc := &directContentService{itemRepo: itemRepo} + + result, err := svc.SearchItems(context.Background(), &Session{}, SearchItemsOptions{ + Query: "fallback", + ItemTypes: []string{"MusicAlbum"}, + Limit: 2, + Offset: 1, + }) + if err != nil { + t.Fatalf("SearchItems error: %v", err) + } + if len(itemRepo.searchQueries) != 1 || itemRepo.searchQueries[0] != "fallback" { + t.Fatalf("search queries = %#v", itemRepo.searchQueries) + } + if want := []string{compatNoMatchType}; !slices.Equal(itemRepo.searchTypes[0], want) { + t.Fatalf("search types = %#v, want %#v", itemRepo.searchTypes[0], want) + } + if result.Total != 3 || !result.HasMore || len(result.Items) != 1 || result.Items[0].Title != "Fallback" { + t.Fatalf("result = %#v", result) + } +} + +func TestSearchItemsProviderReturnsNilResult(t *testing.T) { + // explicitlyNilResultProvider returns (nil, nil) — no error, no result. + // This exercises the nil-guard in directContentService.SearchItems + // (the recordingCatalogSearchProvider stub always returns a non-nil result). + provider := &explicitlyNilResultProvider{} + svc := &directContentService{searchProvider: provider} + + result, err := svc.SearchItems(context.Background(), &Session{}, SearchItemsOptions{ + Query: "ghost", + Limit: 10, + }) + if err != nil { + t.Fatalf("SearchItems error: %v", err) + } + if result == nil { + t.Fatal("expected non-nil result") + } + if len(result.Items) != 0 { + t.Fatalf("Items = %d, want 0 when provider returns nil", len(result.Items)) + } + if result.Total != 0 { + t.Fatalf("Total = %d, want 0 when provider returns nil", result.Total) + } + if result.HasMore { + t.Fatal("HasMore = true, want false when provider returns nil") + } +} + +// explicitlyNilResultProvider is a CatalogSearchProvider that returns (nil, nil) +// on every call, exercising the nil-guard in directContentService.SearchItems. +type explicitlyNilResultProvider struct { + calls int +} + +func (p *explicitlyNilResultProvider) Search(_ context.Context, _ catalog.CatalogSearchRequest) (*catalog.CatalogSearchResult, error) { + p.calls++ + return nil, nil +} + +func TestSearchItemsProviderErrorPropagated(t *testing.T) { + errProvider := &erroringCatalogSearchProvider{err: fmt.Errorf("search backend unavailable")} + svc := &directContentService{searchProvider: errProvider} + + _, err := svc.SearchItems(context.Background(), &Session{}, SearchItemsOptions{ + Query: "test", + Limit: 5, + }) + if err == nil { + t.Fatal("expected error from provider, got nil") + } + if !strings.Contains(err.Error(), "search backend unavailable") { + t.Fatalf("error %q does not contain expected message", err.Error()) + } +} + +// erroringCatalogSearchProvider always returns an error from Search. +type erroringCatalogSearchProvider struct { + err error +} + +func (p *erroringCatalogSearchProvider) Search(_ context.Context, _ catalog.CatalogSearchRequest) (*catalog.CatalogSearchResult, error) { + return nil, p.err +} + +func TestSearchItemsFallbackHasMoreFalseAtBoundary(t *testing.T) { + // When offset + len(items) == total, hasMore must be false. + // e.g. offset=2, return 1 item, total=3 → 2+1 == 3 → hasMore=false + itemRepo := &recordingItemAccessSource{ + items: []*models.MediaItem{{ + ContentID: "movie-3", + Type: "movie", + Title: "Last", + }}, + total: 3, + } + svc := &directContentService{itemRepo: itemRepo} + + result, err := svc.SearchItems(context.Background(), &Session{}, SearchItemsOptions{ + Query: "last", + Limit: 1, + Offset: 2, + }) + if err != nil { + t.Fatalf("SearchItems error: %v", err) + } + if result.HasMore { + t.Fatalf("HasMore = true, want false when offset(%d)+len(%d) == total(%d)", 2, 1, 3) + } + if result.Total != 3 { + t.Fatalf("Total = %d, want 3", result.Total) + } +} + +func TestSearchItemsFallbackHasMoreTrueWhenMoreRemain(t *testing.T) { + // When offset + len(items) < total, hasMore must be true. + // e.g. offset=0, return 1 item, total=3 → 0+1 < 3 → hasMore=true + itemRepo := &recordingItemAccessSource{ + items: []*models.MediaItem{{ + ContentID: "movie-1", + Type: "movie", + Title: "First", + }}, + total: 3, + } + svc := &directContentService{itemRepo: itemRepo} + + result, err := svc.SearchItems(context.Background(), &Session{}, SearchItemsOptions{ + Query: "first", + Limit: 1, + Offset: 0, + }) + if err != nil { + t.Fatalf("SearchItems error: %v", err) + } + if !result.HasMore { + t.Fatalf("HasMore = false, want true when offset(%d)+len(%d) < total(%d)", 0, 1, 3) + } +} + // TestBrowseItems_DoesNotFetchProgressWhenNoPlayedFilter verifies that // BrowseItems does NOT call ListProgressByMediaItems on the user store when // the is_played filter is empty. The handler-level resolveUserStateForContentIDs diff --git a/internal/jellycompat/handlers_items_test.go b/internal/jellycompat/handlers_items_test.go index 7826ab85..0ef085a3 100644 --- a/internal/jellycompat/handlers_items_test.go +++ b/internal/jellycompat/handlers_items_test.go @@ -50,7 +50,7 @@ func (s *countingContentService) BrowseItems(context.Context, *Session, url.Valu panic("unused") } -func (s *countingContentService) SearchItems(context.Context, *Session, string, []string, int, int, *int) (*upstreamBrowseResponse, error) { +func (s *countingContentService) SearchItems(context.Context, *Session, SearchItemsOptions) (*upstreamBrowseResponse, error) { panic("unused") } @@ -81,6 +81,20 @@ func (s *countingContentService) ListItemFilters(context.Context, *Session, url. panic("unused") } +type recordingSearchContentService struct { + countingContentService + options []SearchItemsOptions + result *upstreamBrowseResponse +} + +func (s *recordingSearchContentService) SearchItems(_ context.Context, _ *Session, opts SearchItemsOptions) (*upstreamBrowseResponse, error) { + s.options = append(s.options, opts) + if s.result != nil { + return s.result, nil + } + return &upstreamBrowseResponse{Items: []upstreamListItem{}}, nil +} + func TestHandleItems_SeriesParentSeasonFilterReturnsPagedSeasons(t *testing.T) { codec := NewResourceIDCodec() seriesContentID := "series-1" @@ -140,6 +154,170 @@ func TestHandleItems_SeriesParentSeasonFilterReturnsPagedSeasons(t *testing.T) { } } +func TestHandleItemsSearchPropagatesEnableTotalRecordCount(t *testing.T) { + for _, tc := range []struct { + name string + querySuffix string + wantSkipTotal bool + }{ + {name: "default includes total", wantSkipTotal: false}, + {name: "disabled skips total", querySuffix: "&EnableTotalRecordCount=false", wantSkipTotal: true}, + } { + t.Run(tc.name, func(t *testing.T) { + codec := NewResourceIDCodec() + contentSvc := &recordingSearchContentService{} + h := &ItemsHandler{ + content: contentSvc, + userData: &mockUserDataService{}, + codec: codec, + mapper: newMapper(codec, &config.Config{}), + images: NewImageCache(time.Hour, time.Now), + } + + req := httptest.NewRequest("GET", "/Users/test/Items?SearchTerm=dune&Limit=5&StartIndex=2"+tc.querySuffix, nil) + req = req.WithContext(context.WithValue(req.Context(), compatSessionKey, &Session{ + StreamAppUserID: 1, + ProfileID: "profile-1", + })) + + rec := httptest.NewRecorder() + h.HandleItems(rec, req) + + if rec.Code != 200 { + t.Fatalf("expected status 200; got %d, body=%s", rec.Code, rec.Body.String()) + } + if len(contentSvc.options) != 1 { + t.Fatalf("SearchItems calls = %d, want 1", len(contentSvc.options)) + } + opts := contentSvc.options[0] + if opts.Query != "dune" || opts.Limit != 5 || opts.Offset != 2 { + t.Fatalf("SearchItems options = %#v", opts) + } + if opts.SkipTotal != tc.wantSkipTotal { + t.Fatalf("SkipTotal = %v, want %v", opts.SkipTotal, tc.wantSkipTotal) + } + }) + } +} + +// TestHandleSearchHints_PassesOnlyQueryAndLimit verifies that HandleSearchHints +// calls SearchItems with a SearchItemsOptions that carries only Query and Limit +// (no LibraryID, empty ItemTypes, and SkipTotal=false). The search-hints +// endpoint does not scope by library and never specifies item types — returning +// all video-compatible types — so it must not accidentally suppress the total +// or restrict the result set. +func TestHandleSearchHints_PassesOnlyQueryAndLimit(t *testing.T) { + codec := NewResourceIDCodec() + contentSvc := &recordingSearchContentService{ + result: &upstreamBrowseResponse{ + Items: []upstreamListItem{ + {ContentID: "movie-1", Type: "movie", Title: "Dune"}, + }, + Total: 1, + }, + } + h := &ItemsHandler{ + content: contentSvc, + userData: &mockUserDataService{}, + codec: codec, + mapper: newMapper(codec, &config.Config{}), + images: NewImageCache(time.Hour, time.Now), + } + + req := httptest.NewRequest("GET", "/Search/Hints?SearchTerm=dune&Limit=7", nil) + req = req.WithContext(context.WithValue(req.Context(), compatSessionKey, &Session{ + StreamAppUserID: 1, + ProfileID: "profile-1", + })) + rec := httptest.NewRecorder() + h.HandleSearchHints(rec, req) + + if rec.Code != 200 { + t.Fatalf("expected status 200; got %d, body=%s", rec.Code, rec.Body.String()) + } + if len(contentSvc.options) != 1 { + t.Fatalf("SearchItems calls = %d, want 1", len(contentSvc.options)) + } + opts := contentSvc.options[0] + if opts.Query != "dune" { + t.Errorf("Query = %q, want %q", opts.Query, "dune") + } + if opts.Limit != 7 { + t.Errorf("Limit = %d, want 7", opts.Limit) + } + if len(opts.ItemTypes) != 0 { + t.Errorf("ItemTypes = %#v, want empty (HandleSearchHints must not restrict types)", opts.ItemTypes) + } + if opts.LibraryID != nil { + t.Errorf("LibraryID = %v, want nil", opts.LibraryID) + } + if opts.SkipTotal { + t.Error("SkipTotal = true, want false for search hints") + } + if opts.Offset != 0 { + t.Errorf("Offset = %d, want 0", opts.Offset) + } +} + +// TestHandleSearchHints_EmptyQueryReturnsEmptyWithoutCallingSearch verifies +// that an empty or whitespace-only SearchTerm returns a 200 empty result +// without invoking the ContentService at all. +func TestHandleSearchHints_EmptyQueryReturnsEmptyWithoutCallingSearch(t *testing.T) { + codec := NewResourceIDCodec() + contentSvc := &recordingSearchContentService{} + h := &ItemsHandler{ + content: contentSvc, + userData: &mockUserDataService{}, + codec: codec, + mapper: newMapper(codec, &config.Config{}), + images: NewImageCache(time.Hour, time.Now), + } + + req := httptest.NewRequest("GET", "/Search/Hints?SearchTerm= ", nil) + req = req.WithContext(context.WithValue(req.Context(), compatSessionKey, &Session{ + StreamAppUserID: 1, + ProfileID: "profile-1", + })) + rec := httptest.NewRecorder() + h.HandleSearchHints(rec, req) + + if rec.Code != 200 { + t.Fatalf("expected status 200; got %d, body=%s", rec.Code, rec.Body.String()) + } + if len(contentSvc.options) != 0 { + t.Errorf("SearchItems called %d times, want 0 for empty query", len(contentSvc.options)) + } +} + +// TestHandleSearchHints_DefaultLimitIsTwenty verifies that when Limit is +// omitted from the query string, HandleSearchHints uses a default of 20. +func TestHandleSearchHints_DefaultLimitIsTwenty(t *testing.T) { + codec := NewResourceIDCodec() + contentSvc := &recordingSearchContentService{} + h := &ItemsHandler{ + content: contentSvc, + userData: &mockUserDataService{}, + codec: codec, + mapper: newMapper(codec, &config.Config{}), + images: NewImageCache(time.Hour, time.Now), + } + + req := httptest.NewRequest("GET", "/Search/Hints?SearchTerm=inception", nil) + req = req.WithContext(context.WithValue(req.Context(), compatSessionKey, &Session{ + StreamAppUserID: 1, + ProfileID: "profile-1", + })) + rec := httptest.NewRecorder() + h.HandleSearchHints(rec, req) + + if rec.Code != 200 { + t.Fatalf("expected status 200; got %d, body=%s", rec.Code, rec.Body.String()) + } + if len(contentSvc.options) == 1 && contentSvc.options[0].Limit != 20 { + t.Errorf("Limit = %d, want 20 (default)", contentSvc.options[0].Limit) + } +} + // TestHandleItem_Episode_FetchesSeriesDetailForStableParentImageTags verifies // that episode detail responses fetch parent series image metadata even when // image URLs are already cached. Cached URLs are not enough to build stable diff --git a/internal/jellycompat/handlers_persons_test.go b/internal/jellycompat/handlers_persons_test.go index eb2c3bed..3e35e51f 100644 --- a/internal/jellycompat/handlers_persons_test.go +++ b/internal/jellycompat/handlers_persons_test.go @@ -1,6 +1,139 @@ package jellycompat -import "testing" +import ( + "context" + "net/url" + "slices" + "testing" +) + +// recordingPersonsContentService is a ContentService stub that records all +// SearchItems calls made by PersonsHandler. It is only used in +// handlers_persons_test.go and lives here to keep it close to the tests that +// rely on it. +type recordingPersonsContentService struct { + searchOptions []SearchItemsOptions + searchResult *upstreamBrowseResponse +} + +func (s *recordingPersonsContentService) SearchItems(_ context.Context, _ *Session, opts SearchItemsOptions) (*upstreamBrowseResponse, error) { + s.searchOptions = append(s.searchOptions, opts) + if s.searchResult != nil { + return s.searchResult, nil + } + return &upstreamBrowseResponse{Items: []upstreamListItem{}}, nil +} + +func (s *recordingPersonsContentService) ListUserLibraries(context.Context, *Session) ([]upstreamUserLibrary, error) { + panic("unused") +} +func (s *recordingPersonsContentService) BrowseItems(context.Context, *Session, url.Values) (*upstreamBrowseResponse, error) { + panic("unused") +} +func (s *recordingPersonsContentService) GetItemDetail(context.Context, *Session, string, *int) (*upstreamItemDetail, error) { + panic("unused") +} +func (s *recordingPersonsContentService) ListSeasons(context.Context, *Session, string, *int) ([]upstreamSeason, error) { + panic("unused") +} +func (s *recordingPersonsContentService) GetSeason(context.Context, *Session, string, int, *int) (*upstreamSeason, error) { + panic("unused") +} +func (s *recordingPersonsContentService) ListEpisodes(context.Context, *Session, string, int, *int) ([]upstreamEpisode, error) { + panic("unused") +} +func (s *recordingPersonsContentService) ListEpisodesBySeasonID(context.Context, *Session, string, *int) ([]upstreamEpisode, error) { + panic("unused") +} +func (s *recordingPersonsContentService) ListItemFilters(context.Context, *Session, url.Values) (*upstreamItemFiltersResponse, error) { + panic("unused") +} + +// TestShouldSuppressSearchPeople_PassesSkipTotalTrue verifies that +// shouldSuppressSearchPeople always sets SkipTotal:true in the SearchItems call +// it issues to probe whether the query matches media titles. Fetching a total +// count on the hot people-search path is unnecessary overhead. +func TestShouldSuppressSearchPeople_PassesSkipTotalTrue(t *testing.T) { + // "Avatar 2" fails looksLikePersonName (has a digit) so the probe fires. + contentSvc := &recordingPersonsContentService{ + searchResult: &upstreamBrowseResponse{ + Items: []upstreamListItem{ + {ContentID: "movie-1", Title: "Avatar 2"}, + }, + }, + } + h := &PersonsHandler{content: contentSvc} + + _ = h.shouldSuppressSearchPeople(context.Background(), &Session{StreamAppUserID: 1, ProfileID: "p1"}, "Avatar 2") + + if len(contentSvc.searchOptions) != 1 { + t.Fatalf("SearchItems call count = %d, want 1", len(contentSvc.searchOptions)) + } + opts := contentSvc.searchOptions[0] + if !opts.SkipTotal { + t.Errorf("SkipTotal = false, want true (total is unnecessary on the probe path)") + } + if want := []string{"movie", "series"}; !slices.Equal(opts.ItemTypes, want) { + t.Errorf("ItemTypes = %#v, want %#v", opts.ItemTypes, want) + } + if opts.Limit != 5 { + t.Errorf("Limit = %d, want 5", opts.Limit) + } +} + +// TestShouldSuppressSearchPeople_SkipsProbeForPersonNameShape verifies that +// when the query looks like a person name, SearchItems is never called — +// the handler short-circuits before reaching the media probe. +func TestShouldSuppressSearchPeople_SkipsProbeForPersonNameShape(t *testing.T) { + contentSvc := &recordingPersonsContentService{} + h := &PersonsHandler{content: contentSvc} + + // "Tom Hanks" looks like a person name → no probe. + suppressed := h.shouldSuppressSearchPeople(context.Background(), &Session{StreamAppUserID: 1, ProfileID: "p1"}, "Tom Hanks") + + if len(contentSvc.searchOptions) != 0 { + t.Errorf("SearchItems called %d times, want 0 for person-name queries", len(contentSvc.searchOptions)) + } + if suppressed { + t.Error("suppressed = true for a person-name query, want false") + } +} + +// TestShouldSuppressSearchPeople_ReturnsFalseWhenNoMediaMatch verifies that +// the handler does NOT suppress when the media search returns no results. +func TestShouldSuppressSearchPeople_ReturnsFalseWhenNoMediaMatch(t *testing.T) { + // Empty result → should not suppress. + contentSvc := &recordingPersonsContentService{ + searchResult: &upstreamBrowseResponse{Items: []upstreamListItem{}}, + } + h := &PersonsHandler{content: contentSvc} + + suppressed := h.shouldSuppressSearchPeople(context.Background(), &Session{}, "Avatar 2") + + if suppressed { + t.Error("suppressed = true when no media matched, want false") + } +} + +// TestShouldSuppressSearchPeople_ReturnsTrueOnExactTitleMatch verifies the +// core suppression logic: an exact title match causes the person search to +// be suppressed so clients receive an empty person list. +func TestShouldSuppressSearchPeople_ReturnsTrueOnExactTitleMatch(t *testing.T) { + contentSvc := &recordingPersonsContentService{ + searchResult: &upstreamBrowseResponse{ + Items: []upstreamListItem{ + {ContentID: "movie-1", Title: "Avatar 2"}, + }, + }, + } + h := &PersonsHandler{content: contentSvc} + + suppressed := h.shouldSuppressSearchPeople(context.Background(), &Session{}, "Avatar 2") + + if !suppressed { + t.Error("suppressed = false for exact title match, want true") + } +} func TestLooksLikePersonName_AcceptsRealNames(t *testing.T) { cases := []string{"Christopher Nolan", "Tom Hanks", "Émilie Dequenne", "O'Brien"}