From 046fd1c0bef14f38edead9d2e9bfbb0c980152fb Mon Sep 17 00:00:00 2001
From: WithoutPants <53250216+WithoutPants@users.noreply.github.com>
Date: Sun, 26 Mar 2023 10:56:32 +1100
Subject: [PATCH] Blob fixes (#3599)

* Fix error if movie back image blob was not found
* Don't error out if scene cover get fails
* Don't error out on image get fails
* Add debug logging for fs blobs
* Remove old blob data when no longer referenced
---
 internal/api/resolver_model_movie.go        |  6 +--
 internal/api/resolver_mutation_stash_box.go |  3 +-
 internal/identify/scene.go                  |  3 +-
 internal/identify/scene_test.go             |  4 +-
 internal/manager/running_streams.go         |  5 +--
 pkg/models/mocks/MovieReaderWriter.go       | 21 +++++++++++
 pkg/models/movie.go                         |  1 +
 pkg/movie/export.go                         |  5 ++-
 pkg/movie/export_test.go                    | 10 +++--
 pkg/performer/export.go                     |  3 +-
 pkg/performer/export_test.go                |  5 ++-
 pkg/scene/export.go                         |  3 +-
 pkg/scene/export_test.go                    |  5 ++-
 pkg/sqlite/blob.go                          | 42 ++++++++++++++++++---
 pkg/sqlite/blob/fs.go                       |  2 +
 pkg/sqlite/movies.go                        |  4 ++
 pkg/studio/export.go                        |  3 +-
 pkg/studio/export_test.go                   |  6 ++-
 pkg/tag/export.go                           |  3 +-
 pkg/tag/export_test.go                      |  6 ++-
 20 files changed, 107 insertions(+), 33 deletions(-)

diff --git a/internal/api/resolver_model_movie.go b/internal/api/resolver_model_movie.go
index fbde8a80a..9967ef323 100644
--- a/internal/api/resolver_model_movie.go
+++ b/internal/api/resolver_model_movie.go
@@ -93,10 +93,10 @@ func (r *movieResolver) FrontImagePath(ctx context.Context, obj *models.Movie) (
 
 func (r *movieResolver) BackImagePath(ctx context.Context, obj *models.Movie) (*string, error) {
 	// don't return any thing if there is no back image
-	var img []byte
+	hasImage := false
 	if err := r.withReadTxn(ctx, func(ctx context.Context) error {
 		var err error
-		img, err = r.repository.Movie.GetBackImage(ctx, obj.ID)
+		hasImage, err = r.repository.Movie.HasBackImage(ctx, obj.ID)
 		if err != nil {
 			return err
 		}
@@ -106,7 +106,7 @@ func (r *movieResolver) BackImagePath(ctx context.Context, obj *models.Movie) (*
 		return nil, err
 	}
 
-	if img == nil {
+	if !hasImage {
 		return nil, nil
 	}
 
diff --git a/internal/api/resolver_mutation_stash_box.go b/internal/api/resolver_mutation_stash_box.go
index 9abb7179c..92e0923e7 100644
--- a/internal/api/resolver_mutation_stash_box.go
+++ b/internal/api/resolver_mutation_stash_box.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/stashapp/stash/internal/manager"
 	"github.com/stashapp/stash/internal/manager/config"
+	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/scraper/stashbox"
 )
 
@@ -64,7 +65,7 @@ func (r *mutationResolver) SubmitStashBoxSceneDraft(ctx context.Context, input S
 
 		cover, err := qb.GetCover(ctx, id)
 		if err != nil {
-			return fmt.Errorf("getting scene cover: %w", err)
+			logger.Errorf("Error getting scene cover: %v", err)
 		}
 
 		res, err = client.SubmitSceneDraft(ctx, scene, boxes[input.StashBoxIndex].Endpoint, cover)
diff --git a/internal/identify/scene.go b/internal/identify/scene.go
index d74b47d12..a952cb73b 100644
--- a/internal/identify/scene.go
+++ b/internal/identify/scene.go
@@ -8,6 +8,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/scene"
 	"github.com/stashapp/stash/pkg/sliceutil"
@@ -234,7 +235,7 @@ func (g sceneRelationships) cover(ctx context.Context) ([]byte, error) {
 	// always overwrite if present
 	existingCover, err := g.sceneReader.GetCover(ctx, g.scene.ID)
 	if err != nil {
-		return nil, fmt.Errorf("error getting scene cover: %w", err)
+		logger.Errorf("Error getting scene cover: %v", err)
 	}
 
 	data, err := utils.ProcessImageInput(ctx, *scraped)
diff --git a/internal/identify/scene_test.go b/internal/identify/scene_test.go
index 511023680..5e8091e6f 100644
--- a/internal/identify/scene_test.go
+++ b/internal/identify/scene_test.go
@@ -742,8 +742,8 @@ func Test_sceneRelationships_cover(t *testing.T) {
 			"error getting scene cover",
 			errSceneID,
 			&newDataEncoded,
-			nil,
-			true,
+			newData,
+			false,
 		},
 		{
 			"invalid data",
diff --git a/internal/manager/running_streams.go b/internal/manager/running_streams.go
index 5c95743b7..8fa397640 100644
--- a/internal/manager/running_streams.go
+++ b/internal/manager/running_streams.go
@@ -56,9 +56,8 @@ func (s *SceneServer) ServeScreenshot(scene *models.Scene, w http.ResponseWriter
 
 	var cover []byte
 	readTxnErr := txn.WithReadTxn(r.Context(), s.TxnManager, func(ctx context.Context) error {
-		var err error
-		cover, err = s.SceneCoverGetter.GetCover(ctx, scene.ID)
-		return err
+		cover, _ = s.SceneCoverGetter.GetCover(ctx, scene.ID)
+		return nil
 	})
 	if errors.Is(readTxnErr, context.Canceled) {
 		return
diff --git a/pkg/models/mocks/MovieReaderWriter.go b/pkg/models/mocks/MovieReaderWriter.go
index ad2171a66..3131d31d6 100644
--- a/pkg/models/mocks/MovieReaderWriter.go
+++ b/pkg/models/mocks/MovieReaderWriter.go
@@ -321,6 +321,27 @@ func (_m *MovieReaderWriter) GetFrontImage(ctx context.Context, movieID int) ([]
 	return r0, r1
 }
 
+// HasBackImage provides a mock function with given fields: ctx, movieID
+func (_m *MovieReaderWriter) HasBackImage(ctx context.Context, movieID int) (bool, error) {
+	ret := _m.Called(ctx, movieID)
+
+	var r0 bool
+	if rf, ok := ret.Get(0).(func(context.Context, int) bool); ok {
+		r0 = rf(ctx, movieID)
+	} else {
+		r0 = ret.Get(0).(bool)
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(context.Context, int) error); ok {
+		r1 = rf(ctx, movieID)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // Query provides a mock function with given fields: ctx, movieFilter, findFilter
 func (_m *MovieReaderWriter) Query(ctx context.Context, movieFilter *models.MovieFilterType, findFilter *models.FindFilterType) ([]*models.Movie, int, error) {
 	ret := _m.Called(ctx, movieFilter, findFilter)
diff --git a/pkg/models/movie.go b/pkg/models/movie.go
index adaa4fe03..aac1aa759 100644
--- a/pkg/models/movie.go
+++ b/pkg/models/movie.go
@@ -39,6 +39,7 @@ type MovieReader interface {
 	Query(ctx context.Context, movieFilter *MovieFilterType, findFilter *FindFilterType) ([]*Movie, int, error)
 	GetFrontImage(ctx context.Context, movieID int) ([]byte, error)
 	GetBackImage(ctx context.Context, movieID int) ([]byte, error)
+	HasBackImage(ctx context.Context, movieID int) (bool, error)
 	FindByPerformerID(ctx context.Context, performerID int) ([]*Movie, error)
 	CountByPerformerID(ctx context.Context, performerID int) (int, error)
 	FindByStudioID(ctx context.Context, studioID int) ([]*Movie, error)
diff --git a/pkg/movie/export.go b/pkg/movie/export.go
index 2af697a49..23851f42f 100644
--- a/pkg/movie/export.go
+++ b/pkg/movie/export.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/models/json"
 	"github.com/stashapp/stash/pkg/models/jsonschema"
@@ -64,7 +65,7 @@ func ToJSON(ctx context.Context, reader ImageGetter, studioReader studio.Finder,
 
 	frontImage, err := reader.GetFrontImage(ctx, movie.ID)
 	if err != nil {
-		return nil, fmt.Errorf("error getting movie front image: %v", err)
+		logger.Errorf("Error getting movie front image: %v", err)
 	}
 
 	if len(frontImage) > 0 {
@@ -73,7 +74,7 @@ func ToJSON(ctx context.Context, reader ImageGetter, studioReader studio.Finder,
 
 	backImage, err := reader.GetBackImage(ctx, movie.ID)
 	if err != nil {
-		return nil, fmt.Errorf("error getting movie back image: %v", err)
+		logger.Errorf("Error getting movie back image: %v", err)
 	}
 
 	if len(backImage) > 0 {
diff --git a/pkg/movie/export_test.go b/pkg/movie/export_test.go
index 007383902..898400127 100644
--- a/pkg/movie/export_test.go
+++ b/pkg/movie/export_test.go
@@ -161,13 +161,15 @@ func initTestTable() {
 		},
 		{
 			createFullMovie(errFrontImageID, studioID),
-			nil,
-			true,
+			createFullJSONMovie(studioName, "", backImage),
+			// failure to get front image should not cause error
+			false,
 		},
 		{
 			createFullMovie(errBackImageID, studioID),
-			nil,
-			true,
+			createFullJSONMovie(studioName, frontImage, ""),
+			// failure to get back image should not cause error
+			false,
 		},
 		{
 			createFullMovie(errStudioMovieID, errStudioID),
diff --git a/pkg/performer/export.go b/pkg/performer/export.go
index 2d87d0df6..4b46fd901 100644
--- a/pkg/performer/export.go
+++ b/pkg/performer/export.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"strconv"
 
+	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/models/json"
 	"github.com/stashapp/stash/pkg/models/jsonschema"
@@ -74,7 +75,7 @@ func ToJSON(ctx context.Context, reader ImageAliasStashIDGetter, performer *mode
 
 	image, err := reader.GetImage(ctx, performer.ID)
 	if err != nil {
-		return nil, fmt.Errorf("getting performers image: %w", err)
+		logger.Errorf("Error getting performer image: %v", err)
 	}
 
 	if len(image) > 0 {
diff --git a/pkg/performer/export_test.go b/pkg/performer/export_test.go
index 83278b5eb..f65693e3f 100644
--- a/pkg/performer/export_test.go
+++ b/pkg/performer/export_test.go
@@ -185,8 +185,9 @@ func initTestTable() {
 		},
 		{
 			*createFullPerformer(errImageID, performerName),
-			nil,
-			true,
+			createFullJSONPerformer(performerName, ""),
+			// failure to get image should not cause an error
+			false,
 		},
 	}
 }
diff --git a/pkg/scene/export.go b/pkg/scene/export.go
index f7426c8bd..f076a14b7 100644
--- a/pkg/scene/export.go
+++ b/pkg/scene/export.go
@@ -6,6 +6,7 @@ import (
 	"math"
 	"strconv"
 
+	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/models/json"
 	"github.com/stashapp/stash/pkg/models/jsonschema"
@@ -64,7 +65,7 @@ func ToBasicJSON(ctx context.Context, reader CoverGetter, scene *models.Scene) (
 
 	cover, err := reader.GetCover(ctx, scene.ID)
 	if err != nil {
-		return nil, fmt.Errorf("error getting scene cover: %v", err)
+		logger.Errorf("Error getting scene cover: %v", err)
 	}
 
 	if len(cover) > 0 {
diff --git a/pkg/scene/export_test.go b/pkg/scene/export_test.go
index 623e399a1..684e92db0 100644
--- a/pkg/scene/export_test.go
+++ b/pkg/scene/export_test.go
@@ -178,8 +178,9 @@ var scenarios = []basicTestScenario{
 	},
 	{
 		createFullScene(errImageID),
-		nil,
-		true,
+		createFullJSONScene(""),
+		// failure to get image should not cause an error
+		false,
 	},
 }
 
diff --git a/pkg/sqlite/blob.go b/pkg/sqlite/blob.go
index 96c597fa5..27ecd94ad 100644
--- a/pkg/sqlite/blob.go
+++ b/pkg/sqlite/blob.go
@@ -13,6 +13,7 @@ import (
 	"github.com/mattn/go-sqlite3"
 	"github.com/stashapp/stash/pkg/file"
 	"github.com/stashapp/stash/pkg/hash/md5"
+	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/sqlite/blob"
 	"github.com/stashapp/stash/pkg/utils"
 	"gopkg.in/guregu/null.v4"
@@ -268,6 +269,7 @@ func (qb *BlobStore) Delete(ctx context.Context, checksum string) error {
 	if err := qb.delete(ctx, checksum); err != nil {
 		if qb.isConstraintError(err) {
 			// blob is still referenced - do not delete
+			logger.Debugf("Blob %s is still referenced - not deleting", checksum)
 			return nil
 		}
 
@@ -277,6 +279,7 @@ func (qb *BlobStore) Delete(ctx context.Context, checksum string) error {
 
 	// blob was deleted from the database - delete from filesystem if enabled
 	if qb.options.UseFilesystem {
+		logger.Debugf("Deleting blob %s from filesystem", checksum)
 		if err := qb.fsStore.Delete(ctx, checksum); err != nil {
 			return fmt.Errorf("deleting from filesystem: %w", err)
 		}
@@ -330,17 +333,33 @@ func (qb *blobJoinQueryBuilder) UpdateImage(ctx context.Context, id int, blobCol
 	if len(image) == 0 {
 		return qb.DestroyImage(ctx, id, blobCol)
 	}
+
+	oldChecksum, err := qb.getChecksum(ctx, id, blobCol)
+	if err != nil {
+		return err
+	}
+
 	checksum, err := qb.blobStore.Write(ctx, image)
 	if err != nil {
 		return err
 	}
 
 	sqlQuery := fmt.Sprintf("UPDATE %s SET %s = ? WHERE id = ?", qb.joinTable, blobCol)
-	_, err = qb.tx.Exec(ctx, sqlQuery, checksum, id)
-	return err
+	if _, err := qb.tx.Exec(ctx, sqlQuery, checksum, id); err != nil {
+		return err
+	}
+
+	// #3595 - delete the old blob if the checksum is different
+	if oldChecksum != nil && *oldChecksum != checksum {
+		if err := qb.blobStore.Delete(ctx, *oldChecksum); err != nil {
+			return err
+		}
+	}
+
+	return nil
 }
 
-func (qb *blobJoinQueryBuilder) DestroyImage(ctx context.Context, id int, blobCol string) error {
+func (qb *blobJoinQueryBuilder) getChecksum(ctx context.Context, id int, blobCol string) (*string, error) {
 	sqlQuery := utils.StrFormat(`
 SELECT {joinTable}.{joinCol} FROM {joinTable} WHERE {joinTable}.id = ?
 `, utils.StrFormatMap{
@@ -351,10 +370,23 @@ SELECT {joinTable}.{joinCol} FROM {joinTable} WHERE {joinTable}.id = ?
 	var checksum null.String
 	err := qb.repository.querySimple(ctx, sqlQuery, []interface{}{id}, &checksum)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	if !checksum.Valid {
+		return nil, nil
+	}
+
+	return &checksum.String, nil
+}
+
+func (qb *blobJoinQueryBuilder) DestroyImage(ctx context.Context, id int, blobCol string) error {
+	checksum, err := qb.getChecksum(ctx, id, blobCol)
+	if err != nil {
+		return err
+	}
+
+	if checksum == nil {
 		// no image to delete
 		return nil
 	}
@@ -364,7 +396,7 @@ SELECT {joinTable}.{joinCol} FROM {joinTable} WHERE {joinTable}.id = ?
 		return err
 	}
 
-	return qb.blobStore.Delete(ctx, checksum.String)
+	return qb.blobStore.Delete(ctx, *checksum)
 }
 
 func (qb *blobJoinQueryBuilder) HasImage(ctx context.Context, id int, blobCol string) (bool, error) {
diff --git a/pkg/sqlite/blob/fs.go b/pkg/sqlite/blob/fs.go
index 37ec4b064..9c85f926a 100644
--- a/pkg/sqlite/blob/fs.go
+++ b/pkg/sqlite/blob/fs.go
@@ -11,6 +11,7 @@ import (
 
 	"github.com/stashapp/stash/pkg/file"
 	"github.com/stashapp/stash/pkg/fsutil"
+	"github.com/stashapp/stash/pkg/logger"
 )
 
 const (
@@ -61,6 +62,7 @@ func (s *FilesystemStore) Write(ctx context.Context, checksum string, data []byt
 		return fmt.Errorf("creating directory %q: %w", filepath.Dir(fn), err)
 	}
 
+	logger.Debugf("Writing blob file %s", fn)
 	out, err := s.fs.Create(fn)
 	if err != nil {
 		return fmt.Errorf("creating file %q: %w", fn, err)
diff --git a/pkg/sqlite/movies.go b/pkg/sqlite/movies.go
index 2fa429546..1c591614d 100644
--- a/pkg/sqlite/movies.go
+++ b/pkg/sqlite/movies.go
@@ -364,6 +364,10 @@ func (qb *movieQueryBuilder) GetBackImage(ctx context.Context, movieID int) ([]b
 	return qb.GetImage(ctx, movieID, movieBackImageBlobColumn)
 }
 
+func (qb *movieQueryBuilder) HasBackImage(ctx context.Context, movieID int) (bool, error) {
+	return qb.HasImage(ctx, movieID, movieBackImageBlobColumn)
+}
+
 func (qb *movieQueryBuilder) FindByPerformerID(ctx context.Context, performerID int) ([]*models.Movie, error) {
 	query := `SELECT DISTINCT movies.*
 FROM movies
diff --git a/pkg/studio/export.go b/pkg/studio/export.go
index 27cbaeb38..f0cad2eef 100644
--- a/pkg/studio/export.go
+++ b/pkg/studio/export.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/models/json"
 	"github.com/stashapp/stash/pkg/models/jsonschema"
@@ -61,7 +62,7 @@ func ToJSON(ctx context.Context, reader FinderImageStashIDGetter, studio *models
 
 	image, err := reader.GetImage(ctx, studio.ID)
 	if err != nil {
-		return nil, fmt.Errorf("error getting studio image: %v", err)
+		logger.Errorf("Error getting studio image: %v", err)
 	}
 
 	if len(image) > 0 {
diff --git a/pkg/studio/export_test.go b/pkg/studio/export_test.go
index 8b329668e..702bab863 100644
--- a/pkg/studio/export_test.go
+++ b/pkg/studio/export_test.go
@@ -147,8 +147,9 @@ func initTestTable() {
 		},
 		{
 			createFullStudio(errImageID, parentStudioID),
-			nil,
-			true,
+			createFullJSONStudio(parentStudioName, "", nil),
+			// failure to get image is not an error
+			false,
 		},
 		{
 			createFullStudio(missingParentStudioID, missingStudioID),
@@ -200,6 +201,7 @@ func TestToJSON(t *testing.T) {
 	mockStudioReader.On("GetStashIDs", ctx, studioID).Return(stashIDs, nil).Once()
 	mockStudioReader.On("GetStashIDs", ctx, noImageID).Return(nil, nil).Once()
 	mockStudioReader.On("GetStashIDs", ctx, missingParentStudioID).Return(stashIDs, nil).Once()
+	mockStudioReader.On("GetStashIDs", ctx, errImageID).Return(stashIDs, nil).Once()
 
 	for i, s := range scenarios {
 		studio := s.input
diff --git a/pkg/tag/export.go b/pkg/tag/export.go
index 93f5e97b3..fc37ae43f 100644
--- a/pkg/tag/export.go
+++ b/pkg/tag/export.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/stashapp/stash/pkg/logger"
 	"github.com/stashapp/stash/pkg/models"
 	"github.com/stashapp/stash/pkg/models/json"
 	"github.com/stashapp/stash/pkg/models/jsonschema"
@@ -35,7 +36,7 @@ func ToJSON(ctx context.Context, reader FinderAliasImageGetter, tag *models.Tag)
 
 	image, err := reader.GetImage(ctx, tag.ID)
 	if err != nil {
-		return nil, fmt.Errorf("error getting tag image: %v", err)
+		logger.Errorf("Error getting tag image: %v", err)
 	}
 
 	if len(image) > 0 {
diff --git a/pkg/tag/export_test.go b/pkg/tag/export_test.go
index e7a68aaf1..e207db7a5 100644
--- a/pkg/tag/export_test.go
+++ b/pkg/tag/export_test.go
@@ -92,8 +92,9 @@ func initTestTable() {
 		},
 		{
 			createTag(errImageID),
-			nil,
-			true,
+			createJSONTag(nil, "", nil),
+			// getting the image should not cause an error
+			false,
 		},
 		{
 			createTag(errAliasID),
@@ -140,6 +141,7 @@ func TestToJSON(t *testing.T) {
 	mockTagReader.On("FindByChildTagID", ctx, noImageID).Return(nil, nil).Once()
 	mockTagReader.On("FindByChildTagID", ctx, withParentsID).Return([]*models.Tag{{Name: "parent"}}, nil).Once()
 	mockTagReader.On("FindByChildTagID", ctx, errParentsID).Return(nil, parentsErr).Once()
+	mockTagReader.On("FindByChildTagID", ctx, errImageID).Return(nil, nil).Once()
 
 	for i, s := range scenarios {
 		tag := s.tag