Simulot/issue608 (#627)

* Add the possibility to override the temporary path used by Immich-go

* Add environment variables section to README with IMMICHGO_TEMPDIR description

* Add tests

* Add log helper package for centralized logging functionality

* Add OSFS interface to permit  file system operations logging

* Refactor CacheReader to use OSFS and add reference counting for temporary files

* Update grouping logic to exclude live photos for burst detection

app/log.go

@@ -13,6 +13,7 @@ import (
 	"github.com/phsym/console-slog"
 	slogmulti "github.com/samber/slog-multi"
 	"github.com/simulot/immich-go/internal/configuration"
+	"github.com/simulot/immich-go/internal/loghelper"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	"github.com/spf13/viper"
@@ -67,6 +68,7 @@ func (log *Log) OpenLogFile() error {
 		w = os.Stdout
 	}
 	log.setHandlers(w, nil)
+	loghelper.SetGlobalLogger(log.Logger)
 	return nil
 }
 

internal/assets/assetFile.go

@@ -2,9 +2,9 @@ package assets
 
 import (
 	"fmt"
-	"os"
 
 	"github.com/simulot/immich-go/internal/fshelper/cachereader"
+	"github.com/simulot/immich-go/internal/fshelper/osfs"
 )
 
 func (a *Asset) DeviceAssetID() string {
@@ -14,15 +14,15 @@ func (a *Asset) DeviceAssetID() string {
 // OpenFile return an os.File whatever the type of source reader is.
 // It can be called several times for the same asset.
 
-func (a *Asset) OpenFile() (*os.File, error) {
+func (a *Asset) OpenFile() (osfs.OSFS, error) {
 	if a.cacheReader == nil {
 		// get a FS.File from of the asset
 		f, err := a.File.Open()
 		if err != nil {
 			return nil, err
 		}
 		// Create a cache reader from the FS.File
-		cr, err := cachereader.NewCacheReader(f)
+		cr, err := cachereader.NewCacheReader(a.File.FullName(), f)
 		if err != nil {
 			return nil, err
 		}

internal/assets/metadata.go

@@ -2,6 +2,7 @@ package assets
 
 import (
 	"encoding/json"
+	"fmt"
 	"log/slog"
 	"time"
 
@@ -25,9 +26,16 @@ type Metadata struct {
 }
 
 func (m Metadata) LogValue() slog.Value {
+	var gpsGroup slog.Value
+	if m.Latitude != 0 || m.Longitude != 0 {
+		gpsGroup = slog.GroupValue(
+			slog.String("latitude", fmt.Sprintf("%0.f.xxxx", m.Latitude)),
+			slog.String("longitude", fmt.Sprintf("%0.f.xxxx", m.Longitude)),
+		)
+	}
+
 	return slog.GroupValue(
-		slog.Float64("latitude", m.Latitude),
-		slog.Float64("longitude", m.Longitude),
+		slog.Any("GPS coordinates", gpsGroup),
 		slog.Any("fileName", m.File),
 		slog.Time("dateTaken", m.DateTaken),
 		slog.String("description", m.Description),

internal/e2eTests/upload/e2e_from_folder_test.go

@@ -44,6 +44,31 @@ func TestUploadBurstInAlbums(t *testing.T) {
 	}
 }
 
+func TestUploadBurstFromZip(t *testing.T) {
+	e2e.InitMyEnv()
+	e2e.ResetImmich(t)
+
+	ctx := context.Background()
+	c, a := cmd.RootImmichGoCommand(ctx)
+	c.SetArgs([]string{
+		"upload", "from-folder",
+		"--server=" + e2e.MyEnv("IMMICHGO_SERVER"),
+		"--api-key=" + e2e.MyEnv("IMMICHGO_APIKEY"),
+		"--no-ui",
+		"--into-album=ALBUM",
+		"--manage-raw-jpeg=KeepRaw",
+		"--manage-burst=stack",
+		"--log-level=DEBUG",
+		e2e.MyEnv("IMMICHGO_TESTFILES") + "/burst/storm.zip",
+	})
+
+	// let's start
+	err := c.ExecuteContext(ctx)
+	if err != nil && a.Log().GetSLog() != nil {
+		a.Log().Error(err.Error())
+	}
+}
+
 func TestUploadArchive(t *testing.T) {
 	e2e.InitMyEnv()
 	e2e.ResetImmich(t)

internal/e2eTests/upload/e2e_from_google_photos_test.go

@@ -28,6 +28,7 @@ func TestUploadFromGooglePhotos(t *testing.T) {
 		"upload", "from-google-photos",
 		"--server=" + e2e.MyEnv("IMMICHGO_SERVER"),
 		"--api-key=" + e2e.MyEnv("IMMICHGO_APIKEY"),
+		"--log-level=DEBUG",
 		// "--no-ui",
 		e2e.MyEnv("IMMICHGO_TESTFILES") + "/demo takeout/Takeout",
 	})
@@ -50,6 +51,9 @@ func TestUploadFromGooglePhotosZipped(t *testing.T) {
 		"upload", "from-google-photos",
 		"--server=" + e2e.MyEnv("IMMICHGO_SERVER"),
 		"--api-key=" + e2e.MyEnv("IMMICHGO_APIKEY"),
+		"--log-level=DEBUG",
+		"--manage-burst=Stack",
+		"--manage-raw-jpeg=StackCoverJPG",
 		// "--no-ui",
 		e2e.MyEnv("IMMICHGO_TESTFILES") + "/demo takeout/zip/takeout-*.zip",
 	})
@@ -61,6 +65,32 @@ func TestUploadFromGooglePhotosZipped(t *testing.T) {
 	}
 }
 
+func TestUploadFromGooglePhotosZippedIssue608(t *testing.T) {
+	e2e.InitMyEnv()
+	e2e.ResetImmich(t)
+
+	ctx := context.Background()
+
+	c, a := cmd.RootImmichGoCommand(ctx)
+	c.SetArgs([]string{
+		"upload", "from-google-photos",
+		"--server=" + e2e.MyEnv("IMMICHGO_SERVER"),
+		"--api-key=" + e2e.MyEnv("IMMICHGO_APIKEY"),
+		"--log-level=DEBUG",
+		"--manage-burst=Stack",
+		"--manage-raw-jpeg=StackCoverJPG",
+		"--include-unmatched",
+		// "--no-ui",
+		e2e.MyEnv("IMMICHGO_TESTFILES") + "/burst/takeout-reflex.zip",
+	})
+
+	// let's start
+	err := c.ExecuteContext(ctx)
+	if err != nil && a.Log().GetSLog() != nil {
+		a.Log().Error(err.Error())
+	}
+}
+
 func TestUploadFromGPInCurrent(t *testing.T) {
 	curDir, err := os.Getwd()
 	if err != nil {
@@ -123,3 +153,25 @@ func TestUploadFromGP_issue613(t *testing.T) {
 		a.Log().Error(err.Error())
 	}
 }
+
+func TestUploadFromGP_issue608(t *testing.T) {
+	e2e.InitMyEnv()
+	e2e.ResetImmich(t)
+
+	ctx := context.Background()
+
+	c, a := cmd.RootImmichGoCommand(ctx)
+	c.SetArgs([]string{
+		"upload", "from-google-photos",
+		"--server=" + e2e.MyEnv("IMMICHGO_SERVER"),
+		"--api-key=" + e2e.MyEnv("IMMICHGO_APIKEY"),
+		// "--no-ui",
+		e2e.MyEnv("IMMICHGO_TESTFILES") + "/#608 missing temp files/takeout",
+	})
+
+	// let's start
+	err := c.ExecuteContext(ctx)
+	if err != nil && a.Log().GetSLog() != nil {
+		a.Log().Error(err.Error())
+	}
+}

internal/fshelper/cachereader/cachereader.go

@@ -4,64 +4,97 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"sync/atomic"
+
+	"github.com/simulot/immich-go/internal/fshelper/osfs"
+	"github.com/simulot/immich-go/internal/loghelper"
 )
 
 // CacheReader is a reader that caches the data in a temporary file to allow multiple reads
 type CacheReader struct {
-	tmpFile      *os.File // tmpFile is the temporary file or the original file
+	tmpFile      osfs.OSFS //*os.File // tmpFile is the temporary file or the original file
+	name         string
 	shouldRemove bool
+	references   int64
 }
 
 // NewCacheReader creates a new CacheReader from an io.ReadCloser
 // When the reader is an os.File, it will be used directly
 // Otherwise, the content will be copied into a temporary file, and the original reader will be closed
-func NewCacheReader(rc io.ReadCloser) (*CacheReader, error) {
+func NewCacheReader(name string, rc io.ReadCloser) (*CacheReader, error) {
 	var err error
 	c := &CacheReader{}
-	if f, ok := rc.(*os.File); ok {
+	if f, ok := rc.(osfs.OSFS); ok {
+		c.name = f.Name()
 		c.tmpFile = f
 	} else {
-		d, err := os.UserCacheDir()
-		if err != nil {
-			d = os.TempDir()
+		d := os.Getenv("IMMICHGO_TEMPDIR")
+		if d == "" {
+			d, err = os.UserCacheDir()
+			if err != nil {
+				d = os.TempDir()
+			}
 		}
 		d = filepath.Join(d, "immich-go", "temp")
 
 		err = os.MkdirAll(d, 0o700)
 		if err != nil {
 			d = os.TempDir()
 		}
-
 		c.tmpFile, err = os.CreateTemp(d, "immich-go_*")
 		if err != nil {
 			return nil, err
 		}
+		c.name = c.tmpFile.Name()
 		// be sure to copy the reader content into the temporary file
 		_, err = io.Copy(c.tmpFile, rc)
 		if err != nil {
+			c.tmpFile.Close()
+			_ = os.Remove(c.name)
 			return nil, err
 		}
 		rc.Close()
+		loghelper.Debug("CacheReader: create temporary file", "Source file", name, "temp file", c.name)
 		c.shouldRemove = true
 	}
 	return c, err
 }
 
-// OpenFile creates a new file based on the temporary file
-func (cr *CacheReader) OpenFile() (*os.File, error) {
-	f, err := os.Open(cr.tmpFile.Name())
+// OpenFile creates a new file handler based on the temporary file
+func (cr *CacheReader) OpenFile() (*tempFile, error) {
+	refs := atomic.AddInt64(&cr.references, 1)
+	loghelper.Debug("tempFile:", "Open file", cr.name, "references", refs)
+	f, err := os.Open(cr.name)
 	if err != nil {
 		return nil, err
 	}
-	return f, nil
+	return &tempFile{File: f, cr: cr}, nil
 }
 
 // Close closes the temporary file only if it was created by NewCacheReader
 func (cr *CacheReader) Close() error {
+	refs := atomic.LoadInt64(&cr.references)
+	loghelper.Debug("CacheReader: close", "name", cr.name, "references", refs)
 	if cr.shouldRemove {
 		// the source is already closed
-		return os.Remove(cr.tmpFile.Name())
+		loghelper.Debug("CacheReader: remove temporary file", "name", cr.name)
+		return os.Remove(cr.name)
 	} else {
 		return cr.tmpFile.Close()
 	}
 }
+
+type tempFile struct {
+	*os.File
+	cr *CacheReader
+}
+
+func (t *tempFile) Close() error {
+	refs := atomic.AddInt64(&t.cr.references, -1)
+	loghelper.Debug("tempFile:", "close file", t.File.Name(), "references", refs+1)
+	if refs < 0 {
+		panic("tempFile: Close() called on a closed file")
+	}
+	err := t.File.Close()
+	return err
+}

internal/fshelper/cachereader/cacheredear_test.go

@@ -25,7 +25,7 @@ func Test_NewReaderAtOnBuffer(t *testing.T) {
 	}
 
 	b := makeBuffer(0, 4096)
-	cr, err := NewCacheReader(io.NopCloser(bytes.NewReader(b)))
+	cr, err := NewCacheReader("test", io.NopCloser(bytes.NewReader(b)))
 	if err != nil {
 		t.Fatalf("NewCacheReader() error = %v", err)
 	}
@@ -92,7 +92,7 @@ func Test_NewReaderAtOnFile(t *testing.T) {
 		return
 	}
 
-	cr, err := NewCacheReader(f)
+	cr, err := NewCacheReader("test", f)
 	if err != nil {
 		t.Fatalf("NewCacheReader() error = %v", err)
 	}

internal/fshelper/osfs/osfs.go

@@ -58,3 +58,13 @@ func (dir dirFS) MkSymlink(name, target string) error {
 func (dir dirFS) Remove(name string) error {
 	return os.Remove(filepath.Join(string(dir), name))
 }
+
+type OSFS interface {
+	fs.File
+	Name() string
+	ReadAt(b []byte, off int64) (n int, err error)
+	Seek(offset int64, whence int) (ret int64, err error)
+	Stat() (fs.FileInfo, error)
+	Write(b []byte) (n int, err error)
+	WriteAt(b []byte, off int64) (n int, err error)
+}

internal/groups/series/series.go

@@ -37,7 +37,7 @@ func Group(ctx context.Context, in <-chan *assets.Asset, out chan<- *assets.Asse
 			}
 			r := a.Radical
 			cd := a.CaptureDate
-			if cd.IsZero() || abs(cd.Sub(currentCaptureDate)) > threshold || r != currentRadical {
+			if r != currentRadical || a.Type != filetypes.TypeImage || cd.IsZero() || abs(cd.Sub(currentCaptureDate)) > threshold {
 				if len(currentGroup) > 0 {
 					sendGroup(ctx, out, gOut, currentGroup)
 					currentGroup = []*assets.Asset{}
@@ -102,6 +102,7 @@ func sendGroup(ctx context.Context, out chan<- *assets.Asset, outg chan<- *asset
 					return
 				}
 			}
+			return
 		}
 	}
 

internal/loghelper/loghelp.go

@@ -0,0 +1,39 @@
+package loghelper
+
+import "log/slog"
+
+var globalLogger *slog.Logger
+
+func SetGlobalLogger(l *slog.Logger) {
+	globalLogger = l
+}
+
+func Log(message string, values ...interface{}) {
+	if globalLogger != nil {
+		globalLogger.Info(message, values...)
+	}
+}
+
+func Info(message string, values ...interface{}) {
+	if globalLogger != nil {
+		globalLogger.Info(message, values...)
+	}
+}
+
+func Error(message string, values ...interface{}) {
+	if globalLogger != nil {
+		globalLogger.Error(message, values...)
+	}
+}
+
+func Warn(message string, values ...interface{}) {
+	if globalLogger != nil {
+		globalLogger.Warn(message, values...)
+	}
+}
+
+func Debug(message string, values ...interface{}) {
+	if globalLogger != nil {
+		globalLogger.Debug(message, values...)
+	}
+}