From b9f12eaf377b603a60574a0a66e97e90029f26d2 Mon Sep 17 00:00:00 2001 From: Michael Mayer Date: Sat, 7 Mar 2026 11:16:21 +0100 Subject: [PATCH] Folders: Improve slug collision handling for Unicode paths #5366 Signed-off-by: Michael Mayer --- internal/entity/album.go | 6 +- internal/entity/album_test.go | 2 +- internal/entity/folder.go | 26 +++++--- internal/entity/folder_album_slug.go | 63 +++++++++++++++++++ internal/entity/folder_test.go | 92 +++++++++++++++++++++++++--- pkg/txt/slug.go | 54 ++++++++++++++++ pkg/txt/slug_test.go | 15 ++++- 7 files changed, 235 insertions(+), 23 deletions(-) create mode 100644 internal/entity/folder_album_slug.go diff --git a/internal/entity/album.go b/internal/entity/album.go index de1dde1b5..d222b670c 100644 --- a/internal/entity/album.go +++ b/internal/entity/album.go @@ -401,9 +401,9 @@ func FindAlbumByAttr(slugs, filters []string, albumType string) *Album { // FindFolderAlbum looks up a folder album by its canonical path or slug. func FindFolderAlbum(albumPath string) *Album { albumPath = clean.SlashPath(albumPath) - albumSlug := txt.Slug(albumPath) + albumSlugs := folderAlbumSlugCandidates(albumPath) - if albumSlug == "" { + if len(albumSlugs) == 0 { return nil } @@ -418,7 +418,7 @@ func FindFolderAlbum(albumPath string) *Album { } // Fallback for legacy rows created before album_path was persisted. - stmt = UnscopedDb().Where("album_type = ? AND album_slug = ?", AlbumFolder, albumSlug). + stmt = UnscopedDb().Where("album_type = ? AND album_slug IN (?)", AlbumFolder, albumSlugs). Where("(album_path IS NULL OR album_path = '')") if stmt.First(&m).Error == nil { diff --git a/internal/entity/album_test.go b/internal/entity/album_test.go index 73c972285..04712ff2e 100644 --- a/internal/entity/album_test.go +++ b/internal/entity/album_test.go @@ -493,7 +493,7 @@ func TestFindFolderAlbum(t *testing.T) { legacy := &Album{ AlbumType: AlbumFolder, - AlbumSlug: txt.Slug(parentPath), + AlbumSlug: legacyFolderAlbumSlug(parentPath), AlbumPath: "", AlbumFilter: `path:"` + parentPath + `" public:true`, CreatedAt: Now(), diff --git a/internal/entity/folder.go b/internal/entity/folder.go index 63e3e09ca..03f3308cf 100644 --- a/internal/entity/folder.go +++ b/internal/entity/folder.go @@ -201,22 +201,28 @@ func ReconcileOriginalsFolderAlbums(rootPath string) (reconciled int, err error) } // originalsFolderAlbumReconcileScope returns the effective reconciliation scope for a path. -// If a slug collision with a sibling folder album is detected, the scope is expanded to -// include the parent folder path so related rows can be repaired together. +// If a slug collision is detected, the scope is expanded to the highest colliding +// ancestor folder path so related rows can be repaired together. func originalsFolderAlbumReconcileScope(rootPath string) string { rootPath = clean.UserPath(rootPath) - if rootPath == "" || !hasOriginalsFolderPath(rootPath) || !hasOriginalsFolderAlbumSlugCollision(rootPath) { + if rootPath == "" || !hasOriginalsFolderPath(rootPath) { return rootPath } - parentPath := path.Dir(rootPath) + scopePath := rootPath - if parentPath == "/" || parentPath == "." { - return rootPath + for hasOriginalsFolderAlbumSlugCollision(scopePath) { + parentPath := clean.UserPath(path.Dir(scopePath)) + + if parentPath == "" || parentPath == "." || parentPath == "/" || parentPath == scopePath { + break + } + + scopePath = parentPath } - return parentPath + return scopePath } // hasOriginalsFolderPath reports whether an active originals folder row exists for rootPath. @@ -234,16 +240,16 @@ func hasOriginalsFolderPath(rootPath string) bool { // hasOriginalsFolderAlbumSlugCollision reports whether rootPath collides with another // active folder album that shares the same slug but stores a different non-empty path. func hasOriginalsFolderAlbumSlugCollision(rootPath string) bool { - albumSlug := txt.Slug(rootPath) + albumSlugs := folderAlbumSlugCandidates(rootPath) - if albumSlug == "" { + if len(albumSlugs) == 0 { return false } var collisions int if err := Db().Model(&Album{}). - Where("album_type = ? AND album_slug = ? AND album_path <> '' AND album_path <> ?", AlbumFolder, albumSlug, rootPath). + Where("album_type = ? AND album_slug IN (?) AND album_path <> '' AND album_path <> ?", AlbumFolder, albumSlugs, rootPath). Count(&collisions).Error; err != nil { log.Debugf("folder: %s (check album slug collision for %s)", err, clean.LogQuote(rootPath)) return false diff --git a/internal/entity/folder_album_slug.go b/internal/entity/folder_album_slug.go new file mode 100644 index 000000000..cb9ad09b7 --- /dev/null +++ b/internal/entity/folder_album_slug.go @@ -0,0 +1,63 @@ +package entity + +import ( + "strings" + + "github.com/gosimple/slug" + + "github.com/photoprism/photoprism/pkg/clean" + "github.com/photoprism/photoprism/pkg/txt" +) + +// folderAlbumSlugCandidates returns the current and legacy slug candidates for a folder path. +func folderAlbumSlugCandidates(albumPath string) []string { + albumPath = clean.SlashPath(albumPath) + + if albumPath == "" { + return nil + } + + candidates := make([]string, 0, 2) + + for _, value := range []string{txt.Slug(albumPath), legacyFolderAlbumSlug(albumPath)} { + if value == "" { + continue + } + + duplicate := false + + for _, existing := range candidates { + if existing == value { + duplicate = true + break + } + } + + if !duplicate { + candidates = append(candidates, value) + } + } + + return candidates +} + +// legacyFolderAlbumSlug reproduces the folder album slug logic used before the 2026 collision fixes. +func legacyFolderAlbumSlug(albumPath string) string { + albumPath = strings.TrimSpace(clean.SlashPath(albumPath)) + + if albumPath == "" || albumPath == "-" { + return albumPath + } + + if albumPath[0] == txt.SlugEncoded && txt.ContainsAlnumLower(albumPath[1:]) { + return txt.Clip(albumPath, txt.ClipSlug) + } + + result := slug.Make(albumPath) + + if result == "" { + result = string(txt.SlugEncoded) + txt.SlugEncoding.EncodeToString([]byte(albumPath)) + } + + return txt.Clip(result, txt.ClipSlug) +} diff --git a/internal/entity/folder_test.go b/internal/entity/folder_test.go index 6fbc95391..382d814dc 100644 --- a/internal/entity/folder_test.go +++ b/internal/entity/folder_test.go @@ -549,11 +549,15 @@ func TestFolder_Create(t *testing.T) { // - title/filter point to pathWine _ = UnscopedDb().Where("album_type = ? AND album_path IN (?)", AlbumFolder, []string{pathMirror, pathWine}).Delete(Album{}).Error - stale := NewFolderAlbum(folderWine.Title(), pathMirror, `path:"`+pathWine+`" public:true`) - - if stale == nil { - t.Fatal("expected stale folder album") + stale := &Album{ + AlbumType: AlbumFolder, + AlbumSlug: legacyFolderAlbumSlug(pathMirror), + AlbumPath: pathMirror, + AlbumFilter: `path:"` + pathWine + `" public:true`, + CreatedAt: Now(), + UpdatedAt: Now(), } + stale.SetTitle(folderWine.Title()) if err := stale.Create(); err != nil { t.Fatal(err) @@ -608,11 +612,15 @@ func TestFolder_Create(t *testing.T) { // exists and points to the wrong folder metadata. _ = UnscopedDb().Where("album_type = ? AND album_path IN (?)", AlbumFolder, []string{pathMirror, pathWine}).Delete(Album{}).Error - stale := NewFolderAlbum(folderWine.Title(), pathMirror, `path:"`+pathWine+`" public:true`) - - if stale == nil { - t.Fatal("expected stale folder album") + stale := &Album{ + AlbumType: AlbumFolder, + AlbumSlug: legacyFolderAlbumSlug(pathMirror), + AlbumPath: pathMirror, + AlbumFilter: `path:"` + pathWine + `" public:true`, + CreatedAt: Now(), + UpdatedAt: Now(), } + stale.SetTitle(folderWine.Title()) if err := stale.Create(); err != nil { t.Fatal(err) @@ -644,4 +652,72 @@ func TestFolder_Create(t *testing.T) { assert.Equal(t, pathMirror, mirrorAlbum.AlbumPath) assert.Equal(t, pathWine, wineAlbum.AlbumPath) }) + t.Run("SingleDeepFolderRescanRepairsAncestorCollisionScope", func(t *testing.T) { + parentPath := "ins-deep-" + txt.Slug(time.Now().UTC().Format(time.RFC3339Nano)) + pathMirror := parentPath + "/🍷" + pathEmoji := parentPath + "/🪞" + pathPumpkin := pathEmoji + "/🎃" + pathLink := pathPumpkin + "/🔗" + pathFish := pathLink + "/🐠" + + folderMirror := NewFolder(RootOriginals, pathMirror, time.Now().UTC()) + folderEmoji := NewFolder(RootOriginals, pathEmoji, time.Now().UTC()) + folderPumpkin := NewFolder(RootOriginals, pathPumpkin, time.Now().UTC()) + folderLink := NewFolder(RootOriginals, pathLink, time.Now().UTC()) + folderFish := NewFolder(RootOriginals, pathFish, time.Now().UTC()) + + for _, folder := range []*Folder{&folderMirror, &folderEmoji, &folderPumpkin, &folderLink, &folderFish} { + if err := folder.Create(); err != nil { + t.Fatal(err) + } + } + + t.Cleanup(func() { + _ = UnscopedDb().Where("root = ? AND path IN (?)", RootOriginals, []string{pathMirror, pathEmoji, pathPumpkin, pathLink, pathFish}).Delete(Folder{}).Error + _ = UnscopedDb().Where("album_type = ? AND album_path IN (?)", AlbumFolder, []string{pathMirror, pathEmoji, pathPumpkin, pathLink, pathFish}).Delete(Album{}).Error + }) + + // Start from a stale collision state where a deep emoji-only folder has + // overwritten a sibling album outside its immediate parent subtree. + _ = UnscopedDb().Where("album_type = ? AND album_path IN (?)", AlbumFolder, []string{pathMirror, pathFish}).Delete(Album{}).Error + + stale := &Album{ + AlbumType: AlbumFolder, + AlbumSlug: legacyFolderAlbumSlug(pathMirror), + AlbumPath: pathMirror, + AlbumFilter: `path:"` + pathFish + `" public:true`, + CreatedAt: Now(), + UpdatedAt: Now(), + } + stale.SetTitle(folderFish.Title()) + + if err := stale.Create(); err != nil { + t.Fatal(err) + } + + reconciled, err := ReconcileOriginalsFolderAlbums(pathFish) + + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, 5, reconciled) + + mirrorAlbum := FindFolderAlbum(pathMirror) + + if mirrorAlbum == nil { + t.Fatal("expected mirror folder album") + } + + fishAlbum := FindFolderAlbum(pathFish) + + if fishAlbum == nil { + t.Fatal("expected fish folder album") + } + + assert.Equal(t, folderMirror.Title(), mirrorAlbum.AlbumTitle) + assert.Equal(t, folderFish.Title(), fishAlbum.AlbumTitle) + assert.Equal(t, pathMirror, mirrorAlbum.AlbumPath) + assert.Equal(t, pathFish, fishAlbum.AlbumPath) + }) } diff --git a/pkg/txt/slug.go b/pkg/txt/slug.go index 6aefa2225..845080807 100644 --- a/pkg/txt/slug.go +++ b/pkg/txt/slug.go @@ -1,8 +1,12 @@ package txt import ( + "crypto/sha256" "encoding/base32" + "fmt" "strings" + "unicode" + "unicode/utf8" "github.com/gosimple/slug" ) @@ -32,11 +36,61 @@ func Slug(s string) string { if result == "" { result = string(SlugEncoded) + SlugEncoding.EncodeToString([]byte(s)) + } else if tokens := slugRuneTokens(s); tokens != "" { + result += "-" + tokens + result = clipSlugWithHash(result, s) } return Clip(result, ClipSlug) } +// slugRuneTokens returns stable rune tokens for non-ASCII symbols that slug.Make would drop. +func slugRuneTokens(s string) string { + tokens := make([]string, 0, 4) + + for _, r := range s { + if r <= unicode.MaxASCII || slug.Make(string(r)) != "" { + continue + } + + switch { + case unicode.IsSymbol(r): + case r == '\u200d': + case r >= '\ufe00' && r <= '\ufe0f': + case r >= '\U0001f3fb' && r <= '\U0001f3ff': + default: + continue + } + + tokens = append(tokens, fmt.Sprintf("u%x", r)) + } + + return strings.Join(tokens, "-") +} + +// clipSlugWithHash clips long slugs and appends a deterministic hash suffix to avoid collisions. +func clipSlugWithHash(result, source string) string { + if utf8.RuneCountInString(result) <= ClipSlug { + return result + } + + hash := sha256.Sum256([]byte(source)) + suffix := fmt.Sprintf("%x", hash[:4]) + prefixLen := ClipSlug - len(suffix) - 1 + + if prefixLen <= 0 { + return Clip(result, ClipSlug) + } + + prefix := strings.TrimRight(Clip(result, prefixLen), "-") + + if prefix == "" { + return Clip(result, ClipSlug) + } + + return prefix + "-" + suffix +} + // SlugToTitle converts a slug back to a title func SlugToTitle(s string) string { if s == "" { diff --git a/pkg/txt/slug_test.go b/pkg/txt/slug_test.go index 3f91c6155..d68e5f793 100644 --- a/pkg/txt/slug_test.go +++ b/pkg/txt/slug_test.go @@ -1,7 +1,9 @@ package txt import ( + "strings" "testing" + "unicode/utf8" "github.com/stretchr/testify/assert" ) @@ -24,7 +26,9 @@ func TestSlug(t *testing.T) { assert.Equal(t, "_5cpzfea", Slug("💐")) assert.Equal(t, "_5cpzfea", Slug(" 💐 ")) assert.Equal(t, "_5cpzfdxqt5jja", Slug("💎💐")) - assert.Equal(t, "photoprism", Slug("PhotoPrism 💎")) + assert.Equal(t, "photoprism-u1f48e", Slug("PhotoPrism 💎")) + assert.Equal(t, "ins-u1f377", Slug("ins/🍷")) + assert.Equal(t, "work-u1f618", Slug("Work 😘")) assert.Equal(t, "_3kmib24yr3", Slug("_3kmib24yr3")) assert.Equal(t, "-", Slug("-")) assert.Equal(t, "_", Slug("_")) @@ -32,6 +36,15 @@ func TestSlug(t *testing.T) { assert.Equal(t, "_5cpzfea", Slug("_5cpzfea")) assert.Equal(t, "_5cpzfdxqt5jja", Slug("_5cpzfdxqt5jja")) }) + t.Run("LongInputUsesHashSuffix", func(t *testing.T) { + base := strings.Repeat("Very Long Prefix 💎 ", 8) + slugA := Slug(base + "Alpha") + slugB := Slug(base + "Beta") + + assert.NotEqual(t, slugA, slugB) + assert.Equal(t, ClipSlug, utf8.RuneCountInString(slugA)) + assert.Equal(t, ClipSlug, utf8.RuneCountInString(slugB)) + }) } func TestSlugToTitle(t *testing.T) {