mirror of
https://github.com/Belphemur/CBZOptimizer.git
synced 2026-07-22 11:25:40 +02:00
fix(converter): preserve OriginalName through conversion and fix race
- Fix critical bug: convertPageFile and splitAndConvert now copy OriginalName to the output PageFile so --keep-filenames works end-to-end on actually-converted pages. - Fix concurrency race: ExtractChapter deduplicates OriginalName when source archive has same base filename in different subdirectories. - Fix collision fallback: resolvePageName loops until an unused indexed name is found instead of single-shot assignment. - Fix gofmt indentation in optimize_command.go.
This commit is contained in:
@@ -128,15 +128,15 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error {
|
|||||||
log.Debug().Int("worker_id", workerID).Msg("Worker started")
|
log.Debug().Int("worker_id", workerID).Msg("Worker started")
|
||||||
for path := range fileChan {
|
for path := range fileChan {
|
||||||
log.Debug().Int("worker_id", workerID).Str("file_path", path).Msg("Worker processing file")
|
log.Debug().Int("worker_id", workerID).Str("file_path", path).Msg("Worker processing file")
|
||||||
err := utils2.Optimize(&utils2.OptimizeOptions{
|
err := utils2.Optimize(&utils2.OptimizeOptions{
|
||||||
ChapterConverter: chapterConverter,
|
ChapterConverter: chapterConverter,
|
||||||
Path: path,
|
Path: path,
|
||||||
Quality: quality,
|
Quality: quality,
|
||||||
Override: override,
|
Override: override,
|
||||||
Split: split,
|
Split: split,
|
||||||
KeepFilenames: keepFilenames,
|
KeepFilenames: keepFilenames,
|
||||||
Timeout: timeout,
|
Timeout: timeout,
|
||||||
})
|
})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
log.Error().Int("worker_id", workerID).Str("file_path", path).Err(err).Msg("Worker encountered error")
|
log.Error().Int("worker_id", workerID).Str("file_path", path).Err(err).Msg("Worker encountered error")
|
||||||
errMutex.Lock()
|
errMutex.Lock()
|
||||||
|
|||||||
@@ -42,9 +42,16 @@ func resolvePageName(page *manga.PageFile, usedNames map[string]struct{}) string
|
|||||||
}
|
}
|
||||||
// Collision: fall back to the indexed form so the entry still gets
|
// Collision: fall back to the indexed form so the entry still gets
|
||||||
// written, but make it visibly distinct from the preserved one.
|
// written, but make it visibly distinct from the preserved one.
|
||||||
fallback := fmt.Sprintf("%s_%04d%s", stem, page.Index, page.Extension)
|
// Loop in case the fallback name is itself already taken (e.g. a
|
||||||
usedNames[fallback] = struct{}{}
|
// source file literally named "cover_0001.png" colliding with a
|
||||||
return fallback
|
// page-index 1 fallback "cover_0001.webp").
|
||||||
|
for suffix := page.Index; ; suffix++ {
|
||||||
|
fallback := fmt.Sprintf("%s_%04d%s", stem, suffix, page.Extension)
|
||||||
|
if _, taken := usedNames[fallback]; !taken {
|
||||||
|
usedNames[fallback] = struct{}{}
|
||||||
|
return fallback
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if page.IsSplitted {
|
if page.IsSplitted {
|
||||||
|
|||||||
@@ -168,6 +168,18 @@ func ExtractChapter(ctx context.Context, filePath string, keepFilenames bool) (*
|
|||||||
TempDir: tempDir,
|
TempDir: tempDir,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// usedOriginalNames tracks every OriginalName handed out in this chapter
|
||||||
|
// so it stays unique across pages, even when the source archive contains
|
||||||
|
// the same base filename in different subdirectories (e.g. vol1/page.png
|
||||||
|
// and vol2/page.png — legal in zip). Uniqueness at the extraction
|
||||||
|
// boundary prevents a downstream race in pkg/converter/webp, where two
|
||||||
|
// pages with the same stem would write the same intermediate file.
|
||||||
|
// Allocated lazily so the keepFilenames=false path stays allocation-free.
|
||||||
|
var usedOriginalNames map[string]struct{}
|
||||||
|
if keepFilenames {
|
||||||
|
usedOriginalNames = make(map[string]struct{})
|
||||||
|
}
|
||||||
|
|
||||||
// For CBZ files, read metadata from zip comment
|
// For CBZ files, read metadata from zip comment
|
||||||
pathLower := strings.ToLower(filepath.Ext(filePath))
|
pathLower := strings.ToLower(filepath.Ext(filePath))
|
||||||
if pathLower == ".cbz" {
|
if pathLower == ".cbz" {
|
||||||
@@ -286,7 +298,7 @@ func ExtractChapter(ctx context.Context, filePath string, keepFilenames bool) (*
|
|||||||
FilePath: outputPath,
|
FilePath: outputPath,
|
||||||
}
|
}
|
||||||
if keepFilenames {
|
if keepFilenames {
|
||||||
page.OriginalName = filepath.Base(path)
|
page.OriginalName = allocateUniqueBaseName(filepath.Base(path), pageIndex, usedOriginalNames)
|
||||||
}
|
}
|
||||||
chapter.Pages = append(chapter.Pages, page)
|
chapter.Pages = append(chapter.Pages, page)
|
||||||
|
|
||||||
@@ -328,6 +340,33 @@ func isJunkFile(path string) bool {
|
|||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// allocateUniqueBaseName returns baseName when it is not already taken by an
|
||||||
|
// earlier page in this chapter, or a collision-resolved variant otherwise.
|
||||||
|
//
|
||||||
|
// On collision the resolved name matches the existing fallback style used by
|
||||||
|
// cbz_creator's resolvePageName: stem + "_%04d" + extension. The starting
|
||||||
|
// suffix is pageIndex so the resolved name stays in the same neighborhood as
|
||||||
|
// the page's archive position. If that candidate is itself already taken
|
||||||
|
// (astronomically rare: the source archive would need both the original name
|
||||||
|
// and a matching indexed name), the suffix is incremented until a free name
|
||||||
|
// is found. The chosen name is recorded in usedNames so subsequent calls
|
||||||
|
// cannot pick it again.
|
||||||
|
func allocateUniqueBaseName(baseName string, pageIndex uint16, usedNames map[string]struct{}) string {
|
||||||
|
if _, taken := usedNames[baseName]; !taken {
|
||||||
|
usedNames[baseName] = struct{}{}
|
||||||
|
return baseName
|
||||||
|
}
|
||||||
|
ext := filepath.Ext(baseName)
|
||||||
|
stem := strings.TrimSuffix(baseName, ext)
|
||||||
|
for suffix := int(pageIndex); ; suffix++ {
|
||||||
|
candidate := fmt.Sprintf("%s_%04d%s", stem, suffix, ext)
|
||||||
|
if _, taken := usedNames[candidate]; !taken {
|
||||||
|
usedNames[candidate] = struct{}{}
|
||||||
|
return candidate
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// LoadChapter extracts the chapter from a CBZ/CBR file to disk.
|
// LoadChapter extracts the chapter from a CBZ/CBR file to disk.
|
||||||
// It delegates to ExtractChapter with keepFilenames=false and always
|
// It delegates to ExtractChapter with keepFilenames=false and always
|
||||||
// extracts all pages. Use IsAlreadyConverted for a fast conversion status
|
// extracts all pages. Use IsAlreadyConverted for a fast conversion status
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import (
|
|||||||
"fmt"
|
"fmt"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"regexp"
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
@@ -202,6 +203,84 @@ func TestExtractChapter_PagesHaveSequentialIndices(t *testing.T) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// writeCollisionCBZ builds a CBZ containing the same image base name in two
|
||||||
|
// different subdirectories plus a normal unique page. The test fixture is
|
||||||
|
// specifically crafted to reproduce the race fixed in ExtractChapter: when
|
||||||
|
// keep-filenames naively copied filepath.Base(path) into OriginalName, both
|
||||||
|
// a/page.png and b/page.png would have ended up with the same stem and the
|
||||||
|
// WebP converter would have raced on a single shared intermediate path.
|
||||||
|
func writeCollisionCBZ(t *testing.T, dir string) string {
|
||||||
|
t.Helper()
|
||||||
|
cbzPath := filepath.Join(dir, "collisions.cbz")
|
||||||
|
f, err := os.Create(cbzPath)
|
||||||
|
require.NoError(t, err)
|
||||||
|
w := zip.NewWriter(f)
|
||||||
|
|
||||||
|
for _, entry := range []string{"a/page.png", "b/page.png", "cover.png"} {
|
||||||
|
fw, err := w.Create(entry)
|
||||||
|
require.NoError(t, err)
|
||||||
|
_, err = fw.Write([]byte("dummy image bytes for " + entry))
|
||||||
|
require.NoError(t, err)
|
||||||
|
}
|
||||||
|
require.NoError(t, w.Close())
|
||||||
|
require.NoError(t, f.Close())
|
||||||
|
return cbzPath
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestExtractChapter_KeepFilenames_DeduplicatesCollidingNames(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
cbzPath := writeCollisionCBZ(t, tmpDir)
|
||||||
|
|
||||||
|
t.Run("keep-filenames resolves colliding base names", func(t *testing.T) {
|
||||||
|
chapter, err := ExtractChapter(context.Background(), cbzPath, true)
|
||||||
|
require.NoError(t, err)
|
||||||
|
defer func() { _ = chapter.Cleanup() }()
|
||||||
|
|
||||||
|
require.Len(t, chapter.Pages, 3, "expected 3 pages: two colliding + one unique")
|
||||||
|
|
||||||
|
// All OriginalNames must be non-empty bare base names and unique.
|
||||||
|
seen := make(map[string]struct{}, len(chapter.Pages))
|
||||||
|
for _, page := range chapter.Pages {
|
||||||
|
assert.NotEmpty(t, page.OriginalName, "page %d must have OriginalName set", page.Index)
|
||||||
|
assert.Equal(t, filepath.Base(page.OriginalName), page.OriginalName,
|
||||||
|
"page %d OriginalName should be a bare base filename", page.Index)
|
||||||
|
if _, dup := seen[page.OriginalName]; dup {
|
||||||
|
t.Errorf("duplicate OriginalName across pages: %q", page.OriginalName)
|
||||||
|
}
|
||||||
|
seen[page.OriginalName] = struct{}{}
|
||||||
|
}
|
||||||
|
|
||||||
|
// The unique page keeps its original name; the two colliding pages
|
||||||
|
// must come out as one bare "page.png" and one indexed variant.
|
||||||
|
assert.Contains(t, seen, "page.png", "one colliding page should keep the bare page.png name")
|
||||||
|
assert.Contains(t, seen, "cover.png", "the unique page should keep its original name")
|
||||||
|
|
||||||
|
indexedPattern := regexp.MustCompile(`^page_\d{4}\.png$`)
|
||||||
|
indexedCount := 0
|
||||||
|
for name := range seen {
|
||||||
|
if indexedPattern.MatchString(name) {
|
||||||
|
indexedCount++
|
||||||
|
}
|
||||||
|
}
|
||||||
|
assert.Equal(t, 1, indexedCount,
|
||||||
|
"exactly one colliding page should be resolved to the page_NNNN.png pattern, got %v", seen)
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("keep-filenames off leaves OriginalName empty (regression guard)", func(t *testing.T) {
|
||||||
|
// Same collision-prone archive, but with keep-filenames off: the
|
||||||
|
// dedup logic must not run, so OriginalName stays empty for every
|
||||||
|
// page. This is the historical default and must not regress.
|
||||||
|
chapter, err := ExtractChapter(context.Background(), cbzPath, false)
|
||||||
|
require.NoError(t, err)
|
||||||
|
defer func() { _ = chapter.Cleanup() }()
|
||||||
|
|
||||||
|
require.Len(t, chapter.Pages, 3)
|
||||||
|
for _, page := range chapter.Pages {
|
||||||
|
assert.Empty(t, page.OriginalName, "page %d must have empty OriginalName when keep-filenames is off", page.Index)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
func TestExtractChapter_Cleanup(t *testing.T) {
|
func TestExtractChapter_Cleanup(t *testing.T) {
|
||||||
chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz", false)
|
chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz", false)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|||||||
@@ -251,11 +251,13 @@ func (converter *Converter) convertPageFile(ctx context.Context, page *manga.Pag
|
|||||||
err := EncodeFile(page.FilePath, outputPath, uint(quality))
|
err := EncodeFile(page.FilePath, outputPath, uint(quality))
|
||||||
|
|
||||||
if err == nil {
|
if err == nil {
|
||||||
// Success! No image decoding needed.
|
// Success! No image decoding needed. Preserve OriginalName so
|
||||||
|
// --keep-filenames carries through to the final zip entry name.
|
||||||
return []*manga.PageFile{{
|
return []*manga.PageFile{{
|
||||||
Index: page.Index,
|
Index: page.Index,
|
||||||
Extension: ".webp",
|
Extension: ".webp",
|
||||||
FilePath: outputPath,
|
FilePath: outputPath,
|
||||||
|
OriginalName: page.OriginalName,
|
||||||
}}, nil
|
}}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -356,6 +358,7 @@ func (converter *Converter) splitAndConvert(ctx context.Context, page *manga.Pag
|
|||||||
FilePath: outputPath,
|
FilePath: outputPath,
|
||||||
IsSplitted: true,
|
IsSplitted: true,
|
||||||
SplitPartIndex: uint16(i),
|
SplitPartIndex: uint16(i),
|
||||||
|
OriginalName: page.OriginalName,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user