From 94734ed4cb8d5acce21ad33ad09eafaf05bb74d1 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 8 Jun 2018 02:28:09 -0500 Subject: [PATCH] externalize avatar store with interface --- app/main.go | 2 +- app/rest/api/rest_test.go | 8 ++- app/rest/proxy/avatar.go | 57 ++++--------------- app/rest/proxy/avatar_store.go | 87 +++++++++++++++++++++++++++++ app/rest/proxy/avatar_store_test.go | 65 +++++++++++++++++++++ app/rest/proxy/avatar_test.go | 25 ++------- 6 files changed, 173 insertions(+), 71 deletions(-) create mode 100644 app/rest/proxy/avatar_store.go create mode 100644 app/rest/proxy/avatar_store_test.go diff --git a/app/main.go b/app/main.go index b21b91cc..7cdae9c9 100644 --- a/app/main.go +++ b/app/main.go @@ -135,7 +135,7 @@ func New(opts Opts) (*Application, error) { jwtService := auth.NewJWT(opts.SecretKey, strings.HasPrefix(opts.RemarkURL, "https://"), 7*24*time.Hour) avatarProxy := &proxy.Avatar{ - StorePath: opts.AvatarStore, + Store: proxy.NewFSAvatarStore(opts.AvatarStore), RoutePath: "/api/v1/avatar", RemarkURL: strings.TrimSuffix(opts.RemarkURL, "/"), } diff --git a/app/rest/api/rest_test.go b/app/rest/api/rest_test.go index 934edcc1..90b00b65 100644 --- a/app/rest/api/rest_test.go +++ b/app/rest/api/rest_test.go @@ -37,12 +37,14 @@ func TestRest_FileServer(t *testing.T) { } func TestRest_Shutdown(t *testing.T) { - srv := Rest{Authenticator: auth.Authenticator{}, - AvatarProxy: &proxy.Avatar{StorePath: "/tmp", RoutePath: "/api/v1/avatar"}, ImageProxy: &proxy.Image{}} + srv := Rest{Authenticator: auth.Authenticator{}, AvatarProxy: &proxy.Avatar{Store: proxy.NewFSAvatarStore("/tmp"), + RoutePath: "/api/v1/avatar"}, ImageProxy: &proxy.Image{}} + go func() { time.Sleep(100 * time.Millisecond) srv.Shutdown() }() + st := time.Now() srv.Run(0) assert.True(t, time.Since(st).Seconds() < 1, "should take about 100ms") @@ -65,7 +67,7 @@ func prep(t *testing.T) (srv *Rest, ts *httptest.Server) { Cache: &mockCache{}, WebRoot: "/tmp", RemarkURL: "https://demo.remark42.com", - AvatarProxy: &proxy.Avatar{StorePath: "/tmp", RoutePath: "/api/v1/avatar"}, + AvatarProxy: &proxy.Avatar{Store: proxy.NewFSAvatarStore("/tmp"), RoutePath: "/api/v1/avatar"}, ImageProxy: &proxy.Image{}, ReadOnlyAge: 10, } diff --git a/app/rest/proxy/avatar.go b/app/rest/proxy/avatar.go index 89c2dcf6..8b4ec746 100644 --- a/app/rest/proxy/avatar.go +++ b/app/rest/proxy/avatar.go @@ -1,13 +1,10 @@ package proxy import ( - "fmt" "hash/crc64" "io" "log" "net/http" - "os" - "path" "strconv" "strings" "sync" @@ -23,7 +20,7 @@ import ( // Avatar provides file-system store and http handler for avatars // On user login auth will call Put and it will retrieve and save picture locally. type Avatar struct { - StorePath string + Store AvatarStore RoutePath string RemarkURL string @@ -57,33 +54,13 @@ func (p *Avatar) Put(u store.User) (avatarURL string, err error) { return "", errors.Errorf("failed to get avatar from the orig, status %s", resp.Status) } - // get ID and location of locally cached avatar - encID := store.EncodeID(u.ID) - location := p.location(encID) // location adds partition to path - - if _, err = os.Stat(location); os.IsNotExist(err) { - if e := os.Mkdir(location, 0700); e != nil { - return "", errors.Wrapf(e, "failed to mkdir avatar location %s", location) - } - } - - avFile := path.Join(location, encID+imgSfx) - fh, err := os.Create(avFile) + avatar, err := p.Store.Put(u.ID, resp.Body) if err != nil { - return "", errors.Wrapf(err, "can't create file %s", avFile) - } - defer func() { - if e := fh.Close(); e != nil { - log.Printf("[WARN] can't close avatar file %s, %s", avFile, e) - } - }() - - if _, err = io.Copy(fh, resp.Body); err != nil { - return "", errors.Wrapf(err, "can't save file %s", avFile) + return "", err } - log.Printf("[DEBUG] saved avatar from %s to %s, user %q", u.Picture, avFile, u.Name) - return p.RemarkURL + p.RoutePath + "/" + encID + imgSfx, nil + log.Printf("[DEBUG] saved avatar from %s to %s, user %q", u.Picture, avatar, u.Name) + return p.RemarkURL + p.RoutePath + "/" + avatar, nil } // Routes returns auth routes for given provider @@ -107,39 +84,25 @@ func (p *Avatar) Routes(middlewares ...func(http.Handler) http.Handler) (string, } } - location := p.location(strings.TrimSuffix(avatar, imgSfx)) - avFile := path.Join(location, avatar) - fh, err := os.Open(avFile) + avReader, size, err := p.Store.Get(avatar) if err != nil { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't load avatar") return } defer func() { - if e := fh.Close(); e != nil { - log.Printf("[WARN] can't close avatar file %s, %s", avFile, e) + if e := avReader.Close(); e != nil { + log.Printf("[WARN] can't close avatar reader for %s, %s", avatar, e) } }() w.Header().Set("Content-Type", "image/*") - if fi, e := fh.Stat(); e == nil { - w.Header().Set("Content-Length", strconv.Itoa(int(fi.Size()))) - } + w.Header().Set("Content-Length", strconv.Itoa(size)) w.WriteHeader(http.StatusOK) - if _, err = io.Copy(w, fh); err != nil { + if _, err = io.Copy(w, avReader); err != nil { log.Printf("[WARN] can't send response to %s, %s", r.RemoteAddr, err) } }) return p.RoutePath, router } - -// get location (directory) for user id by adding partition to final path in order to keep files -// in different subdirectories and avoid too many files in a single place. -// the end result is a full path like this - /tmp/avatars.test/92 -func (p *Avatar) location(id string) string { - p.once.Do(func() { p.ctcTable = crc64.MakeTable(crc64.ECMA) }) - checksum64 := crc64.Checksum([]byte(id), p.ctcTable) - partition := checksum64 % 100 - return path.Join(p.StorePath, fmt.Sprintf("%02d", partition)) -} diff --git a/app/rest/proxy/avatar_store.go b/app/rest/proxy/avatar_store.go new file mode 100644 index 00000000..2db744ea --- /dev/null +++ b/app/rest/proxy/avatar_store.go @@ -0,0 +1,87 @@ +package proxy + +import ( + "fmt" + "hash/crc64" + "io" + "log" + "os" + "path" + "strings" + "sync" + + "github.com/pkg/errors" + + "github.com/umputun/remark/app/store" +) + +// AvatarStore defines interface to store and serve avatars +type AvatarStore interface { + Put(userID string, reader io.Reader) (avatarURL string, err error) + Get(userID string) (reader io.ReadCloser, size int, err error) +} + +// FSAvatarStore implements AvatarStore for local file system +type FSAvatarStore struct { + storePath string + ctcTable *crc64.Table + once sync.Once +} + +// NewFSAvatarStore makes file-system avatar store +func NewFSAvatarStore(storePath string) *FSAvatarStore { + return &FSAvatarStore{storePath: storePath} +} + +// Put avatar for userID to file and return avatar name +func (fs *FSAvatarStore) Put(userID string, reader io.Reader) (avatar string, err error) { + + id := store.EncodeID(userID) + location := fs.location(id) // location adds partition to path + + if _, err = os.Stat(location); os.IsNotExist(err) { + if e := os.Mkdir(location, 0700); e != nil { + return "", errors.Wrapf(e, "failed to mkdir avatar location %s", location) + } + } + + avFile := path.Join(location, id+imgSfx) + fh, err := os.Create(avFile) + if err != nil { + return "", errors.Wrapf(err, "can't create file %s", avFile) + } + defer func() { + if e := fh.Close(); e != nil { + log.Printf("[WARN] can't close avatar file %s, %s", avFile, e) + } + }() + + if _, err = io.Copy(fh, reader); err != nil { + return "", errors.Wrapf(err, "can't save file %s", avFile) + } + return id + imgSfx, nil +} + +// Get avatar reader for avatar id.image +func (fs *FSAvatarStore) Get(avatar string) (reader io.ReadCloser, size int, err error) { + location := fs.location(strings.TrimSuffix(avatar, imgSfx)) + avFile := path.Join(location, avatar) + fh, err := os.Open(avFile) + if err != nil { + return nil, 0, errors.Wrapf(err, "can't load avatar %s, id") + } + if fi, e := fh.Stat(); e == nil { + size = int(fi.Size()) + } + return fh, size, nil +} + +// get location (directory) for user id by adding partition to final path in order to keep files +// in different subdirectories and avoid too many files in a single place. +// the end result is a full path like this - /tmp/avatars.test/92 +func (fs *FSAvatarStore) location(id string) string { + fs.once.Do(func() { fs.ctcTable = crc64.MakeTable(crc64.ECMA) }) + checksum64 := crc64.Checksum([]byte(id), fs.ctcTable) + partition := checksum64 % 100 + return path.Join(fs.storePath, fmt.Sprintf("%02d", partition)) +} diff --git a/app/rest/proxy/avatar_store_test.go b/app/rest/proxy/avatar_store_test.go new file mode 100644 index 00000000..ba01e24c --- /dev/null +++ b/app/rest/proxy/avatar_store_test.go @@ -0,0 +1,65 @@ +package proxy + +import ( + "io/ioutil" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAvatarStore_Put(t *testing.T) { + p := NewFSAvatarStore("/tmp/avatars.test") + os.MkdirAll("/tmp/avatars.test", 0700) + defer os.RemoveAll("/tmp/avatars.test") + + avatar, err := p.Put("user1", strings.NewReader("some picture bin data")) + require.Nil(t, err) + assert.Equal(t, "b3daa77b4c04a9551b8781d03191fe098f325e67.image", avatar) + fi, err := os.Stat("/tmp/avatars.test/30/b3daa77b4c04a9551b8781d03191fe098f325e67.image") + assert.NoError(t, err) + assert.Equal(t, int64(21), fi.Size()) + + avatar, err = p.Put("user2", strings.NewReader("some picture bin data 123")) + require.Nil(t, err) + assert.Equal(t, "a1881c06eec96db9901c7bbfe41c42a3f08e9cb4.image", avatar) + fi, err = os.Stat("/tmp/avatars.test/84/a1881c06eec96db9901c7bbfe41c42a3f08e9cb4.image") + assert.NoError(t, err) + assert.Equal(t, int64(25), fi.Size()) + + p = NewFSAvatarStore("/dev/null") + _, err = p.Put("user1", strings.NewReader("some picture bin data")) + assert.EqualError(t, err, "can't create file /dev/null/30/b3daa77b4c04a9551b8781d03191fe098f325e67.image: open /dev/null/30/b3daa77b4c04a9551b8781d03191fe098f325e67.image: not a directory") +} + +func TestAvatarStore_Get(t *testing.T) { + p := NewFSAvatarStore("/tmp/avatars.test") + os.MkdirAll("/tmp/avatars.test/30", 0700) + defer os.RemoveAll("/tmp/avatars.test") + ioutil.WriteFile("/tmp/avatars.test/30/b3daa77b4c04a9551b8781d03191fe098f325e67.image", []byte("something"), 0666) + r, size, err := p.Get("b3daa77b4c04a9551b8781d03191fe098f325e67.image") + assert.Nil(t, err) + assert.Equal(t, 9, size) + data, err := ioutil.ReadAll(r) + assert.Nil(t, err) + assert.Equal(t, "something", string(data)) +} + +func TestAvatarStore_Location(t *testing.T) { + p := NewFSAvatarStore("/tmp/avatars.test") + + tbl := []struct { + id string + res string + }{ + {"abc", "/tmp/avatars.test/35"}, + {"xyz", "/tmp/avatars.test/69"}, + {"blah blah", "/tmp/avatars.test/29"}, + } + + for i, tt := range tbl { + assert.Equal(t, tt.res, p.location(tt.id), "test #%d", i) + } +} diff --git a/app/rest/proxy/avatar_test.go b/app/rest/proxy/avatar_test.go index cbb7850c..8da97a92 100644 --- a/app/rest/proxy/avatar_test.go +++ b/app/rest/proxy/avatar_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/app/store" ) @@ -27,7 +28,7 @@ func TestAvatar_Put(t *testing.T) { })) defer ts.Close() - p := Avatar{StorePath: "/tmp/avatars.test", RoutePath: "/avatar", RemarkURL: "http://localhost:8080"} + p := Avatar{RoutePath: "/avatar", RemarkURL: "http://localhost:8080", Store: NewFSAvatarStore("/tmp/avatars.test")} os.MkdirAll("/tmp/avatars.test", 0700) defer os.RemoveAll("/tmp/avatars.test") @@ -56,7 +57,8 @@ func TestAvatar_PutFailed(t *testing.T) { })) defer ts.Close() - p := Avatar{StorePath: "/tmp/avatars.test", RoutePath: "/avatar"} + p := Avatar{RoutePath: "/avatar", Store: NewFSAvatarStore("/tmp/avatars.test")} + u := store.User{ID: "user1", Name: "user1 name"} _, err := p.Put(u) assert.EqualError(t, err, "no picture for user1") @@ -85,7 +87,7 @@ func TestAvatar_Routes(t *testing.T) { })) defer ts.Close() - p := Avatar{StorePath: "/tmp/avatars.test", RoutePath: "/avatar"} + p := Avatar{RoutePath: "/avatar", Store: NewFSAvatarStore("/tmp/avatars.test")} os.MkdirAll("/tmp/avatars.test", 0700) defer os.RemoveAll("/tmp/avatars.test") @@ -116,20 +118,3 @@ func TestAvatar_Routes(t *testing.T) { assert.Equal(t, int64(21), sz) assert.Equal(t, "some picture bin data", bb.String()) } - -func TestAvatar_Location(t *testing.T) { - p := Avatar{StorePath: "/tmp/avatars.test"} - - tbl := []struct { - id string - res string - }{ - {"abc", "/tmp/avatars.test/35"}, - {"xyz", "/tmp/avatars.test/69"}, - {"blah blah", "/tmp/avatars.test/29"}, - } - - for i, tt := range tbl { - assert.Equal(t, tt.res, p.location(tt.id), "test #%d", i) - } -}