fix(importer): move single-file audiobooks into their folder on reorganize (#2894)

changelog.d/2894-reorganize-single-file-audiobook.md

@@ -0,0 +1,2 @@
+### Fixed
+- **Rename files no longer turns a single-file audiobook into a file with no extension** (#2894): an audiobook tracked as the `.m4b` itself, which is what a library scan records for a flat `Author/Title.m4b` layout, was moved onto its templated folder path, so `Stephen King/The Shining.m4b` became a file named `Stephen King/The Shining (1977)`. Rename then reported it as already correct, and an ebook import for the same book failed with `not a directory`. The file now moves inside the templated folder under its own name, its `metadata.opf` goes in that folder, and a file already in its folder stays put instead of being moved to `Title (Year) (2)`. With a shared library root it joins the book's ebook folder, the same as an import. A file already flattened by the old behaviour is now shown as an error in Rename files instead of being moved again, and needs fixing by hand: move it into a folder and give it back its extension (for example `The Shining (1977)/The Shining.m4b`), use **Forget this file** on the book for the old path (not Delete, which removes it from disk), then run a library scan so Bindery picks up the folder. Thanks zikeji for the report and the fix.

internal/importer/destination_preview_test.go

@@ -80,9 +80,9 @@ func TestPreviewImportDestination_FormatHintWins(t *testing.T) {
 	if err != nil {
 		t.Fatalf("PreviewImportDestination: %v", err)
 	}
-	want := filepath.Join(audiobookDir, "Jane Doe", "Right Book (2020)")
+	want := filepath.Join(audiobookDir, "Jane Doe", "Right Book (2020)", "mislabelled.epub")
 	if got.Destination != want {
-		t.Errorf("destination = %q, want the audiobook root %q", got.Destination, want)
+		t.Errorf("destination = %q, want inside the audiobook root %q", got.Destination, want)
 	}
 }
 

internal/importer/reorganize.go

@@ -160,7 +160,14 @@
 	// than a false collision (or an endless (N)→(N+1) churn). Ebooks are single
 	// files and are not uniquified at import, so they skip this.
 	if audiobook {
-		dest = uniqueDirExcluding(dest, f.Path)
+		if isSingleFile(f.Path) {
+			if flattenedOntoFolderPath(f.Path, dest) {
+				return dest, ReorgStatusError, flattenedOntoFolderMsg
+			}
+			dest = s.singleFileAudiobookDest(ctx, book, dest, f.Path)
+		} else {
+			dest = uniqueDirExcluding(dest, f.Path)
+		}
 	}
 
 	if filepath.Clean(dest) == filepath.Clean(f.Path) {
@@ -200,6 +207,39 @@
 	return dest, ReorgStatusMove, ""
 }
 
+func isSingleFile(path string) bool {
+	info, err := os.Stat(path)
+	return err == nil && !info.IsDir()
+}
+
+// singleFileAudiobookDest mirrors the single-file import branch: join the
+// book's own ebook folder (#2686), otherwise uniquify with the file's current
+// folder treated as available so a file already in place stays a noop.
+func (s *Scanner) singleFileAudiobookDest(ctx context.Context, book *models.Book, destDir, current string) string {
+	if existing, merging := s.existingEbookDir(ctx, book); !merging || filepath.Clean(destDir) != existing {
+		destDir = uniqueDirExcluding(destDir, filepath.Dir(current))
+	}
+	return filepath.Join(destDir, filepath.Base(current))
+}
+
+const flattenedOntoFolderMsg = "this audiobook file sits where its folder belongs and has lost its extension (#2894). " +
+	"Move it into a folder and restore its extension, use Forget this file on the book for the old path, then run a library scan"
+
+// flattenedOntoFolderPath reports whether a tracked single audiobook file is
+// what Rename files left behind before #2894 was fixed: the file was moved onto
+// its templated folder path itself (or the " (N)" variant it uniquified to), so
+// it has the folder's name and no audio extension. Older releases read this as
+// a noop. Proposing a move from here would bury the extensionless file one
+// level down in a " (2)" folder, so it is reported instead and left for the
+// manual recovery the message describes.
+func flattenedOntoFolderPath(path, destDir string) bool {
+	if IsAudioFile(path) {
+		return false
+	}
+	p, d := filepath.Clean(path), filepath.Clean(destDir)
+	return p == d || strings.HasPrefix(p, d+" (")
+}
+
 // uniqueDirExcluding mirrors UniqueDir but treats keep as if it were absent, so
 // resolving the destination for a file already parked at a uniquified location
 // returns that same location (a noop) instead of bumping to the next suffix.
@@ -271,6 +311,7 @@
 		return m
 	}
 
+	singleFile := isSingleFile(file.Path)
 	if err := s.moveTrackedFile(ctx, file, proposed); err != nil {
 		m.Status = ReorgStatusFailed
 		m.Message = err.Error()
@@ -288,11 +329,12 @@
 		return m
 	}
 
-	// A single-file ebook leaves its metadata.opf behind when it moves out of a
-	// folder, which both strands a stale sidecar and blocks the prune below
-	// from reclaiming the now-empty folder. An audiobook moves as a whole
-	// directory, so its sidecar travels with it and there is nothing to clean.
-	if file.Format != models.MediaTypeAudiobook {
+	// A single file (an ebook, or a lone audiobook file) leaves its
+	// metadata.opf behind when it moves out of a folder, which both strands a
+	// stale sidecar and blocks the prune below from reclaiming the now-empty
+	// folder. An audiobook folder moves whole, so its sidecar travels with it
+	// and there is nothing to clean.
+	if singleFile {
 		removeOrphanedSidecar(filepath.Dir(file.Path), filepath.Dir(proposed))
 	}
 	pruneEmptyParents(filepath.Dir(file.Path), s.rootsForFormat(ctx, author, file.Format))
@@ -315,7 +357,7 @@
 	// feature that is off by default.
 	if s.opfSidecarEnabled(ctx) {
 		sidecarDir := proposed
-		if file.Format != models.MediaTypeAudiobook {
+		if singleFile {
 			sidecarDir = filepath.Dir(proposed)
 		}
 		edition := s.resolveCalibreEdition(ctx, nil, book)

internal/importer/reorganize_test.go

@@ -595,3 +595,215 @@ func TestReorganize_NoSidecarWhenDisabled(t *testing.T) {
 		t.Errorf("no sidecar may be written while the setting is off, stat err = %v", err)
 	}
 }
+
+// TestReorganize_SingleFileAudiobookMovesIntoFolder is the regression test for
+// #2894. An audiobook tracked as a lone .m4b used to be given the templated
+// folder path as its own destination, so the move renamed the file to
+// "My Book (2020)" with no extension and no folder.
+func TestReorganize_SingleFileAudiobookMovesIntoFolder(t *testing.T) {
+	env, _, audiobookDir, ctx := reorgFixture(t)
+	book := env.seed(t, ctx, "Jane Doe", "My Book")
+
+	oldPath := filepath.Join(audiobookDir, "Jane Doe", "My Book.m4b")
+	writeFileAt(t, oldPath)
+	if err := env.books.AddBookFile(ctx, book.ID, models.MediaTypeAudiobook, oldPath); err != nil {
+		t.Fatal(err)
+	}
+
+	want := filepath.Join(audiobookDir, "Jane Doe", "My Book (2020)", "My Book.m4b")
+	moves, err := env.s.PreviewReorganizeBook(ctx, book.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(moves) != 1 || moves[0].Status != ReorgStatusMove || moves[0].Proposed != want {
+		t.Fatalf("preview = %+v, want move to %q", moves, want)
+	}
+
+	results := env.s.ApplyReorganize(ctx, []int64{moves[0].FileID})
+	if results[0].Status != ReorgStatusMoved {
+		t.Fatalf("apply = %+v, want moved", results[0])
+	}
+	info, err := os.Stat(want)
+	if err != nil {
+		t.Fatalf("file not at templated location: %v", err)
+	}
+	if !info.Mode().IsRegular() {
+		t.Errorf("%q is %v, want a regular file", want, info.Mode())
+	}
+	files, err := env.books.ListBookFiles(ctx, book.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(files) != 1 || files[0].Path != want {
+		t.Errorf("book_files path = %v, want %q", files, want)
+	}
+}
+
+// A single-file audiobook already inside its templated folder is a noop. The
+// folder exists because the file is in it, so it must not read as a collision
+// and push the destination to "My Book (2020) (2)".
+func TestReorganize_SingleFileAudiobookInPlaceIsNoop(t *testing.T) {
+	env, _, audiobookDir, ctx := reorgFixture(t)
+	book := env.seed(t, ctx, "Jane Doe", "My Book")
+
+	path := filepath.Join(audiobookDir, "Jane Doe", "My Book (2020)", "My Book.m4b")
+	writeFileAt(t, path)
+	if err := env.books.AddBookFile(ctx, book.ID, models.MediaTypeAudiobook, path); err != nil {
+		t.Fatal(err)
+	}
+
+	moves, err := env.s.PreviewReorganizeBook(ctx, book.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(moves) != 1 || moves[0].Status != ReorgStatusNoop || moves[0].Proposed != path {
+		t.Fatalf("preview = %+v, want noop at %q", moves, path)
+	}
+}
+
+func TestReorganize_SingleFileAudiobookSidecarGoesInsideTheFolder(t *testing.T) {
+	env, _, audiobookDir, ctx := reorgFixture(t)
+	env.enableOPFSidecar(t, ctx)
+	book := env.seed(t, ctx, "Jane Doe", "My Book")
+
+	oldPath := filepath.Join(audiobookDir, "Jane Doe", "My Book.m4b")
+	writeFileAt(t, oldPath)
+	if err := env.books.AddBookFile(ctx, book.ID, models.MediaTypeAudiobook, oldPath); err != nil {
+		t.Fatal(err)
+	}
+
+	moves, err := env.s.PreviewReorganizeBook(ctx, book.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if results := env.s.ApplyReorganize(ctx, []int64{moves[0].FileID}); results[0].Status != ReorgStatusMoved {
+		t.Fatalf("apply = %+v, want moved", results[0])
+	}
+
+	folder := filepath.Join(audiobookDir, "Jane Doe", "My Book (2020)")
+	if _, err := os.Stat(filepath.Join(folder, "metadata.opf")); err != nil {
+		t.Errorf("sidecar should be inside the audiobook folder: %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(folder, "My Book.m4b")); err != nil {
+		t.Errorf("audiobook should be inside its folder: %v", err)
+	}
+}
+
+// With a shared root the templated audiobook folder is the ebook's folder. The
+// single-file import merges into it (#2686), so reorganize must place the file
+// there too rather than in "My Book (2020) (2)".
+func TestReorganize_SingleFileAudiobookJoinsItsEbookFolder(t *testing.T) {
+	database, err := db.OpenMemory()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { database.Close() })
+	ctx := context.Background()
+	root := t.TempDir()
+	books := db.NewBookRepo(database)
+	authors := db.NewAuthorRepo(database)
+	s := NewScanner(db.NewDownloadRepo(database), db.NewDownloadClientRepo(database),
+		books, authors, db.NewHistoryRepo(database), root, root, "", "", "")
+	s.WithSettings(db.NewSettingsRepo(database))
+	s.WithRootFolders(db.NewRootFolderRepo(database))
+	s.WithSeriesRepo(db.NewSeriesRepo(database))
+	env := reorgEnv{s: s, books: books, authors: authors}
+
+	book := env.seed(t, ctx, "Jane Doe", "My Book")
+	ebook := filepath.Join(root, "Jane Doe", "My Book (2020)", "My Book - Jane Doe.epub")
+	writeFileAt(t, ebook)
+	if err := books.AddBookFile(ctx, book.ID, models.MediaTypeEbook, ebook); err != nil {
+		t.Fatal(err)
+	}
+	audio := filepath.Join(root, "Jane Doe", "My Book.m4b")
+	writeFileAt(t, audio)
+	if err := books.AddBookFile(ctx, book.ID, models.MediaTypeAudiobook, audio); err != nil {
+		t.Fatal(err)
+	}
+
+	want := filepath.Join(root, "Jane Doe", "My Book (2020)", "My Book.m4b")
+	moves, err := s.PreviewReorganizeBook(ctx, book.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, m := range moves {
+		switch m.Format {
+		case models.MediaTypeEbook:
+			if m.Status != ReorgStatusNoop {
+				t.Errorf("ebook: status = %q (%s), want noop", m.Status, m.Message)
+			}
+		case models.MediaTypeAudiobook:
+			if m.Status != ReorgStatusMove || m.Proposed != want {
+				t.Errorf("audiobook: %+v, want move to %q", m, want)
+			}
+			if results := s.ApplyReorganize(ctx, []int64{m.FileID}); results[0].Status != ReorgStatusMoved {
+				t.Fatalf("apply = %+v, want moved", results[0])
+			}
+		}
+	}
+	for _, p := range []string{ebook, want} {
+		if _, err := os.Stat(p); err != nil {
+			t.Errorf("%q should be in the shared folder after apply: %v", p, err)
+		}
+	}
+}
+
+// A file already flattened by the old #2894 behaviour sits on the folder path
+// itself (or its " (N)" variant) with no extension. Proposing a move from there
+// would push it, still extensionless, into "My Book (2020) (2)/My Book (2020)".
+// It must be reported as an error with the recovery steps and left untouched.
+func TestReorganize_SingleFileAudiobookFlattenedOntoFolderIsReported(t *testing.T) {
+	for _, name := range []string{"My Book (2020)", "My Book (2020) (2)"} {
+		t.Run(name, func(t *testing.T) {
+			env, _, audiobookDir, ctx := reorgFixture(t)
+			book := env.seed(t, ctx, "Jane Doe", "My Book")
+
+			path := filepath.Join(audiobookDir, "Jane Doe", name)
+			writeFileAt(t, path)
+			if err := env.books.AddBookFile(ctx, book.ID, models.MediaTypeAudiobook, path); err != nil {
+				t.Fatal(err)
+			}
+
+			moves, err := env.s.PreviewReorganizeBook(ctx, book.ID)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if len(moves) != 1 || moves[0].Status != ReorgStatusError || !strings.Contains(moves[0].Message, "#2894") {
+				t.Fatalf("preview = %+v, want an error naming #2894", moves)
+			}
+			if results := env.s.ApplyReorganize(ctx, []int64{moves[0].FileID}); results[0].Status != ReorgStatusError {
+				t.Fatalf("apply = %+v, want it left alone", results[0])
+			}
+			if info, err := os.Stat(path); err != nil || !info.Mode().IsRegular() {
+				t.Errorf("flattened file must stay where it is: %v", err)
+			}
+		})
+	}
+}
+
+// A lone audiobook file moving out of an old folder leaves that folder's
+// metadata.opf behind, the same as an ebook. It must be cleaned up so the old
+// folder can be pruned.
+func TestReorganize_SingleFileAudiobookLeavesNoOrphanSidecar(t *testing.T) {
+	env, _, audiobookDir, ctx := reorgFixture(t)
+	book := env.seed(t, ctx, "Jane Doe", "My Book")
+
+	oldDir := filepath.Join(audiobookDir, "Jane Doe", "Old Title (2020)")
+	oldPath := filepath.Join(oldDir, "My Book.m4b")
+	writeFileAt(t, oldPath)
+	writeFileAt(t, filepath.Join(oldDir, "metadata.opf"))
+	if err := env.books.AddBookFile(ctx, book.ID, models.MediaTypeAudiobook, oldPath); err != nil {
+		t.Fatal(err)
+	}
+
+	moves, err := env.s.PreviewReorganizeBook(ctx, book.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if results := env.s.ApplyReorganize(ctx, []int64{moves[0].FileID}); results[0].Status != ReorgStatusMoved {
+		t.Fatalf("apply = %+v, want moved", results[0])
+	}
+	if _, err := os.Stat(oldDir); !os.IsNotExist(err) {
+		t.Errorf("old folder should be pruned once its stale sidecar is removed, stat err = %v", err)
+	}
+}