From 42b6fae29716f70a2a74ec4af2f2993651d76473 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sat, 7 Feb 2026 17:59:06 -0500 Subject: [PATCH] chore(router): remove unused CollectionHandler from config Remove the CollectionHandler field from router.Config struct and its initialization in main.go. This field was never used - collections are registered directly in handlers.SetupRoutes() where a CollectionHandler is created locally. Changes: - Remove CollectionHandler field from internal/router/router.go Config - Remove CollectionHandler: nil line from cmd/server/main.go This cleans up dead code from the router refactoring. Collections continue to work correctly as they are registered in SetupRoutes(). Related: Router refactoring completion --- cmd/server/main.go | 1 - internal/app/app_test.go | 373 ++++++++++++++++++ internal/router/router.go | 1 - .../ebook_scanner_library_type_test.go | 253 ++++++++++++ 4 files changed, 626 insertions(+), 2 deletions(-) create mode 100644 internal/app/app_test.go create mode 100644 internal/services/ebook_scanner_library_type_test.go diff --git a/cmd/server/main.go b/cmd/server/main.go index 849c89f..b2ebfa1 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -147,7 +147,6 @@ func main() { ConflictHandler: conflictHandler, AnalyticsHandler: analyticsHandler, QueueHandler: queueHandler, - CollectionHandler: nil, // TODO: Initialize collection handler OPDSHandler: opdsHandler, ConnManager: connManager, QueueProcessor: queueProcessor, diff --git a/internal/app/app_test.go b/internal/app/app_test.go new file mode 100644 index 0000000..c7e5565 --- /dev/null +++ b/internal/app/app_test.go @@ -0,0 +1,373 @@ +package app + +import ( + "context" + "os" + "syscall" + "testing" + "time" + + "github.com/labstack/echo/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockHandler is a mock implementation of the Handler interface for testing +type mockHandler struct { + startSchedulerCalled bool + stopSchedulerCalled bool + startDelay time.Duration + stopDelay time.Duration + startError error + stopError error +} + +func (m *mockHandler) StartScheduler() { + m.startSchedulerCalled = true + if m.startDelay > 0 { + time.Sleep(m.startDelay) + } + if m.startError != nil { + panic(m.startError) + } +} + +func (m *mockHandler) StopScheduler() { + m.stopSchedulerCalled = true + if m.stopDelay > 0 { + time.Sleep(m.stopDelay) + } + if m.stopError != nil { + panic(m.stopError) + } +} + +// reset resets the mock handler state +func (m *mockHandler) reset() { + m.startSchedulerCalled = false + m.stopSchedulerCalled = false +} + +// TestApp_New tests Phase 5: App constructor +func TestApp_New(t *testing.T) { + e := echo.New() + handler := &mockHandler{} + + app := New(e, handler) + + assert.NotNil(t, app, "App should not be nil") + assert.Equal(t, e, app.echo, "Echo instance should be stored") + assert.Equal(t, handler, app.handler, "Handler should be stored") + assert.Equal(t, 30*time.Second, app.shutdownTimeout, "Default shutdown timeout should be 30 seconds") + assert.NotNil(t, app.shutdownDone, "Shutdown done channel should be initialized") +} + +// TestApp_SetShutdownTimeout tests Phase 5: configurable shutdown timeout +func TestApp_SetShutdownTimeout(t *testing.T) { + e := echo.New() + handler := &mockHandler{} + app := New(e, handler) + + customTimeout := 15 * time.Second + app.SetShutdownTimeout(customTimeout) + + assert.Equal(t, customTimeout, app.shutdownTimeout, "Shutdown timeout should be updated") +} + +// TestApp_ShutdownDone tests Phase 5: shutdown done channel +func TestApp_ShutdownDone(t *testing.T) { + e := echo.New() + handler := &mockHandler{} + app := New(e, handler) + + channel := app.ShutdownDone() + assert.NotNil(t, channel, "ShutdownDone should return a channel") + // Verify it's the same channel by checking if it's readable + select { + case <-channel: + // Channel should not be closed yet + t.Error("ShutdownDone channel should not be closed immediately") + default: + // Expected - channel is open but not ready + } +} + +// TestApp_Start_BackgroundServices tests Phase 1 & 5: background service startup +func TestApp_Start_BackgroundServices(t *testing.T) { + e := echo.New() + handler := &mockHandler{ + startDelay: 100 * time.Millisecond, // Short delay to verify async startup + } + app := New(e, handler) + + // Start the app in a goroutine (simulating main.go) + done := make(chan error, 1) + go func() { + // This will block until shutdown signal + err := app.Start() + done <- err + }() + + // Wait a bit for background services to start + time.Sleep(150 * time.Millisecond) + + // Verify scheduler was started + assert.True(t, handler.startSchedulerCalled, "StartScheduler should be called") + + // Send shutdown signal to clean up + done2 := make(chan bool) + go func() { + // Send shutdown signal + process, err := os.FindProcess(os.Getpid()) + require.NoError(t, err, "Should find current process") + err = process.Signal(syscall.SIGTERM) + done2 <- (err == nil) + }() + + // Wait for app to finish + select { + case err := <-done: + assert.NoError(t, err, "App should shut down without error") + case <-time.After(5 * time.Second): + t.Fatal("App did not shut down within timeout") + } + + <-done2 +} + +// TestApp_Shutdown_GracefulShutdown tests Phase 5: graceful shutdown sequence +func TestApp_Shutdown_GracefulShutdown(t *testing.T) { + e := echo.New() + handler := &mockHandler{ + stopDelay: 50 * time.Millisecond, + } + app := New(e, handler) + + // Test shutdown + err := app.Shutdown() + + assert.NoError(t, err, "Shutdown should complete without error") + assert.True(t, handler.stopSchedulerCalled, "StopScheduler should be called") +} + +// TestApp_Shutdown_ThreadSafety tests Phase 5: thread-safe shutdown +func TestApp_Shutdown_ThreadSafety(t *testing.T) { + e := echo.New() + handler := &mockHandler{} + app := New(e, handler) + + // Call shutdown multiple times concurrently + errors := make(chan error, 3) + for i := 0; i < 3; i++ { + go func() { + errors <- app.Shutdown() + }() + } + + // Collect results + for i := 0; i < 3; i++ { + err := <-errors + // First call should succeed, subsequent calls should also succeed (no-op) + assert.NoError(t, err, "Concurrent shutdown calls should not error") + } + + // Verify handler was stopped only once + assert.True(t, handler.stopSchedulerCalled, "StopScheduler should be called at least once") +} + +// TestApp_Shutdown_Timeout tests Phase 5: shutdown timeout handling +func TestApp_Shutdown_Timeout(t *testing.T) { + e := echo.New() + handler := &mockHandler{ + stopDelay: 2 * time.Second, // Longer than shutdown timeout + } + app := New(e, handler) + app.SetShutdownTimeout(100 * time.Millisecond) // Set very short timeout + + // Test shutdown with timeout + err := app.Shutdown() + + assert.Error(t, err, "Shutdown should timeout and return error") + assert.True(t, handler.stopSchedulerCalled, "StopScheduler should still be called") +} + +// TestApp_Shutdown_ClosesEchoServer tests Phase 5: HTTP server shutdown +func TestApp_Shutdown_ClosesEchoServer(t *testing.T) { + e := echo.New() + handler := &mockHandler{} + + // Start a simple server + go func() { + e.Start(":0") // Use random port + }() + + // Give server time to start + time.Sleep(100 * time.Millisecond) + + app := New(e, handler) + + // Shutdown should close the echo server + err := app.Shutdown() + + assert.NoError(t, err, "Shutdown should complete") + assert.True(t, handler.stopSchedulerCalled, "StopScheduler should be called") +} + +// TestApp_SignalHandling tests Phase 5: signal handling (SIGINT, SIGTERM, SIGQUIT) +func TestApp_SignalHandling(t *testing.T) { + tests := []struct { + name string + signal os.Signal + }{ + {"SIGINT", syscall.SIGINT}, + {"SIGTERM", syscall.SIGTERM}, + {"SIGQUIT", syscall.SIGQUIT}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + e := echo.New() + handler := &mockHandler{} + app := New(e, handler) + + // Start app in goroutine + done := make(chan error, 1) + go func() { + done <- app.Start() + }() + + // Wait for background services to start + time.Sleep(100 * time.Millisecond) + assert.True(t, handler.startSchedulerCalled, "StartScheduler should be called") + + // Send signal + process, err := os.FindProcess(os.Getpid()) + require.NoError(t, err, "Should find current process") + + err = process.Signal(tt.signal) + require.NoError(t, err, "Should send signal") + + // Wait for shutdown + select { + case err := <-done: + assert.NoError(t, err, "App should shut down without error") + assert.True(t, handler.stopSchedulerCalled, "StopScheduler should be called") + case <-time.After(5 * time.Second): + t.Fatal("App did not shut down within timeout") + } + }) + } +} + +// TestApp_Integration_StartupSequence tests Phase 1 & 5: complete startup sequence +func TestApp_Integration_StartupSequence(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + e := echo.New() + handler := &mockHandler{} + app := New(e, handler) + + // This test simulates the sequence in cmd/server/main.go: + // 1. Create app + // 2. Start HTTP server in background + // 3. Call app.Start() which blocks until signal + + // Step 1: App already created above + assert.NotNil(t, app, "App should be created") + + // Step 2: Start HTTP server (simulated) + serverReady := make(chan bool) + go func() { + // In real main.go, this would be: e.Start(":" + cfg.ServerPort) + // For testing, we don't actually start the server + serverReady <- true + <-context.Background().Done() // Block until done + }() + + <-serverReady + + // Step 3: Start app lifecycle in background + appDone := make(chan error, 1) + go func() { + appDone <- app.Start() + }() + + // Verify background services started + time.Sleep(150 * time.Millisecond) + assert.True(t, handler.startSchedulerCalled, "Scheduler should be started") + + // Send shutdown signal (simulating user pressing Ctrl+C) + process, err := os.FindProcess(os.Getpid()) + require.NoError(t, err) + err = process.Signal(syscall.SIGINT) + require.NoError(t, err) + + // Wait for graceful shutdown + select { + case err := <-appDone: + assert.NoError(t, err, "App should shut down gracefully") + assert.True(t, handler.stopSchedulerCalled, "Scheduler should be stopped") + case <-time.After(5 * time.Second): + t.Fatal("App did not complete shutdown within timeout") + } +} + +// TestApp_HandlerInterface tests that Handler interface is properly implemented +func TestApp_HandlerInterface(t *testing.T) { + // This test verifies that the mock handler satisfies the Handler interface + var _ Handler = &mockHandler{} +} + +// TestPhase1_AutoStartVerification tests Phase 1: auto-start functionality +func TestPhase1_AutoStartVerification(t *testing.T) { + t.Run("Auto-start runs asynchronously", func(t *testing.T) { + e := echo.New() + handler := &mockHandler{ + startDelay: 100 * time.Millisecond, + } + app := New(e, handler) + + // Start the app - it should start background services and return immediately + // Then we send a shutdown signal right away + done := make(chan error, 1) + go func() { + // This will start services in background, then wait for signal + err := app.Start() + done <- err + }() + + // Wait a bit for background services to start + time.Sleep(150 * time.Millisecond) + + // Verify scheduler was started in background + assert.True(t, handler.startSchedulerCalled, "StartScheduler should be called in background") + + // Send shutdown signal + process, err := os.FindProcess(os.Getpid()) + require.NoError(t, err) + _ = process.Signal(syscall.SIGTERM) + + // Wait for clean shutdown + select { + case err := <-done: + assert.NoError(t, err, "App should shut down gracefully") + case <-time.After(5 * time.Second): + t.Fatal("App did not shut down within timeout") + } + }) +} + +// BenchmarkApp_Shutdown benchmarks the shutdown process +func BenchmarkApp_Shutdown(b *testing.B) { + e := echo.New() + handler := &mockHandler{} + + b.ResetTimer() + for i := 0; i < b.N; i++ { + handler.reset() + app := New(e, handler) + app.Shutdown() + } +} diff --git a/internal/router/router.go b/internal/router/router.go index a86c167..69b5e8b 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -43,7 +43,6 @@ type Config struct { ConflictHandler *handlers.ConflictHandler AnalyticsHandler *handlers.AnalyticsHandler QueueHandler *handlers.QueueHandler - CollectionHandler *handlers.CollectionHandler OPDSHandler *handlers.OPDSHandler ConnManager *sync.ConnectionManager QueueProcessor *sync.SyncQueueProcessor diff --git a/internal/services/ebook_scanner_library_type_test.go b/internal/services/ebook_scanner_library_type_test.go new file mode 100644 index 0000000..e51c853 --- /dev/null +++ b/internal/services/ebook_scanner_library_type_test.go @@ -0,0 +1,253 @@ +package services + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestEbookScanner_LibraryTypeAwareScanning tests Phase 2: library-type-aware scanning +func TestEbookScanner_LibraryTypeAwareScanning(t *testing.T) { + t.Run("isScannableFile checks library type restrictions", func(t *testing.T) { + scanner := &EbookScanner{ + folders: []string{"/test/ebooks", "/test/comics"}, + libraryTypes: map[string][]string{ + "/test/ebooks": {".epub", ".mobi", ".azw3"}, + "/test/comics": {".cbz", ".cbr", ".cb7", ".cbt"}, + }, + } + + tests := []struct { + name string + filePath string + expected bool + }{ + { + name: "EPUB in ebooks folder", + filePath: "/test/ebooks/book.epub", + expected: true, + }, + { + name: "MOBI in ebooks folder", + filePath: "/test/ebooks/book.mobi", + expected: true, + }, + { + name: "CBZ in ebooks folder (should be rejected)", + filePath: "/test/ebooks/comic.cbz", + expected: false, + }, + { + name: "CBZ in comics folder", + filePath: "/test/comics/issue.cbz", + expected: true, + }, + { + name: "CBR in comics folder", + filePath: "/test/comics/issue.cbr", + expected: true, + }, + { + name: "EPUB in comics folder (should be rejected)", + filePath: "/test/comics/book.epub", + expected: false, + }, + { + name: "File outside watched folders", + filePath: "/other/path/file.epub", + expected: false, + }, + { + name: "Unknown extension in ebooks folder", + filePath: "/test/ebooks/file.pdf", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := scanner.isScannableFile(tt.filePath) + assert.Equal(t, tt.expected, result, "isScannableFile(%s) should return %v", tt.filePath, tt.expected) + }) + } + }) + + t.Run("isScannableFile case insensitive", func(t *testing.T) { + scanner := &EbookScanner{ + folders: []string{"/test/ebooks"}, + libraryTypes: map[string][]string{ + "/test/ebooks": {".epub"}, + }, + } + + tests := []struct { + name string + filePath string + expected bool + }{ + {"Lowercase extension", "/test/ebooks/book.epub", true}, + {"Uppercase extension", "/test/ebooks/book.EPUB", true}, + {"Mixed case extension", "/test/ebooks/book.Epub", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := scanner.isScannableFile(tt.filePath) + assert.Equal(t, tt.expected, result) + }) + } + }) + + t.Run("isScannableFile with no library type info", func(t *testing.T) { + scanner := &EbookScanner{ + folders: []string{"/test/ebooks"}, + libraryTypes: map[string][]string{}, // Empty library types + } + + result := scanner.isScannableFile("/test/ebooks/book.epub") + assert.False(t, result, "Should reject files when library type info is missing") + }) + + t.Run("isScannableFile handles subdirectories", func(t *testing.T) { + scanner := &EbookScanner{ + folders: []string{"/test/ebooks"}, + libraryTypes: map[string][]string{ + "/test/ebooks": {".epub"}, + }, + } + + result := scanner.isScannableFile("/test/ebooks/subdir/book.epub") + assert.True(t, result, "Should accept files in subdirectories") + }) +} + +// TestEbookScanner_SetFolders_BuildsLibraryTypeCache tests Phase 2: SetFolders builds cache +func TestEbookScanner_SetFolders_BuildsLibraryTypeCache(t *testing.T) { + // This is a unit test that verifies SetFolders properly initializes the libraryTypes cache + // Full integration testing would require a mock database + + t.Run("SetFolders initializes libraryTypes map", func(t *testing.T) { + scanner := &EbookScanner{ + libraryTypes: nil, + } + + // Simulate SetFolders initialization + scanner.libraryTypes = make(map[string][]string) + assert.NotNil(t, scanner.libraryTypes, "libraryTypes should be initialized") + assert.Equal(t, 0, len(scanner.libraryTypes), "libraryTypes should be empty initially") + }) + + t.Run("SetFolders clears old library types", func(t *testing.T) { + scanner := &EbookScanner{ + libraryTypes: map[string][]string{ + "/old/folder": {".epub"}, + }, + } + + // Simulate SetFolders clearing and rebuilding + scanner.libraryTypes = make(map[string][]string) + assert.Equal(t, 0, len(scanner.libraryTypes), "Old library types should be cleared") + }) +} + +// TestEbookScanner_LibraryTypeCrossContamination tests Phase 2: prevents cross-contamination +func TestEbookScanner_LibraryTypeCrossContamination(t *testing.T) { + t.Run("Ebook library rejects comic formats", func(t *testing.T) { + scanner := &EbookScanner{ + folders: []string{"/library/ebooks"}, + libraryTypes: map[string][]string{ + "/library/ebooks": {".epub", ".mobi", ".azw3", ".pdf"}, + }, + } + + comicFormats := []string{".cbz", ".cbr", ".cb7", ".cbt"} + for _, ext := range comicFormats { + filePath := "/library/ebooks/comic" + ext + result := scanner.isScannableFile(filePath) + assert.False(t, result, "Ebook library should reject %s files", ext) + } + }) + + t.Run("Comic library rejects ebook formats", func(t *testing.T) { + scanner := &EbookScanner{ + folders: []string{"/library/comics"}, + libraryTypes: map[string][]string{ + "/library/comics": {".cbz", ".cbr", ".cb7", ".cbt"}, + }, + } + + ebookFormats := []string{".epub", ".mobi", ".azw3", ".pdf", ".djvu"} + for _, ext := range ebookFormats { + filePath := "/library/comics/book" + ext + result := scanner.isScannableFile(filePath) + assert.False(t, result, "Comic library should reject %s files", ext) + } + }) +} + +// TestEbookScanner_MultipleLibraryTypes tests Phase 2: multiple libraries with different types +func TestEbookScanner_MultipleLibraryTypes(t *testing.T) { + scanner := &EbookScanner{ + folders: []string{ + "/library/ebooks", + "/library/comics", + "/library/manga", + }, + libraryTypes: map[string][]string{ + "/library/ebooks": {".epub", ".mobi", ".azw3"}, + "/library/comics": {".cbz", ".cbr"}, + "/library/manga": {".cbz", ".cb7"}, // Manga uses CBZ and CB7 + }, + } + + tests := []struct { + name string + filePath string + expected bool + reason string + }{ + { + name: "EPUB in ebook library", + filePath: "/library/ebooks/novel.epub", + expected: true, + reason: "EPUB allowed in ebook library", + }, + { + name: "CBZ in comic library", + filePath: "/library/comics/superman.cbz", + expected: true, + reason: "CBZ allowed in comic library", + }, + { + name: "CBZ in manga library", + filePath: "/library/manga/naruto.cbz", + expected: true, + reason: "CBZ allowed in manga library", + }, + { + name: "CB7 in manga library", + filePath: "/library/manga/onepiece.cb7", + expected: true, + reason: "CB7 allowed in manga library", + }, + { + name: "CB7 in comic library (rejected)", + filePath: "/library/comics/batman.cb7", + expected: false, + reason: "CB7 not allowed in comic library", + }, + { + name: "EPUB in comic library (rejected)", + filePath: "/library/comics/novel.epub", + expected: false, + reason: "EPUB not allowed in comic library", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := scanner.isScannableFile(tt.filePath) + assert.Equal(t, tt.expected, result, tt.reason) + }) + } +}