diff --git a/pkg/gallery/scan.go b/pkg/gallery/scan.go index 2064355cd..b3e5d2c3c 100644 --- a/pkg/gallery/scan.go +++ b/pkg/gallery/scan.go @@ -135,13 +135,14 @@ func (h *ScanHandler) associateExisting(ctx context.Context, existing []*models. if err := h.CreatorUpdater.AddFileID(ctx, i.ID, f.Base().ID); err != nil { return fmt.Errorf("adding file to gallery: %w", err) } - // update updated_at time - if _, err := h.CreatorUpdater.UpdatePartial(ctx, i.ID, models.NewGalleryPartial()); err != nil { - return fmt.Errorf("updating gallery: %w", err) - } } if !found || updateExisting { + // update updated_at time when file association or content changes + if _, err := h.CreatorUpdater.UpdatePartial(ctx, i.ID, models.NewGalleryPartial()); err != nil { + return fmt.Errorf("updating gallery: %w", err) + } + h.PluginCache.RegisterPostHooks(ctx, i.ID, hook.GalleryUpdatePost, nil, nil) } } diff --git a/pkg/gallery/scan_test.go b/pkg/gallery/scan_test.go new file mode 100644 index 000000000..4a89206e3 --- /dev/null +++ b/pkg/gallery/scan_test.go @@ -0,0 +1,108 @@ +package gallery + +import ( + "context" + "testing" + + "github.com/stashapp/stash/pkg/models" + "github.com/stashapp/stash/pkg/models/mocks" + "github.com/stashapp/stash/pkg/plugin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func TestAssociateExisting_UpdatePartialOnContentChange(t *testing.T) { + const ( + testGalleryID = 1 + testFileID = 100 + ) + + existingFile := &models.BaseFile{ID: models.FileID(testFileID), Path: "test.zip"} + + makeGallery := func() *models.Gallery { + return &models.Gallery{ + ID: testGalleryID, + Files: models.NewRelatedFiles([]models.File{existingFile}), + } + } + + tests := []struct { + name string + updateExisting bool + expectUpdate bool + }{ + { + name: "calls UpdatePartial when file content changed", + updateExisting: true, + expectUpdate: true, + }, + { + name: "skips UpdatePartial when file unchanged and already associated", + updateExisting: false, + expectUpdate: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + db := mocks.NewDatabase() + db.Gallery.On("GetFiles", mock.Anything, testGalleryID).Return([]models.File{existingFile}, nil) + + if tt.expectUpdate { + db.Gallery.On("UpdatePartial", mock.Anything, testGalleryID, mock.Anything). + Return(&models.Gallery{ID: testGalleryID}, nil) + } + + h := &ScanHandler{ + CreatorUpdater: db.Gallery, + PluginCache: &plugin.Cache{}, + } + + db.WithTxnCtx(func(ctx context.Context) { + err := h.associateExisting(ctx, []*models.Gallery{makeGallery()}, existingFile, tt.updateExisting) + assert.NoError(t, err) + }) + + if tt.expectUpdate { + db.Gallery.AssertCalled(t, "UpdatePartial", mock.Anything, testGalleryID, mock.Anything) + } else { + db.Gallery.AssertNotCalled(t, "UpdatePartial", mock.Anything, mock.Anything, mock.Anything) + } + }) + } +} + +func TestAssociateExisting_UpdatePartialOnNewFile(t *testing.T) { + const ( + testGalleryID = 1 + existFileID = 100 + newFileID = 200 + ) + + existingFile := &models.BaseFile{ID: models.FileID(existFileID), Path: "existing.zip"} + newFile := &models.BaseFile{ID: models.FileID(newFileID), Path: "new.zip"} + + gallery := &models.Gallery{ + ID: testGalleryID, + Files: models.NewRelatedFiles([]models.File{existingFile}), + } + + db := mocks.NewDatabase() + db.Gallery.On("GetFiles", mock.Anything, testGalleryID).Return([]models.File{existingFile}, nil) + db.Gallery.On("AddFileID", mock.Anything, testGalleryID, models.FileID(newFileID)).Return(nil) + db.Gallery.On("UpdatePartial", mock.Anything, testGalleryID, mock.Anything). + Return(&models.Gallery{ID: testGalleryID}, nil) + + h := &ScanHandler{ + CreatorUpdater: db.Gallery, + PluginCache: &plugin.Cache{}, + } + + db.WithTxnCtx(func(ctx context.Context) { + err := h.associateExisting(ctx, []*models.Gallery{gallery}, newFile, false) + assert.NoError(t, err) + }) + + db.Gallery.AssertCalled(t, "AddFileID", mock.Anything, testGalleryID, models.FileID(newFileID)) + db.Gallery.AssertCalled(t, "UpdatePartial", mock.Anything, testGalleryID, mock.Anything) +} diff --git a/pkg/image/scan.go b/pkg/image/scan.go index 317e3605f..99b31f698 100644 --- a/pkg/image/scan.go +++ b/pkg/image/scan.go @@ -210,8 +210,8 @@ func (h *ScanHandler) associateExisting(ctx context.Context, existing []*models. changed = true } - if changed { - // always update updated_at time + if changed || updateExisting { + // update updated_at time when file association or content changes imagePartial := models.NewImagePartial() imagePartial.GalleryIDs = galleryIDs @@ -229,9 +229,7 @@ func (h *ScanHandler) associateExisting(ctx context.Context, existing []*models. return fmt.Errorf("updating gallery updated at timestamp: %w", err) } } - } - if changed || updateExisting { h.PluginCache.RegisterPostHooks(ctx, i.ID, hook.ImageUpdatePost, nil, nil) } } diff --git a/pkg/image/scan_test.go b/pkg/image/scan_test.go new file mode 100644 index 000000000..f48c188ee --- /dev/null +++ b/pkg/image/scan_test.go @@ -0,0 +1,120 @@ +package image + +import ( + "context" + "testing" + + "github.com/stashapp/stash/pkg/models" + "github.com/stashapp/stash/pkg/models/mocks" + "github.com/stashapp/stash/pkg/plugin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +type mockScanConfig struct{} + +func (m *mockScanConfig) GetCreateGalleriesFromFolders() bool { return false } + +func TestAssociateExisting_UpdatePartialOnContentChange(t *testing.T) { + const ( + testImageID = 1 + testFileID = 100 + ) + + existingFile := &models.BaseFile{ID: models.FileID(testFileID), Path: "/images/test.jpg"} + + makeImage := func() *models.Image { + return &models.Image{ + ID: testImageID, + Files: models.NewRelatedFiles([]models.File{existingFile}), + GalleryIDs: models.NewRelatedIDs([]int{}), + } + } + + tests := []struct { + name string + updateExisting bool + expectUpdate bool + }{ + { + name: "calls UpdatePartial when file content changed", + updateExisting: true, + expectUpdate: true, + }, + { + name: "skips UpdatePartial when file unchanged and already associated", + updateExisting: false, + expectUpdate: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + db := mocks.NewDatabase() + db.Image.On("GetFiles", mock.Anything, testImageID).Return([]models.File{existingFile}, nil) + db.Image.On("GetGalleryIDs", mock.Anything, testImageID).Return([]int{}, nil) + + if tt.expectUpdate { + db.Image.On("UpdatePartial", mock.Anything, testImageID, mock.Anything). + Return(&models.Image{ID: testImageID}, nil) + } + + h := &ScanHandler{ + CreatorUpdater: db.Image, + GalleryFinder: db.Gallery, + ScanConfig: &mockScanConfig{}, + PluginCache: &plugin.Cache{}, + } + + db.WithTxnCtx(func(ctx context.Context) { + err := h.associateExisting(ctx, []*models.Image{makeImage()}, existingFile, tt.updateExisting) + assert.NoError(t, err) + }) + + if tt.expectUpdate { + db.Image.AssertCalled(t, "UpdatePartial", mock.Anything, testImageID, mock.Anything) + } else { + db.Image.AssertNotCalled(t, "UpdatePartial", mock.Anything, mock.Anything, mock.Anything) + } + }) + } +} + +func TestAssociateExisting_UpdatePartialOnNewFile(t *testing.T) { + const ( + testImageID = 1 + existFileID = 100 + newFileID = 200 + ) + + existingFile := &models.BaseFile{ID: models.FileID(existFileID), Path: "/images/existing.jpg"} + newFile := &models.BaseFile{ID: models.FileID(newFileID), Path: "/images/new.jpg"} + + image := &models.Image{ + ID: testImageID, + Files: models.NewRelatedFiles([]models.File{existingFile}), + GalleryIDs: models.NewRelatedIDs([]int{}), + } + + db := mocks.NewDatabase() + db.Image.On("GetFiles", mock.Anything, testImageID).Return([]models.File{existingFile}, nil) + db.Image.On("GetGalleryIDs", mock.Anything, testImageID).Return([]int{}, nil) + db.Image.On("AddFileID", mock.Anything, testImageID, models.FileID(newFileID)).Return(nil) + db.Image.On("UpdatePartial", mock.Anything, testImageID, mock.Anything). + Return(&models.Image{ID: testImageID}, nil) + + h := &ScanHandler{ + CreatorUpdater: db.Image, + GalleryFinder: db.Gallery, + ScanConfig: &mockScanConfig{}, + PluginCache: &plugin.Cache{}, + } + + db.WithTxnCtx(func(ctx context.Context) { + err := h.associateExisting(ctx, []*models.Image{image}, newFile, false) + assert.NoError(t, err) + }) + + db.Image.AssertCalled(t, "AddFileID", mock.Anything, testImageID, models.FileID(newFileID)) + db.Image.AssertCalled(t, "UpdatePartial", mock.Anything, testImageID, mock.Anything) +} diff --git a/pkg/models/mocks/database.go b/pkg/models/mocks/database.go index ec4177b30..88f106e19 100644 --- a/pkg/models/mocks/database.go +++ b/pkg/models/mocks/database.go @@ -3,6 +3,7 @@ package mocks import ( "context" + "errors" "github.com/stashapp/stash/pkg/models" "github.com/stashapp/stash/pkg/txn" @@ -89,6 +90,16 @@ func (db *Database) AssertExpectations(t mock.TestingT) { db.SavedFilter.AssertExpectations(t) } +// WithTxnCtx runs fn with a context that has a transaction hook manager registered, +// so code that calls txn.AddPostCommitHook (e.g. plugin cache) won't nil-panic. +// Always rolls back to avoid executing the registered hooks. +func (db *Database) WithTxnCtx(fn func(ctx context.Context)) { + _ = txn.WithTxn(context.Background(), db, func(ctx context.Context) error { + fn(ctx) + return errors.New("rollback") + }) +} + func (db *Database) Repository() models.Repository { return models.Repository{ TxnManager: db, diff --git a/pkg/scene/scan.go b/pkg/scene/scan.go index e1038fbc3..c70c44a9e 100644 --- a/pkg/scene/scan.go +++ b/pkg/scene/scan.go @@ -160,15 +160,15 @@ func (h *ScanHandler) associateExisting(ctx context.Context, existing []*models. if err := h.CreatorUpdater.AddFileID(ctx, s.ID, f.ID); err != nil { return fmt.Errorf("adding file to scene: %w", err) } + } - // update updated_at time + if !found || updateExisting { + // update updated_at time when file association or content changes scenePartial := models.NewScenePartial() if _, err := h.CreatorUpdater.UpdatePartial(ctx, s.ID, scenePartial); err != nil { return fmt.Errorf("updating scene: %w", err) } - } - if !found || updateExisting { h.PluginCache.RegisterPostHooks(ctx, s.ID, hook.SceneUpdatePost, nil, nil) } } diff --git a/pkg/scene/scan_test.go b/pkg/scene/scan_test.go new file mode 100644 index 000000000..71729bb57 --- /dev/null +++ b/pkg/scene/scan_test.go @@ -0,0 +1,114 @@ +package scene + +import ( + "context" + "testing" + + "github.com/stashapp/stash/pkg/models" + "github.com/stashapp/stash/pkg/models/mocks" + "github.com/stashapp/stash/pkg/plugin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func TestAssociateExisting_UpdatePartialOnContentChange(t *testing.T) { + const ( + testSceneID = 1 + testFileID = 100 + ) + + existingFile := &models.VideoFile{ + BaseFile: &models.BaseFile{ID: models.FileID(testFileID), Path: "test.mp4"}, + } + + makeScene := func() *models.Scene { + return &models.Scene{ + ID: testSceneID, + Files: models.NewRelatedVideoFiles([]*models.VideoFile{existingFile}), + } + } + + tests := []struct { + name string + updateExisting bool + expectUpdate bool + }{ + { + name: "calls UpdatePartial when file content changed", + updateExisting: true, + expectUpdate: true, + }, + { + name: "skips UpdatePartial when file unchanged and already associated", + updateExisting: false, + expectUpdate: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + db := mocks.NewDatabase() + db.Scene.On("GetFiles", mock.Anything, testSceneID).Return([]*models.VideoFile{existingFile}, nil) + + if tt.expectUpdate { + db.Scene.On("UpdatePartial", mock.Anything, testSceneID, mock.Anything). + Return(&models.Scene{ID: testSceneID}, nil) + } + + h := &ScanHandler{ + CreatorUpdater: db.Scene, + PluginCache: &plugin.Cache{}, + } + + db.WithTxnCtx(func(ctx context.Context) { + err := h.associateExisting(ctx, []*models.Scene{makeScene()}, existingFile, tt.updateExisting) + assert.NoError(t, err) + }) + + if tt.expectUpdate { + db.Scene.AssertCalled(t, "UpdatePartial", mock.Anything, testSceneID, mock.Anything) + } else { + db.Scene.AssertNotCalled(t, "UpdatePartial", mock.Anything, mock.Anything, mock.Anything) + } + }) + } +} + +func TestAssociateExisting_UpdatePartialOnNewFile(t *testing.T) { + const ( + testSceneID = 1 + existFileID = 100 + newFileID = 200 + ) + + existingFile := &models.VideoFile{ + BaseFile: &models.BaseFile{ID: models.FileID(existFileID), Path: "existing.mp4"}, + } + newFile := &models.VideoFile{ + BaseFile: &models.BaseFile{ID: models.FileID(newFileID), Path: "new.mp4"}, + } + + scene := &models.Scene{ + ID: testSceneID, + Files: models.NewRelatedVideoFiles([]*models.VideoFile{existingFile}), + } + + db := mocks.NewDatabase() + db.Scene.On("GetFiles", mock.Anything, testSceneID).Return([]*models.VideoFile{existingFile}, nil) + db.Scene.On("AddFileID", mock.Anything, testSceneID, models.FileID(newFileID)).Return(nil) + db.Scene.On("UpdatePartial", mock.Anything, testSceneID, mock.Anything). + Return(&models.Scene{ID: testSceneID}, nil) + + h := &ScanHandler{ + CreatorUpdater: db.Scene, + PluginCache: &plugin.Cache{}, + } + + db.WithTxnCtx(func(ctx context.Context) { + err := h.associateExisting(ctx, []*models.Scene{scene}, newFile, false) + assert.NoError(t, err) + }) + + db.Scene.AssertCalled(t, "AddFileID", mock.Anything, testSceneID, models.FileID(newFileID)) + db.Scene.AssertCalled(t, "UpdatePartial", mock.Anything, testSceneID, mock.Anything) +}