From df9af66a28663df929cc0344b3f9e250c00d6344 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Etienne?= Date: Wed, 26 Jun 2019 14:36:29 +0100 Subject: [PATCH] MM-16524 Migrate "LinkMetadata.Get" to Sync by default (#11376) * Update LinkMetadata.Get store and interface * Update mocks with make store-mocks * Update tests and implementation * Avoid casting LinkMetadata.Data * Fix indent * Make error more explicit * Fix indent (was containing spaces) * Test value returned by LinkMetadata().Get() --- app/post_metadata.go | 6 +-- store/sqlstore/link_metadata_store.go | 47 +++++++++------------- store/store.go | 2 +- store/storetest/link_metadata_store.go | 41 +++++++++---------- store/storetest/mocks/LinkMetadataStore.go | 19 ++++++--- 5 files changed, 57 insertions(+), 58 deletions(-) diff --git a/app/post_metadata.go b/app/post_metadata.go index a877cf216e..941aa9f85a 100644 --- a/app/post_metadata.go +++ b/app/post_metadata.go @@ -417,12 +417,12 @@ func getLinkMetadataFromCache(requestURL string, timestamp int64) (*opengraph.Op } func (a *App) getLinkMetadataFromDatabase(requestURL string, timestamp int64) (*opengraph.OpenGraph, *model.PostImage, bool) { - result := <-a.Srv.Store.LinkMetadata().Get(requestURL, timestamp) - if result.Err != nil { + linkMetadata, err := a.Srv.Store.LinkMetadata().Get(requestURL, timestamp) + if err != nil { return nil, nil, false } - data := result.Data.(*model.LinkMetadata).Data + data := linkMetadata.Data switch v := data.(type) { case *opengraph.OpenGraph: diff --git a/store/sqlstore/link_metadata_store.go b/store/sqlstore/link_metadata_store.go index c9f2487d00..96ce3715bf 100644 --- a/store/sqlstore/link_metadata_store.go +++ b/store/sqlstore/link_metadata_store.go @@ -54,35 +54,28 @@ func (s SqlLinkMetadataStore) Save(metadata *model.LinkMetadata) store.StoreChan }) } -func (s SqlLinkMetadataStore) Get(url string, timestamp int64) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - var metadata *model.LinkMetadata +func (s SqlLinkMetadataStore) Get(url string, timestamp int64) (*model.LinkMetadata, *model.AppError) { + var metadata *model.LinkMetadata - err := s.GetReplica().SelectOne(&metadata, - `SELECT - * - FROM - LinkMetadata - WHERE - URL = :URL - AND Timestamp = :Timestamp`, map[string]interface{}{"URL": url, "Timestamp": timestamp}) - if err != nil { - result.Err = model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusInternalServerError) - - if err == sql.ErrNoRows { - result.Err.StatusCode = http.StatusNotFound - } - - return + err := s.GetReplica().SelectOne(&metadata, + `SELECT + * + FROM + LinkMetadata + WHERE + URL = :URL + AND Timestamp = :Timestamp`, map[string]interface{}{"URL": url, "Timestamp": timestamp}) + if err != nil { + if err == sql.ErrNoRows { + return nil, model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusNotFound) } + return nil, model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusInternalServerError) + } - err = metadata.DeserializeDataToConcreteType() - if err != nil { - result.Err = model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusInternalServerError) + err = metadata.DeserializeDataToConcreteType() + if err != nil { + return nil, model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusInternalServerError) + } - return - } - - result.Data = metadata - }) + return metadata, nil } diff --git a/store/store.go b/store/store.go index a61fd16b2d..04f9f7fc24 100644 --- a/store/store.go +++ b/store/store.go @@ -614,7 +614,7 @@ type GroupStore interface { type LinkMetadataStore interface { Save(linkMetadata *model.LinkMetadata) StoreChannel - Get(url string, timestamp int64) StoreChannel + Get(url string, timestamp int64) (*model.LinkMetadata, *model.AppError) } // ChannelSearchOpts contains options for searching channels. diff --git a/store/storetest/link_metadata_store.go b/store/storetest/link_metadata_store.go index 354d315998..b8a4e302e6 100644 --- a/store/storetest/link_metadata_store.go +++ b/store/storetest/link_metadata_store.go @@ -117,9 +117,9 @@ func testLinkMetadataStoreSave(t *testing.T, ss store.Store) { assert.Equal(t, result.Data.(*model.LinkMetadata).Data, &model.PostImage{Height: 10, Width: 20}) // Should return the original result, not the duplicate one - result = <-ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) - require.Nil(t, result.Err) - assert.Equal(t, &model.PostImage{}, result.Data.(*model.LinkMetadata).Data) + metadata, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) + require.Nil(t, err) + assert.Equal(t, &model.PostImage{}, metadata.Data) }) } @@ -135,11 +135,11 @@ func testLinkMetadataStoreGet(t *testing.T, ss store.Store) { result := <-ss.LinkMetadata().Save(metadata) require.Nil(t, result.Err) - result = <-ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) + linkMetadata, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) - require.Nil(t, result.Err) - require.IsType(t, metadata, result.Data) - assert.Equal(t, *metadata, *result.Data.(*model.LinkMetadata)) + require.Nil(t, err) + require.IsType(t, metadata, linkMetadata) + assert.Equal(t, *metadata, *linkMetadata) }) t.Run("should return not found with incorrect URL", func(t *testing.T) { @@ -153,10 +153,10 @@ func testLinkMetadataStoreGet(t *testing.T, ss store.Store) { result := <-ss.LinkMetadata().Save(metadata) require.Nil(t, result.Err) - result = <-ss.LinkMetadata().Get("http://example.com/another_page", metadata.Timestamp) + _, err := ss.LinkMetadata().Get("http://example.com/another_page", metadata.Timestamp) - require.NotNil(t, result.Err) - assert.Equal(t, http.StatusNotFound, result.Err.StatusCode) + require.NotNil(t, err) + assert.Equal(t, http.StatusNotFound, err.StatusCode) }) t.Run("should return not found with incorrect timestamp", func(t *testing.T) { @@ -170,10 +170,10 @@ func testLinkMetadataStoreGet(t *testing.T, ss store.Store) { result := <-ss.LinkMetadata().Save(metadata) require.Nil(t, result.Err) - result = <-ss.LinkMetadata().Get(metadata.URL, getNextLinkMetadataTimestamp()) + _, err := ss.LinkMetadata().Get(metadata.URL, getNextLinkMetadataTimestamp()) - require.NotNil(t, result.Err) - assert.Equal(t, http.StatusNotFound, result.Err.StatusCode) + require.NotNil(t, err) + assert.Equal(t, http.StatusNotFound, err.StatusCode) }) } @@ -196,10 +196,9 @@ func testLinkMetadataStoreTypes(t *testing.T, ss store.Store) { require.IsType(t, &model.PostImage{}, received.Data) assert.Equal(t, *(metadata.Data.(*model.PostImage)), *(received.Data.(*model.PostImage))) - result = <-ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) - require.Nil(t, result.Err) + received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) + require.Nil(t, err) - received = result.Data.(*model.LinkMetadata) require.IsType(t, &model.PostImage{}, received.Data) assert.Equal(t, *(metadata.Data.(*model.PostImage)), *(received.Data.(*model.PostImage))) }) @@ -228,10 +227,9 @@ func testLinkMetadataStoreTypes(t *testing.T, ss store.Store) { require.IsType(t, &opengraph.OpenGraph{}, received.Data) assert.Equal(t, *(metadata.Data.(*opengraph.OpenGraph)), *(received.Data.(*opengraph.OpenGraph))) - result = <-ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) - require.Nil(t, result.Err) + received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) + require.Nil(t, err) - received = result.Data.(*model.LinkMetadata) require.IsType(t, &opengraph.OpenGraph{}, received.Data) assert.Equal(t, *(metadata.Data.(*opengraph.OpenGraph)), *(received.Data.(*opengraph.OpenGraph))) }) @@ -250,10 +248,9 @@ func testLinkMetadataStoreTypes(t *testing.T, ss store.Store) { received := result.Data.(*model.LinkMetadata) assert.Nil(t, received.Data) - result = <-ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) - require.Nil(t, result.Err) + received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp) + require.Nil(t, err) - received = result.Data.(*model.LinkMetadata) require.Nil(t, received.Data) }) } diff --git a/store/storetest/mocks/LinkMetadataStore.go b/store/storetest/mocks/LinkMetadataStore.go index d018d6ab08..f1a56fde8c 100644 --- a/store/storetest/mocks/LinkMetadataStore.go +++ b/store/storetest/mocks/LinkMetadataStore.go @@ -14,19 +14,28 @@ type LinkMetadataStore struct { } // Get provides a mock function with given fields: url, timestamp -func (_m *LinkMetadataStore) Get(url string, timestamp int64) store.StoreChannel { +func (_m *LinkMetadataStore) Get(url string, timestamp int64) (*model.LinkMetadata, *model.AppError) { ret := _m.Called(url, timestamp) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, int64) store.StoreChannel); ok { + var r0 *model.LinkMetadata + if rf, ok := ret.Get(0).(func(string, int64) *model.LinkMetadata); ok { r0 = rf(url, timestamp) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.LinkMetadata) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, int64) *model.AppError); ok { + r1 = rf(url, timestamp) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // Save provides a mock function with given fields: linkMetadata