MM 16523 Migrate "LinkMetadata.Save" to Sync by default (#11416)

* Update LinkMetadata.Save store and interface

* Update mocks with make store-mocks

* Update tests and implementation

app/post_metadata.go

@@ -450,9 +450,9 @@ func (a *App) saveLinkMetadataToDatabase(requestURL string, timestamp int64, og
 		metadata.Type = model.LINK_METADATA_TYPE_NONE
 	}
 
-	result := <-a.Srv.Store.LinkMetadata().Save(metadata)
+	_, err := a.Srv.Store.LinkMetadata().Save(metadata)
-	if result.Err != nil {
+	if err != nil {
-		mlog.Warn("Failed to write link metadata", mlog.String("request_url", requestURL), mlog.Err(result.Err))
+		mlog.Warn("Failed to write link metadata", mlog.String("request_url", requestURL), mlog.Err(err))
 	}
 }
 

store/sqlstore/link_metadata_store.go

@@ -36,22 +36,19 @@ func (s SqlLinkMetadataStore) CreateIndexesIfNotExists() {
 	}
 }
 
-func (s SqlLinkMetadataStore) Save(metadata *model.LinkMetadata) store.StoreChannel {
+func (s SqlLinkMetadataStore) Save(metadata *model.LinkMetadata) (*model.LinkMetadata, *model.AppError) {
-	return store.Do(func(result *store.StoreResult) {
+	if err := metadata.IsValid(); err != nil {
-		if result.Err = metadata.IsValid(); result.Err != nil {
+		return nil, err
-			return
+	}
-		}
 
-		metadata.PreSave()
+	metadata.PreSave()
 
-		err := s.GetMaster().Insert(metadata)
+	err := s.GetMaster().Insert(metadata)
-		if err != nil && !IsUniqueConstraintError(err, []string{"PRIMARY", "linkmetadata_pkey"}) {
+	if err != nil && !IsUniqueConstraintError(err, []string{"PRIMARY", "linkmetadata_pkey"}) {
-			result.Err = model.NewAppError("SqlLinkMetadataStore.Save", "store.sql_link_metadata.save.app_error", nil, "url="+metadata.URL+", "+err.Error(), http.StatusInternalServerError)
+		return nil, model.NewAppError("SqlLinkMetadataStore.Save", "store.sql_link_metadata.save.app_error", nil, "url="+metadata.URL+", "+err.Error(), http.StatusInternalServerError)
-			return
+	}
-		}
 
-		result.Data = metadata
+	return metadata, nil
-	})
 }
 
 func (s SqlLinkMetadataStore) Get(url string, timestamp int64) (*model.LinkMetadata, *model.AppError) {

store/store.go

@@ -613,7 +613,7 @@ type GroupStore interface {
 }
 
 type LinkMetadataStore interface {
-	Save(linkMetadata *model.LinkMetadata) StoreChannel
+	Save(linkMetadata *model.LinkMetadata) (*model.LinkMetadata, *model.AppError)
 	Get(url string, timestamp int64) (*model.LinkMetadata, *model.AppError)
 }
 

store/storetest/link_metadata_store.go

@@ -38,11 +38,10 @@ func testLinkMetadataStoreSave(t *testing.T, ss store.Store) {
 			Data:      &model.PostImage{},
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		linkMetadata, err := ss.LinkMetadata().Save(metadata)
 
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
-		require.IsType(t, metadata, result.Data)
+		assert.Equal(t, *metadata, *linkMetadata)
-		assert.Equal(t, *metadata, *result.Data.(*model.LinkMetadata))
 	})
 
 	t.Run("should fail to save invalid item", func(t *testing.T) {
@@ -53,9 +52,9 @@ func testLinkMetadataStoreSave(t *testing.T, ss store.Store) {
 			Data:      nil,
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		_, err := ss.LinkMetadata().Save(metadata)
 
-		assert.NotNil(t, result.Err)
+		assert.NotNil(t, err)
 	})
 
 	t.Run("should save with duplicate URL and different timestamp", func(t *testing.T) {
@@ -66,16 +65,15 @@ func testLinkMetadataStoreSave(t *testing.T, ss store.Store) {
 			Data:      &model.PostImage{},
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		_, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
 
 		metadata.Timestamp = getNextLinkMetadataTimestamp()
 
-		result = <-ss.LinkMetadata().Save(metadata)
+		linkMetadata, err := ss.LinkMetadata().Save(metadata)
 
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
-		require.IsType(t, metadata, result.Data)
+		assert.Equal(t, *metadata, *linkMetadata)
-		assert.Equal(t, *metadata, *result.Data.(*model.LinkMetadata))
 	})
 
 	t.Run("should save with duplicate timestamp and different URL", func(t *testing.T) {
@@ -86,16 +84,15 @@ func testLinkMetadataStoreSave(t *testing.T, ss store.Store) {
 			Data:      &model.PostImage{},
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		_, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
 
 		metadata.URL = "http://example.com/another/page"
 
-		result = <-ss.LinkMetadata().Save(metadata)
+		linkMetadata, err := ss.LinkMetadata().Save(metadata)
 
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
-		require.IsType(t, metadata, result.Data)
+		assert.Equal(t, *metadata, *linkMetadata)
-		assert.Equal(t, *metadata, *result.Data.(*model.LinkMetadata))
 	})
 
 	t.Run("should not save with duplicate URL and timestamp, but should not return an error", func(t *testing.T) {
@@ -106,20 +103,20 @@ func testLinkMetadataStoreSave(t *testing.T, ss store.Store) {
 			Data:      &model.PostImage{},
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		linkMetadata, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
-		assert.Equal(t, &model.PostImage{}, result.Data.(*model.LinkMetadata).Data)
+		assert.Equal(t, &model.PostImage{}, linkMetadata.Data)
 
 		metadata.Data = &model.PostImage{Height: 10, Width: 20}
 
-		result = <-ss.LinkMetadata().Save(metadata)
+		linkMetadata, err = ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
-		assert.Equal(t, result.Data.(*model.LinkMetadata).Data, &model.PostImage{Height: 10, Width: 20})
+		assert.Equal(t, linkMetadata.Data, &model.PostImage{Height: 10, Width: 20})
 
 		// Should return the original result, not the duplicate one
-		metadata, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
+		linkMetadata, err = ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
 		require.Nil(t, err)
-		assert.Equal(t, &model.PostImage{}, metadata.Data)
+		assert.Equal(t, &model.PostImage{}, linkMetadata.Data)
 	})
 }
 
@@ -132,8 +129,8 @@ func testLinkMetadataStoreGet(t *testing.T, ss store.Store) {
 			Data:      &model.PostImage{},
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		_, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
 
 		linkMetadata, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
 
@@ -150,10 +147,10 @@ func testLinkMetadataStoreGet(t *testing.T, ss store.Store) {
 			Data:      &model.PostImage{},
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		_, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
 
-		_, err := ss.LinkMetadata().Get("http://example.com/another_page", metadata.Timestamp)
+		_, err = ss.LinkMetadata().Get("http://example.com/another_page", metadata.Timestamp)
 
 		require.NotNil(t, err)
 		assert.Equal(t, http.StatusNotFound, err.StatusCode)
@@ -167,10 +164,10 @@ func testLinkMetadataStoreGet(t *testing.T, ss store.Store) {
 			Data:      &model.PostImage{},
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		_, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
 
-		_, err := ss.LinkMetadata().Get(metadata.URL, getNextLinkMetadataTimestamp())
+		_, err = ss.LinkMetadata().Get(metadata.URL, getNextLinkMetadataTimestamp())
 
 		require.NotNil(t, err)
 		assert.Equal(t, http.StatusNotFound, err.StatusCode)
@@ -189,14 +186,13 @@ func testLinkMetadataStoreTypes(t *testing.T, ss store.Store) {
 			},
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		received, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		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)))
 
-		received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
+		received, err = ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
 		require.Nil(t, err)
 
 		require.IsType(t, &model.PostImage{}, received.Data)
@@ -220,14 +216,13 @@ func testLinkMetadataStoreTypes(t *testing.T, ss store.Store) {
 			Data:      og,
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		received, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		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)))
 
-		received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
+		received, err = ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
 		require.Nil(t, err)
 
 		require.IsType(t, &opengraph.OpenGraph{}, received.Data)
@@ -242,13 +237,11 @@ func testLinkMetadataStoreTypes(t *testing.T, ss store.Store) {
 			Data:      nil,
 		}
 
-		result := <-ss.LinkMetadata().Save(metadata)
+		received, err := ss.LinkMetadata().Save(metadata)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
-
-		received := result.Data.(*model.LinkMetadata)
 		assert.Nil(t, received.Data)
 
-		received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
+		received, err = ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
 		require.Nil(t, err)
 
 		require.Nil(t, received.Data)

store/storetest/mocks/LinkMetadataStore.go

@@ -6,7 +6,6 @@ package mocks
 
 import mock "github.com/stretchr/testify/mock"
 import model "github.com/mattermost/mattermost-server/model"
-import store "github.com/mattermost/mattermost-server/store"
 
 // LinkMetadataStore is an autogenerated mock type for the LinkMetadataStore type
 type LinkMetadataStore struct {
@@ -39,17 +38,26 @@ func (_m *LinkMetadataStore) Get(url string, timestamp int64) (*model.LinkMetada
 }
 
 // Save provides a mock function with given fields: linkMetadata
-func (_m *LinkMetadataStore) Save(linkMetadata *model.LinkMetadata) store.StoreChannel {
+func (_m *LinkMetadataStore) Save(linkMetadata *model.LinkMetadata) (*model.LinkMetadata, *model.AppError) {
 	ret := _m.Called(linkMetadata)
 
-	var r0 store.StoreChannel
+	var r0 *model.LinkMetadata
-	if rf, ok := ret.Get(0).(func(*model.LinkMetadata) store.StoreChannel); ok {
+	if rf, ok := ret.Get(0).(func(*model.LinkMetadata) *model.LinkMetadata); ok {
 		r0 = rf(linkMetadata)
 	} 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(*model.LinkMetadata) *model.AppError); ok {
+		r1 = rf(linkMetadata)
+	} else {
+		if ret.Get(1) != nil {
+			r1 = ret.Get(1).(*model.AppError)
+		}
+	}
+
+	return r0, r1
 }