Simpler image service

backend/app/rest/proxy/image.go

@@ -159,10 +159,7 @@ func (p Image) cacheImage(r io.Reader, imgID string) {
 	if err != nil {
 		log.Printf("[WARN] unable to save image to the storage: %+v", err)
 	}
-	// In the future we can do something smarter than just committing everything (eg, some kind of LFU/LRU)
-	if err := p.ImageService.Commit(id); err != nil {
-		log.Printf("[WARN] unable to commit image %s", imgID)
-	}
+	p.ImageService.Submit(func() []string { return []string{id} })
 }
 
 // download an image. Returns a Reader which has to be closed by a caller
@@ -189,8 +186,7 @@ func (p Image) downloadImage(ctx context.Context, imgURL string) (io.ReadCloser,
 		return e
 	})
 	if err != nil {
-		log.Print(err.Error())
-		return nil, err
+		return nil, errors.Wrapf(err, "can't download image %s", imgURL)
 	}
 
 	if resp.StatusCode != http.StatusOK {

backend/app/rest/proxy/image_test.go

@@ -19,7 +19,7 @@ import (
 	"github.com/umputun/remark/backend/app/store/image"
 )
 
-func TestPicture_Extract(t *testing.T) {
+func TestImage_Extract(t *testing.T) {
 
 	tbl := []struct {
 		inp string
@@ -61,7 +61,7 @@ func TestPicture_Extract(t *testing.T) {
 	}
 }
 
-func TestPicture_Replace(t *testing.T) {
+func TestImage_Replace(t *testing.T) {
 	img := Image{HTTP2HTTPS: true, RoutePath: "/img"}
 	r := img.replace(`<img src="http://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`,
 		[]string{"http://radio-t.com/img3.png", "http://images.pexels.com/67636/img4.jpeg"})
@@ -73,7 +73,7 @@ func TestImage_Routes(t *testing.T) {
 
 	ts := httptest.NewServer(http.HandlerFunc(img.Handler))
 	defer ts.Close()
-	httpSrv := imgHTTPServer(t)
+	httpSrv := imgHTTPTestsServer(t)
 	defer httpSrv.Close()
 
 	encodedImgURL := base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/img1.png"))
@@ -95,7 +95,7 @@ func TestImage_Routes(t *testing.T) {
 	assert.Equal(t, 400, resp.StatusCode)
 }
 
-func TestImage_Routes_CachingImage(t *testing.T) {
+func TestImage_RoutesCachingImage(t *testing.T) {
 	imageStore := image.MockStore{}
 	img := Image{
 		CacheExternal: true,
@@ -106,15 +106,15 @@ func TestImage_Routes_CachingImage(t *testing.T) {
 
 	ts := httptest.NewServer(http.HandlerFunc(img.Handler))
 	defer ts.Close()
-	httpSrv := imgHTTPServer(t)
+	httpSrv := imgHTTPTestsServer(t)
 	defer httpSrv.Close()
 
 	imgURL := httpSrv.URL + "/image/img1.png"
 	encodedImgURL := base64.URLEncoding.EncodeToString([]byte(imgURL))
 
 	imageStore.On("Load", mock.Anything).Once().Return(nil, int64(0), nil)
 	imageStore.On("SaveWithID", mock.Anything, mock.Anything).Once().Run(func(args mock.Arguments) { _, _ = ioutil.ReadAll(args.Get(1).(io.Reader)) }).Return("", nil)
-	imageStore.On("Commit", mock.Anything).Once().Return(nil)
+	imageStore.On("commit", mock.Anything).Once().Return(nil)
 
 	resp, err := http.Get(ts.URL + "/?src=" + encodedImgURL)
 	require.Nil(t, err)
@@ -124,10 +124,10 @@ func TestImage_Routes_CachingImage(t *testing.T) {
 
 	imageStore.AssertCalled(t, "Load", mock.Anything)
 	imageStore.AssertCalled(t, "SaveWithID", "cached_images/4b84b15bff6ee5796152495a230e45e3d7e947d9-"+sha1Str(imgURL), mock.Anything)
-	imageStore.AssertCalled(t, "Commit", mock.Anything)
+	imageStore.AssertCalled(t, "commit", mock.Anything)
 }
 
-func TestImage_Routes_Using_Cachded_Image(t *testing.T) {
+func TestImage_RoutesUsingCachedImage(t *testing.T) {
 	imageStore := image.MockStore{}
 	img := Image{
 		CacheExternal: true,
@@ -138,12 +138,12 @@ func TestImage_Routes_Using_Cachded_Image(t *testing.T) {
 
 	ts := httptest.NewServer(http.HandlerFunc(img.Handler))
 	defer ts.Close()
-	httpSrv := imgHTTPServer(t)
+	httpSrv := imgHTTPTestsServer(t)
 	defer httpSrv.Close()
 
 	encodedImgURL := base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/img1.png"))
 
-	// In order to validate that cached data is used cache "will return" some other data from what http server would
+	// In order to validate that cached data used cache "will return" some other data from what http server would
 	imageReader := ioutil.NopCloser(bytes.NewReader([]byte(fmt.Sprintf("%256s", "X"))))
 	imageStore.On("Load", mock.Anything).Once().Return(imageReader, int64(256), nil)
 
@@ -161,7 +161,7 @@ func TestImage_RoutesTimedOut(t *testing.T) {
 
 	ts := httptest.NewServer(http.HandlerFunc(img.Handler))
 	defer ts.Close()
-	httpSrv := imgHTTPServer(t)
+	httpSrv := imgHTTPTestsServer(t)
 	defer httpSrv.Close()
 
 	encodedImgURL := base64.URLEncoding.EncodeToString([]byte(httpSrv.URL + "/image/img-slow.png"))
@@ -174,7 +174,7 @@ func TestImage_RoutesTimedOut(t *testing.T) {
 	assert.True(t, strings.Contains(string(b), "deadline exceeded"))
 }
 
-func TestPicture_Convert_ProxyMode(t *testing.T) {
+func TestImage_ConvertProxyMode(t *testing.T) {
 	img := Image{HTTP2HTTPS: true, RoutePath: "/img"}
 	r := img.Convert(`<img src="http://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`)
 	assert.Equal(t, `<img src="/img?src=aHR0cDovL3JhZGlvLXQuY29tL2ltZzMucG5n"/> xyz <img src="/img?src=aHR0cDovL2ltYWdlcy5wZXhlbHMuY29tLzY3NjM2L2ltZzQuanBlZw==">`, r)
@@ -191,7 +191,7 @@ func TestPicture_Convert_ProxyMode(t *testing.T) {
 	assert.Equal(t, `<img src="http://radio-t.com/img3.png"/> xyz`, r, "disabled, no proxy")
 }
 
-func TestPicture_Convert_CachingMode(t *testing.T) {
+func TestImage_ConvertCachingMode(t *testing.T) {
 	img := Image{CacheExternal: true, RoutePath: "/img", RemarkURL: "https://remark42.com"}
 	r := img.Convert(`<img src="http://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`)
 	assert.Equal(t, `<img src="https://remark42.com/img?src=aHR0cDovL3JhZGlvLXQuY29tL2ltZzMucG5n"/> xyz <img src="https://remark42.com/img?src=aHR0cDovL2ltYWdlcy5wZXhlbHMuY29tLzY3NjM2L2ltZzQuanBlZw==">`, r)
@@ -206,13 +206,13 @@ func TestPicture_Convert_CachingMode(t *testing.T) {
 	r = img.Convert(`<img src="http://radio-t.com/img3.png"/>`)
 	assert.Equal(t, `<img src="http://radio-t.com/img3.png"/>`, r)
 
-	// both Caching and Proxy are enabled
+	// both Caching and Proxy enabled
 	img = Image{CacheExternal: true, HTTP2HTTPS: true, RoutePath: "/img", RemarkURL: "https://remark42.com"}
 	r = img.Convert(`<img src="http://radio-t.com/img3.png"/> xyz <img src="http://images.pexels.com/67636/img4.jpeg">`)
 	assert.Equal(t, `<img src="https://remark42.com/img?src=aHR0cDovL3JhZGlvLXQuY29tL2ltZzMucG5n"/> xyz <img src="https://remark42.com/img?src=aHR0cDovL2ltYWdlcy5wZXhlbHMuY29tLzY3NjM2L2ltZzQuanBlZw==">`, r)
 }
 
-func imgHTTPServer(t *testing.T) *httptest.Server {
+func imgHTTPTestsServer(t *testing.T) *httptest.Server {
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path == "/image/img1.png" {
 			t.Log("http img request", r.URL)

backend/app/store/image/bolt_store.go

@@ -29,7 +29,7 @@ type Bolt struct {
 	MaxWidth  int
 }
 
-// Create Bolt Store.
+// NewBoltStorage create bolt image store
 func NewBoltStorage(fileName string, maxSize int, maxHeight int, maxWidth int, options bolt.Options) (*Bolt, error) {
 	db, err := bolt.Open(fileName, 0600, &options)
 	if err != nil {
@@ -60,7 +60,7 @@ func NewBoltStorage(fileName string, maxSize int, maxHeight int, maxWidth int, o
 	}, nil
 }
 
-// SaveWithID saves data from reader with given id
+// SaveWithID saves data from a reader, for given id
 func (b *Bolt) SaveWithID(id string, r io.Reader) (string, error) {
 	data, err := readAndValidateImage(r, b.MaxSize)
 	if err != nil {
@@ -87,14 +87,14 @@ func (b *Bolt) SaveWithID(id string, r io.Reader) (string, error) {
 }
 
 // Save data from reader to staging bucket in DB
-func (b *Bolt) Save(fileName string, userID string, r io.Reader) (id string, err error) {
+func (b *Bolt) Save(_ string, userID string, r io.Reader) (id string, err error) {
 	id = path.Join(userID, guid())
 	return b.SaveWithID(id, r)
 }
 
 // Commit file stored in staging bucket by copying it to permanent bucket
 // Data from staging bucket not removed immediately, but would be removed on cleanup
-func (b *Bolt) Commit(id string) error {
+func (b *Bolt) commit(id string) error {
 	err := b.db.Update(func(tx *bolt.Tx) error {
 		data := tx.Bucket([]byte(imagesStagedBktName)).Get([]byte(id))
 		if data == nil {
@@ -127,7 +127,7 @@ func (b *Bolt) Load(id string) (io.ReadCloser, int64, error) {
 }
 
 // Cleanup runs scan of staging and removes old data based on ttl
-func (b *Bolt) Cleanup(ctx context.Context, ttl time.Duration) error {
+func (b *Bolt) cleanup(_ context.Context, ttl time.Duration) error {
 	err := b.db.Update(func(tx *bolt.Tx) error {
 		c := tx.Bucket([]byte(insertTimeBktName)).Cursor()
 

backend/app/store/image/bolt_store_test.go

@@ -31,7 +31,7 @@ func TestBoltStore_SaveCommit(t *testing.T) {
 	})
 	assert.NoError(t, err)
 
-	err = svc.Commit(id)
+	err = svc.commit(id)
 	require.NoError(t, err)
 
 	err = svc.db.View(func(tx *bolt.Tx) error {
@@ -90,18 +90,18 @@ func TestBoltStore_Cleanup(t *testing.T) {
 	time.Sleep(100 * time.Millisecond)
 	img3 := save("blah_ff3.png", "user2")
 
-	err := svc.Cleanup(context.Background(), time.Since(img1ts)) // clean first images
+	err := svc.cleanup(context.Background(), time.Since(img1ts)) // clean first images
 	assert.NoError(t, err)
 
 	assertBoltImgNil(t, svc.db, imagesStagedBktName, img1)
 	assertBoltImgNil(t, svc.db, imagesBktName, img1)
 	assertBoltImgNotNil(t, svc.db, imagesStagedBktName, img2)
 	assertBoltImgNotNil(t, svc.db, imagesStagedBktName, img3)
 
-	err = svc.Commit(img3)
+	err = svc.commit(img3)
 	require.NoError(t, err)
 
-	err = svc.Cleanup(context.Background(), time.Millisecond*10)
+	err = svc.cleanup(context.Background(), time.Millisecond*10)
 	assert.NoError(t, err)
 
 	assertBoltImgNil(t, svc.db, imagesStagedBktName, img2)

backend/app/store/image/fs_store.go

@@ -36,7 +36,7 @@ type FileSystem struct {
 	}
 }
 
-// SaveWithID saves data from reader with given id
+// SaveWithID saves data from a reader, with given id
 func (f *FileSystem) SaveWithID(id string, r io.Reader) (string, error) {
 	data, err := readAndValidateImage(r, f.MaxSize)
 	if err != nil {
@@ -51,26 +51,26 @@ func (f *FileSystem) SaveWithID(id string, r io.Reader) (string, error) {
 	}
 
 	if err = ioutil.WriteFile(dst, data, 0600); err != nil {
-		return "", errors.Wrapf(err, "can't write file")
+		return "", errors.Wrapf(err, "can't write image file with id %s", id)
 	}
 
 	log.Printf("[DEBUG] file %s saved for image %s, size=%d", dst, id, len(data))
 	return id, nil
 }
 
-// Save data from reader for given file name to local FS, staging directory. Returns id as user/uuid
-// Files partitioned across multiple subdirectories and the final path includes part, i.e. /location/user1/03/123-4567
+// Save data from a reader for given file name to local FS, staging directory. Returns id as user/uuid
+// Files partitioned across multiple subdirectories, and the final path includes part, i.e. /location/user1/03/123-4567
 func (f *FileSystem) Save(fileName string, userID string, r io.Reader) (id string, err error) {
-	id = path.Join(userID, guid()) // make id as user/uuid
-	finalID, err := f.SaveWithID(id, r)
+	tempId := path.Join(userID, guid()) // make id as user/uuid
+	id, err = f.SaveWithID(tempId, r)
 	if err != nil {
-		err = errors.Wrapf(err, "can't save file %s", fileName)
+		err = errors.Wrapf(err, "can't save image file %s", fileName)
 	}
-	return finalID, err
+	return id, err
 }
 
 // Commit file stored in staging location by moving it to permanent location
-func (f *FileSystem) Commit(id string) error {
+func (f *FileSystem) commit(id string) error {
 	log.Printf("[DEBUG] commit image %s", id)
 	stagingImage, permImage := f.location(f.Staging, id), f.location(f.Location, id)
 
@@ -110,12 +110,13 @@ func (f *FileSystem) Load(id string) (io.ReadCloser, int64, error) {
 }
 
 // Cleanup runs scan of staging and removes old files based on ttl
-func (f *FileSystem) Cleanup(ctx context.Context, ttl time.Duration) error {
+func (f *FileSystem) cleanup(_ context.Context, ttl time.Duration) error {
 
 	if _, err := os.Stat(f.Staging); os.IsNotExist(err) {
 		return nil
 	}
 
+	// we can ignore context as on local FS remove is relatively fast operation
 	err := filepath.Walk(f.Staging, func(fpath string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err

backend/app/store/image/fs_store_test.go

@@ -129,7 +129,7 @@ func TestFsStore_SaveAndCommit(t *testing.T) {
 
 	id, err := svc.Save("file1.png", "user1", gopherPNG())
 	require.NoError(t, err)
-	err = svc.Commit(id)
+	err = svc.commit(id)
 	require.NoError(t, err)
 
 	imgStaging := svc.location(svc.Staging, id)
@@ -180,7 +180,7 @@ func TestFsStore_LoadAfterCommit(t *testing.T) {
 	id, err := svc.Save("blah_ff1.png", "user1", gopherPNG())
 	assert.NoError(t, err)
 	t.Log(id)
-	err = svc.Commit(id)
+	err = svc.commit(id)
 	require.NoError(t, err)
 
 	r, sz, err := svc.Load(id)
@@ -260,7 +260,7 @@ func TestFsStore_Cleanup(t *testing.T) {
 	img3 := save("blah_ff3.png", "user2")
 
 	time.Sleep(100 * time.Millisecond) // make first image expired
-	err := svc.Cleanup(context.Background(), time.Millisecond*300)
+	err := svc.cleanup(context.Background(), time.Millisecond*300)
 	assert.NoError(t, err)
 
 	_, err = os.Stat(img1)
@@ -280,7 +280,7 @@ func TestFsStore_Cleanup(t *testing.T) {
 	assert.NoError(t, err, "file on staging")
 
 	time.Sleep(200 * time.Millisecond) // make all images expired
-	err = svc.Cleanup(context.Background(), time.Millisecond*300)
+	err = svc.cleanup(context.Background(), time.Millisecond*300)
 	assert.NoError(t, err)
 
 	_, err = os.Stat(img2)