diff --git a/backend/internal/e2e/copy_edit_test.go b/backend/internal/e2e/copy_edit_test.go new file mode 100644 index 0000000..b587e69 --- /dev/null +++ b/backend/internal/e2e/copy_edit_test.go @@ -0,0 +1,164 @@ +package e2e + +import ( + "context" + "io/fs" + "path/filepath" + "testing" + "time" + + assets "galo" + "galo/internal/albums" + "galo/internal/photos" +) + +// A copy of an edited photo must not inherit the edited version: the edit +// belongs to the photo whose edits produced it. +func TestCopyDoesNotShareEditedVersion(t *testing.T) { + ctx := context.Background() + st, closeDB := openStack(t, filepath.Join(t.TempDir(), "galo.db"), t.TempDir()) + defer closeDB() + + user, _, err := st.auth.Register(ctx, "copy@test.local", "", "password123", "C") + if err != nil { + t.Fatalf("register: %v", err) + } + album, err := st.albums.Create(ctx, user.ID, albums.CreateInput{Title: "Source"}) + if err != nil { + t.Fatalf("create album: %v", err) + } + target, err := st.albums.Create(ctx, user.ID, albums.CreateInput{Title: "Target"}) + if err != nil { + t.Fatalf("create target album: %v", err) + } + + p, err := st.photos.Upload(ctx, photos.UploadInput{AlbumID: album.ID, UserID: user.ID, Filename: "a.png", Data: makePNG(t)}) + if err != nil { + t.Fatalf("upload: %v", err) + } + + w := photos.NewWorker(st.photos, time.Second, 10, time.Minute, 2) + w.ProcessOnce(ctx) + + if _, err := st.photos.EditPhoto(ctx, p.ID, user.ID, photos.EditParams{Light: photos.LightParams{Exposure: 0.5}}); err != nil { + t.Fatalf("edit photo: %v", err) + } + w.ProcessOnce(ctx) + + source, err := st.photos.Get(ctx, p.ID, user.ID) + if err != nil { + t.Fatalf("get source: %v", err) + } + if source.EditThumbURL == "" || source.EditPreviewURL == "" { + t.Fatalf("source has no edited version: %+v", source) + } + + clones, err := st.photos.CopyBatch(ctx, []string{p.ID}, user.ID, target.ID) + if err != nil || len(clones) != 1 { + t.Fatalf("copy: %v len=%d", err, len(clones)) + } + clone := clones[0] + + // The copy shares the base variants with the source. + if clone.ThumbURL != source.ThumbURL || clone.PreviewURL != source.PreviewURL { + t.Fatalf("clone does not share base variants: src=%+v clone=%+v", source, clone) + } + // But it shows the original picture and carries no edit parameters. + if clone.EditURL != "" || clone.EditThumbURL != "" || clone.EditMediumURL != "" || clone.EditPreviewURL != "" { + t.Fatalf("clone shares the edited version: %+v", clone) + } + if len(clone.EditParams) != 0 { + t.Fatalf("clone inherited edit params: %+v", clone.EditParams) + } +} + +// The migration drops the edit bindings that copies carried before the fix, +// while the edited version of the photo that owns them stays untouched. +func TestMigrationDropsEditBindingsOfCopies(t *testing.T) { + ctx := context.Background() + st, closeDB := openStack(t, filepath.Join(t.TempDir(), "galo.db"), t.TempDir()) + defer closeDB() + + user, _, err := st.auth.Register(ctx, "mig@test.local", "", "password123", "M") + if err != nil { + t.Fatalf("register: %v", err) + } + album, err := st.albums.Create(ctx, user.ID, albums.CreateInput{Title: "Source"}) + if err != nil { + t.Fatalf("create album: %v", err) + } + target, err := st.albums.Create(ctx, user.ID, albums.CreateInput{Title: "Target"}) + if err != nil { + t.Fatalf("create target album: %v", err) + } + + p, err := st.photos.Upload(ctx, photos.UploadInput{AlbumID: album.ID, UserID: user.ID, Filename: "a.png", Data: makePNG(t)}) + if err != nil { + t.Fatalf("upload: %v", err) + } + w := photos.NewWorker(st.photos, time.Second, 10, time.Minute, 2) + w.ProcessOnce(ctx) + if _, err := st.photos.EditPhoto(ctx, p.ID, user.ID, photos.EditParams{Light: photos.LightParams{Exposure: 0.5}}); err != nil { + t.Fatalf("edit photo: %v", err) + } + w.ProcessOnce(ctx) + + var editObjectID string + if err := st.db.QueryRowContext(ctx, + `SELECT object_id FROM photo_objects WHERE photo_id = $1 AND role = 'edit_thumb'`, p.ID).Scan(&editObjectID); err != nil { + t.Fatalf("edited thumb object: %v", err) + } + + clones, err := st.photos.CopyBatch(ctx, []string{p.ID}, user.ID, target.ID) + if err != nil || len(clones) != 1 { + t.Fatalf("copy: %v len=%d", err, len(clones)) + } + clone := clones[0] + + // Reproduce the state left by the old code: the copy carries the edited + // version of the source photo. + if _, err := st.db.ExecContext(ctx, + `INSERT INTO photo_objects (photo_id, object_id, role) VALUES ($1, $2, 'edit_thumb')`, + clone.ID, editObjectID); err != nil { + t.Fatalf("bind edited version to the copy: %v", err) + } + + migFS, err := fs.Sub(assets.Migrations, "migrations/sqlite") + if err != nil { + t.Fatalf("migrations fs: %v", err) + } + up, err := fs.ReadFile(migFS, "000008_photo_edit_copy_bindings.up.sql") + if err != nil { + t.Fatalf("read migration: %v", err) + } + if _, err := st.db.ExecContext(ctx, string(up)); err != nil { + t.Fatalf("apply migration: %v", err) + } + + var n int + if err := st.db.QueryRowContext(ctx, + `SELECT count(*) FROM photo_objects WHERE photo_id = $1 AND role LIKE 'edit%'`, clone.ID).Scan(&n); err != nil { + t.Fatalf("count copy edit bindings: %v", err) + } + if n != 0 { + t.Fatalf("copy still carries %d edit bindings", n) + } + // The photo that owns the edited version keeps it. + if err := st.db.QueryRowContext(ctx, + `SELECT count(*) FROM photo_objects WHERE photo_id = $1 AND role = 'edit_thumb'`, p.ID).Scan(&n); err != nil { + t.Fatalf("count source edit bindings: %v", err) + } + if n != 1 { + t.Fatalf("source edit bindings = %d, want 1", n) + } + // A bound object is never garbage, so the edited version stays alive. + claimed, err := st.repos.Photos.ClaimGarbage(ctx, 10) + if err != nil { + t.Fatalf("claim garbage: %v", err) + } + for _, o := range claimed { + if o.ID == editObjectID { + t.Fatalf("edited version of the owner was claimed as garbage") + } + } +} diff --git a/backend/internal/photos/copy_test.go b/backend/internal/photos/copy_test.go index e060616..95eec51 100644 --- a/backend/internal/photos/copy_test.go +++ b/backend/internal/photos/copy_test.go @@ -74,6 +74,60 @@ func TestCopySharesStorageObjects(t *testing.T) { } } +func TestCopyDoesNotShareEditedVersion(t *testing.T) { + pool := testPool(t) + ctx := context.Background() + svc, cl, _ := newRecordingServiceFS(t, pool) + user := createUser(t, ctx, pool) + albumA := createAlbum(t, ctx, pool, user) + albumB := createAlbum(t, ctx, pool, user) + + p, err := svc.Upload(ctx, UploadInput{ + AlbumID: albumA, UserID: user, Filename: "p.png", MimeType: "image/png", Data: makePNG(t), + }) + if err != nil { + t.Fatalf("upload: %v", err) + } + t.Cleanup(func() { cleanupPhoto(t, ctx, pool, cl, p.ID) }) + + w := NewWorker(svc, time.Second, 10, time.Minute, 2) + w.processVariantJobs(ctx) + + // The edit belongs to the photo: the parameters live in its metadata and + // the background render binds the edited version to it. + if _, err := svc.EditPhoto(ctx, p.ID, user, EditParams{Light: LightParams{Exposure: 0.5}}); err != nil { + t.Fatalf("edit photo: %v", err) + } + w.processEditJobs(ctx) + + src, err := svc.store.GetByID(ctx, p.ID, "") + if err != nil { + t.Fatalf("get source: %v", err) + } + if src.EditThumbKey == "" || src.EditPreviewKey == "" { + t.Fatalf("source has no edited version: %+v", src) + } + + clones, err := svc.CopyBatch(ctx, []string{p.ID}, user, albumB) + if err != nil || len(clones) != 1 { + t.Fatalf("copy: %v len=%d", err, len(clones)) + } + clone := clones[0] + t.Cleanup(func() { cleanupPhoto(t, ctx, pool, cl, clone.ID) }) + + // The copy shares the base variants with the source. + if clone.ThumbKey != src.ThumbKey || clone.MediumKey != src.MediumKey || clone.PreviewKey != src.PreviewKey { + t.Fatalf("clone does not share base variants: src=%+v clone=%+v", src, clone) + } + // But it never inherits the edited version or its parameters. + if clone.EditKey != "" || clone.EditThumbKey != "" || clone.EditMediumKey != "" || clone.EditPreviewKey != "" { + t.Fatalf("clone shares the edited version: %+v", clone) + } + if len(clone.EditParams) != 0 { + t.Fatalf("clone inherited edit params: %+v", clone.EditParams) + } +} + func sameSet(a, b []string) bool { if len(a) != len(b) { return false diff --git a/backend/internal/photos/edit_render.go b/backend/internal/photos/edit_render.go index 3769ce2..5849c0f 100644 --- a/backend/internal/photos/edit_render.go +++ b/backend/internal/photos/edit_render.go @@ -19,6 +19,22 @@ const ( RoleEditPreview = "edit_preview" ) +// EditRoles lists the roles of the edited version. The edited version belongs +// to the photo whose edits produced it, so copies of that photo never share +// these roles. +func EditRoles() []string { + return []string{RoleEdit, RoleEditThumb, RoleEditMedium, RoleEditPreview} +} + +// IsEditRole reports whether the role belongs to the edited version. +func IsEditRole(role string) bool { + switch role { + case RoleEdit, RoleEditThumb, RoleEditMedium, RoleEditPreview: + return true + } + return false +} + // applyLight renders the light adjustments into a copy of the image. The // original pixels are never modified in place. func applyLight(src image.Image, p LightParams) *image.NRGBA { diff --git a/backend/internal/photos/edit_service.go b/backend/internal/photos/edit_service.go index 8d58247..67d8c91 100644 --- a/backend/internal/photos/edit_service.go +++ b/backend/internal/photos/edit_service.go @@ -74,8 +74,7 @@ func (s *Service) ResetEdit(ctx context.Context, photoID, userID string) (*model if err := s.store.DeleteEditJob(ctx, photoID); err != nil { return nil, err } - if err := s.store.DeleteObjectBindings(ctx, photoID, - RoleEdit, RoleEditThumb, RoleEditMedium, RoleEditPreview); err != nil { + if err := s.store.DeleteObjectBindings(ctx, photoID, EditRoles()...); err != nil { return nil, err } return s.publishEdit(ctx, photoID, userID) diff --git a/backend/internal/photos/edit_test.go b/backend/internal/photos/edit_test.go index b072d55..9134567 100644 --- a/backend/internal/photos/edit_test.go +++ b/backend/internal/photos/edit_test.go @@ -34,6 +34,23 @@ func TestParseEditParamsClampsValues(t *testing.T) { } } +func TestEditRolesCoverTheEditedVersion(t *testing.T) { + roles := EditRoles() + if len(roles) != 4 { + t.Fatalf("edit roles = %v, want four", roles) + } + for _, role := range roles { + if !IsEditRole(role) { + t.Fatalf("role %q must be an edit role", role) + } + } + for _, role := range []string{RoleOriginal, RoleThumb, RoleMedium, RolePreview, RolePlayback} { + if IsEditRole(role) { + t.Fatalf("role %q must not be an edit role", role) + } + } +} + func TestEditParamsIsEmpty(t *testing.T) { var zero EditParams if !zero.IsEmpty() { diff --git a/backend/internal/photos/sqlite_store.go b/backend/internal/photos/sqlite_store.go index 62e1f79..8492bc0 100644 --- a/backend/internal/photos/sqlite_store.go +++ b/backend/internal/photos/sqlite_store.go @@ -250,10 +250,18 @@ func (s *SQLiteStore) Clone(ctx context.Context, src *models.Photo, albumID, upl mediaKind(src.Kind), src.DurationMS); err != nil { return err } + // The edited version belongs to the photo whose edits produced it, so the + // copy shares the base variants only. + roles := EditRoles() + args := []any{id, src.ID} + for _, role := range roles { + args = append(args, role) + } if _, err := tx.ExecContext(ctx, `INSERT INTO photo_objects (photo_id, object_id, role) - SELECT $1, object_id, role FROM photo_objects WHERE photo_id = $2`, - id, src.ID); err != nil { + SELECT $1, object_id, role FROM photo_objects + WHERE photo_id = $2 AND role NOT IN (`+sqliteutil.Placeholders(3, len(roles))+`)`, + args...); err != nil { return err } return tx.Commit() diff --git a/backend/internal/photos/store.go b/backend/internal/photos/store.go index df3e91d..763daf7 100644 --- a/backend/internal/photos/store.go +++ b/backend/internal/photos/store.go @@ -290,10 +290,13 @@ func (s *Store) Clone(ctx context.Context, src *models.Photo, albumID, uploaderI mediaKind(src.Kind), src.DurationMS); err != nil { return err } + // The edited version belongs to the photo whose edits produced it, so the + // copy shares the base variants only. if _, err := tx.Exec(ctx, `INSERT INTO photo_objects (photo_id, object_id, role) - SELECT $1, object_id, role FROM photo_objects WHERE photo_id = $2`, - id, src.ID); err != nil { + SELECT $1, object_id, role FROM photo_objects + WHERE photo_id = $2 AND role <> ALL($3::text[])`, + id, src.ID, EditRoles()); err != nil { return err } return tx.Commit(ctx) diff --git a/backend/internal/repo/contract/contract.go b/backend/internal/repo/contract/contract.go index 6d4d5f3..d08133a 100644 --- a/backend/internal/repo/contract/contract.go +++ b/backend/internal/repo/contract/contract.go @@ -49,6 +49,7 @@ func Run(t *testing.T, r Repos) { {"Social", testSocial}, {"Photos", testPhotos}, {"Objects", testObjects}, + {"Clone", testClone}, {"UserAvatar", testUserAvatar}, {"VariantJobKinds", testVariantJobKinds}, {"Settings", testSettings}, @@ -709,6 +710,56 @@ func testObjects(t *testing.T, ctx context.Context, r Repos) { } } +func testClone(t *testing.T, ctx context.Context, r Repos) { + owner := seedUser(t, ctx, r, "Owner") + a := seedAlbum(t, ctx, r, owner.ID) + target := seedAlbum(t, ctx, r, owner.ID) + p := seedPhoto(t, ctx, r, owner.ID, a.ID) + + // The photo carries the base variants and an edited version. + thumb := &photos.Object{ID: newID(), StorageKey: "objects/" + newID(), MimeType: "image/webp", State: photos.ObjectPending} + if err := r.Photos.CreateObject(ctx, thumb); err != nil { + t.Fatalf("create thumb: %v", err) + } + if err := r.Photos.ActivateObject(ctx, thumb.ID); err != nil { + t.Fatalf("activate thumb: %v", err) + } + if err := r.Photos.BindObject(ctx, p.ID, thumb.ID, photos.RoleThumb); err != nil { + t.Fatalf("bind thumb: %v", err) + } + edited := &photos.Object{ID: newID(), StorageKey: "objects/" + newID(), MimeType: "image/webp", State: photos.ObjectPending} + if err := r.Photos.CreateObject(ctx, edited); err != nil { + t.Fatalf("create edited object: %v", err) + } + if err := r.Photos.ActivateObject(ctx, edited.ID); err != nil { + t.Fatalf("activate edited object: %v", err) + } + if err := r.Photos.BindObject(ctx, p.ID, edited.ID, photos.RoleEdit); err != nil { + t.Fatalf("bind edited version: %v", err) + } + + cloneID := newID() + if err := r.Photos.Clone(ctx, p, target.ID, owner.ID, cloneID); err != nil { + t.Fatalf("clone: %v", err) + } + + cloneObjects, err := r.Photos.PhotoObjectIDs(ctx, cloneID) + if err != nil { + t.Fatalf("clone objects: %v", err) + } + // The copy shares the base variants. + if !contains(cloneObjects, thumb.ID) { + t.Fatalf("clone objects = %+v, want thumb %s", cloneObjects, thumb.ID) + } + // The edited version belongs to the photo whose edits produced it. + if contains(cloneObjects, edited.ID) { + t.Fatalf("clone shares the edited version: %+v", cloneObjects) + } + if refs, _ := r.Photos.ObjectRefCount(ctx, edited.ID); refs != 1 { + t.Fatalf("edited object refs = %d, want 1", refs) + } +} + func testUserAvatar(t *testing.T, ctx context.Context, r Repos) { owner := seedUser(t, ctx, r, "Owner") diff --git a/backend/migrations/000016_photo_edit_copy_bindings.down.sql b/backend/migrations/000016_photo_edit_copy_bindings.down.sql new file mode 100644 index 0000000..2938537 --- /dev/null +++ b/backend/migrations/000016_photo_edit_copy_bindings.down.sql @@ -0,0 +1,3 @@ +-- The removed bindings cannot be restored: which object belonged to which copy +-- is not recorded anywhere. Rollback of the code is enough - the edited +-- versions of the photos that own them stay untouched. diff --git a/backend/migrations/000016_photo_edit_copy_bindings.up.sql b/backend/migrations/000016_photo_edit_copy_bindings.up.sql new file mode 100644 index 0000000..423e843 --- /dev/null +++ b/backend/migrations/000016_photo_edit_copy_bindings.up.sql @@ -0,0 +1,10 @@ +-- The edited version belongs to the photo whose edits produced it. Copies made +-- before the fix still carried the edit bindings, so a photo without edit +-- parameters showed the edited picture. Drop those bindings: the objects +-- become collectable garbage and the copies show the original picture again. + +DELETE FROM photo_objects +WHERE role IN ('edit', 'edit_thumb', 'edit_medium', 'edit_preview') + AND photo_id IN ( + SELECT id FROM photos WHERE edit_params IS NULL OR edit_params = '{}'::jsonb + ); diff --git a/backend/migrations/sqlite/000008_photo_edit_copy_bindings.down.sql b/backend/migrations/sqlite/000008_photo_edit_copy_bindings.down.sql new file mode 100644 index 0000000..2938537 --- /dev/null +++ b/backend/migrations/sqlite/000008_photo_edit_copy_bindings.down.sql @@ -0,0 +1,3 @@ +-- The removed bindings cannot be restored: which object belonged to which copy +-- is not recorded anywhere. Rollback of the code is enough - the edited +-- versions of the photos that own them stay untouched. diff --git a/backend/migrations/sqlite/000008_photo_edit_copy_bindings.up.sql b/backend/migrations/sqlite/000008_photo_edit_copy_bindings.up.sql new file mode 100644 index 0000000..c13f151 --- /dev/null +++ b/backend/migrations/sqlite/000008_photo_edit_copy_bindings.up.sql @@ -0,0 +1,10 @@ +-- The edited version belongs to the photo whose edits produced it. Copies made +-- before the fix still carried the edit bindings, so a photo without edit +-- parameters showed the edited picture. Drop those bindings: the objects +-- become collectable garbage and the copies show the original picture again. + +DELETE FROM photo_objects +WHERE role IN ('edit', 'edit_thumb', 'edit_medium', 'edit_preview') + AND photo_id IN ( + SELECT id FROM photos WHERE edit_params IS NULL OR edit_params = '{}' + );