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

@@ -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)
+	linkMetadata, err := a.Srv.Store.LinkMetadata().Get(requestURL, timestamp)
-	if result.Err != nil {
+	if err != nil {
 		return nil, nil, false
 	}
 
-	data := result.Data.(*model.LinkMetadata).Data
+	data := linkMetadata.Data
 
 	switch v := data.(type) {
 	case *opengraph.OpenGraph:

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 {
+func (s SqlLinkMetadataStore) Get(url string, timestamp int64) (*model.LinkMetadata, *model.AppError) {
-	return store.Do(func(result *store.StoreResult) {
+	var metadata *model.LinkMetadata
-		var metadata *model.LinkMetadata
 
-		err := s.GetReplica().SelectOne(&metadata,
+	err := s.GetReplica().SelectOne(&metadata,
-			`SELECT
+		`SELECT
-				*
+			*
-			FROM
+		FROM
-				LinkMetadata
+			LinkMetadata
-			WHERE
+		WHERE
-				URL = :URL
+			URL = :URL
-				AND Timestamp = :Timestamp`, map[string]interface{}{"URL": url, "Timestamp": timestamp})
+			AND Timestamp = :Timestamp`, map[string]interface{}{"URL": url, "Timestamp": timestamp})
-		if err != nil {
+	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 {
-
+			return nil, model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusNotFound)
-			if err == sql.ErrNoRows {
-				result.Err.StatusCode = http.StatusNotFound
-			}
-
-			return
 		}
+		return nil, model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusInternalServerError)
+	}
 
-		err = metadata.DeserializeDataToConcreteType()
+	err = metadata.DeserializeDataToConcreteType()
-		if err != nil {
+	if err != nil {
-			result.Err = model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusInternalServerError)
+		return nil, model.NewAppError("SqlLinkMetadataStore.Get", "store.sql_link_metadata.get.app_error", nil, "url="+url+", "+err.Error(), http.StatusInternalServerError)
+	}
 
-			return
+	return metadata, nil
-		}
-
-		result.Data = metadata
-	})
 }

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.

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)
+		metadata, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
-		assert.Equal(t, &model.PostImage{}, result.Data.(*model.LinkMetadata).Data)
+		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.Nil(t, err)
-		require.IsType(t, metadata, result.Data)
+		require.IsType(t, metadata, linkMetadata)
-		assert.Equal(t, *metadata, *result.Data.(*model.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)
+		require.NotNil(t, err)
-		assert.Equal(t, http.StatusNotFound, result.Err.StatusCode)
+		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)
+		require.NotNil(t, err)
-		assert.Equal(t, http.StatusNotFound, result.Err.StatusCode)
+		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)
+		received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
-		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)))
 	})
@@ -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)
+		received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
-		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)))
 	})
@@ -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)
+		received, err := ss.LinkMetadata().Get(metadata.URL, metadata.Timestamp)
-		require.Nil(t, result.Err)
+		require.Nil(t, err)
 
-		received = result.Data.(*model.LinkMetadata)
 		require.Nil(t, received.Data)
 	})
 }

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
+	var r0 *model.LinkMetadata
-	if rf, ok := ret.Get(0).(func(string, int64) store.StoreChannel); ok {
+	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