diff --git a/app/diagnostics.go b/app/diagnostics.go index 3b14392b07..2673276acc 100644 --- a/app/diagnostics.go +++ b/app/diagnostics.go @@ -493,9 +493,10 @@ func (a *App) trackConfig() { }) a.SendDiagnostic(TRACK_CONFIG_EXPERIMENTAL, map[string]interface{}{ - "client_side_cert_enable": *cfg.ExperimentalSettings.ClientSideCertEnable, - "isdefault_client_side_cert_check": isDefault(*cfg.ExperimentalSettings.ClientSideCertCheck, model.CLIENT_SIDE_CERT_CHECK_PRIMARY_AUTH), - "enable_post_metadata": *cfg.ExperimentalSettings.EnablePostMetadata, + "client_side_cert_enable": *cfg.ExperimentalSettings.ClientSideCertEnable, + "isdefault_client_side_cert_check": isDefault(*cfg.ExperimentalSettings.ClientSideCertCheck, model.CLIENT_SIDE_CERT_CHECK_PRIMARY_AUTH), + "enable_post_metadata": *cfg.ExperimentalSettings.EnablePostMetadata, + "link_metadata_timeout_milliseconds": *cfg.ExperimentalSettings.LinkMetadataTimeoutMilliseconds, }) a.SendDiagnostic(TRACK_CONFIG_ANALYTICS, map[string]interface{}{ diff --git a/app/post_metadata.go b/app/post_metadata.go index 6816efbf0a..d166af94c0 100644 --- a/app/post_metadata.go +++ b/app/post_metadata.go @@ -9,6 +9,7 @@ import ( "net/http" "net/url" "strings" + "time" "github.com/dyatlov/go-opengraph/opengraph" "github.com/mattermost/mattermost-server/mlog" @@ -346,17 +347,19 @@ func (a *App) getLinkMetadata(requestURL string, timestamp int64, isNewPost bool request.Header.Add("Accept", "text/html, image/*") - res, err := a.HTTPService.MakeClient(false).Do(request) - if err != nil { - return nil, nil, err + client := a.HTTPService.MakeClient(false) + client.Timeout = time.Duration(*a.Config().ExperimentalSettings.LinkMetadataTimeoutMilliseconds) * time.Millisecond + + res, err := client.Do(request) + + if err == nil { + defer res.Body.Close() + + // Parse the data + og, image, err = a.parseLinkMetadata(requestURL, res.Body, res.Header.Get("Content-Type")) } - defer res.Body.Close() - - // Parse the data - og, image, err = a.parseLinkMetadata(requestURL, res.Body, res.Header.Get("Content-Type")) - - // Write back to cache and database + // Write back to cache and database, even if there was an error and the results are nil cacheLinkMetadata(requestURL, timestamp, og, image) a.saveLinkMetadataToDatabase(requestURL, timestamp, og, image) diff --git a/app/post_metadata_test.go b/app/post_metadata_test.go index 10488c05ab..4987a06100 100644 --- a/app/post_metadata_test.go +++ b/app/post_metadata_test.go @@ -11,6 +11,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "strconv" "strings" "testing" @@ -1038,6 +1039,15 @@ func TestGetLinkMetadata(t *testing.T) { w.Header().Set("Content-Type", "application/json") w.Write([]byte("true")) + } else if strings.HasPrefix(r.URL.Path, "/timeout") { + w.Header().Set("Content-Type", "text/html") + + w.Write([]byte("")) + select { + case <-time.After(60 * time.Second): + case <-r.Context().Done(): + } + w.Write([]byte("")) } else { w.WriteHeader(http.StatusInternalServerError) } @@ -1274,7 +1284,7 @@ func TestGetLinkMetadata(t *testing.T) { assert.Exactly(t, img, fromDatabase) }) - t.Run("should cache error results", func(t *testing.T) { + t.Run("should cache general errors", func(t *testing.T) { th := setup() defer th.TearDown() @@ -1304,6 +1314,71 @@ func TestGetLinkMetadata(t *testing.T) { assert.Nil(t, imageFromDatabase) }) + t.Run("should cache invalid URL errors", func(t *testing.T) { + th := setup() + defer th.TearDown() + + requestURL := "http://notarealdomainthatactuallyexists.ca/?name=" + t.Name() + timestamp := int64(1547510400000) + + _, _, ok := getLinkMetadataFromCache(requestURL, timestamp) + require.False(t, ok, "data should not exist in in-memory cache") + + _, _, ok = th.App.getLinkMetadataFromDatabase(requestURL, timestamp) + require.False(t, ok, "data should not exist in database") + + og, img, err := th.App.getLinkMetadata(requestURL, timestamp, false) + + assert.Nil(t, og) + assert.Nil(t, img) + assert.IsType(t, &url.Error{}, err) + + ogFromCache, imgFromCache, ok := getLinkMetadataFromCache(requestURL, timestamp) + assert.True(t, ok) + assert.Nil(t, ogFromCache) + assert.Nil(t, imgFromCache) + + ogFromDatabase, imageFromDatabase, ok := th.App.getLinkMetadataFromDatabase(requestURL, timestamp) + assert.True(t, ok) + assert.Nil(t, ogFromDatabase) + assert.Nil(t, imageFromDatabase) + }) + + t.Run("should cache timeout errors", func(t *testing.T) { + th := setup() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ExperimentalSettings.LinkMetadataTimeoutMilliseconds = 100 + }) + + requestURL := server.URL + "/timeout?name=" + t.Name() + timestamp := int64(1547510400000) + + _, _, ok := getLinkMetadataFromCache(requestURL, timestamp) + require.False(t, ok, "data should not exist in in-memory cache") + + _, _, ok = th.App.getLinkMetadataFromDatabase(requestURL, timestamp) + require.False(t, ok, "data should not exist in database") + + og, img, err := th.App.getLinkMetadata(requestURL, timestamp, false) + + assert.Nil(t, og) + assert.Nil(t, img) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "Client.Timeout") + + ogFromCache, imgFromCache, ok := getLinkMetadataFromCache(requestURL, timestamp) + assert.True(t, ok) + assert.Nil(t, ogFromCache) + assert.Nil(t, imgFromCache) + + ogFromDatabase, imageFromDatabase, ok := th.App.getLinkMetadataFromDatabase(requestURL, timestamp) + assert.True(t, ok) + assert.Nil(t, ogFromDatabase) + assert.Nil(t, imageFromDatabase) + }) + t.Run("should cache database results in memory", func(t *testing.T) { th := setup() defer th.TearDown() diff --git a/config/default.json b/config/default.json index 3d046308bb..98012a5596 100644 --- a/config/default.json +++ b/config/default.json @@ -358,7 +358,8 @@ "ExperimentalSettings": { "ClientSideCertEnable": false, "ClientSideCertCheck": "secondary", - "EnablePostMetadata": false + "EnablePostMetadata": false, + "LinkMetadataTimeoutMilliseconds": 5000 }, "AnalyticsSettings": { "MaxUsersForStatistics": 2500 @@ -417,4 +418,4 @@ "RemoteImageProxyURL": "", "RemoteImageProxyOptions": "" } -} \ No newline at end of file +} diff --git a/model/config.go b/model/config.go index 3d3dbe021b..0c043de41a 100644 --- a/model/config.go +++ b/model/config.go @@ -139,6 +139,8 @@ const ( NATIVEAPP_SETTINGS_DEFAULT_ANDROID_APP_DOWNLOAD_LINK = "https://about.mattermost.com/mattermost-android-app/" NATIVEAPP_SETTINGS_DEFAULT_IOS_APP_DOWNLOAD_LINK = "https://about.mattermost.com/mattermost-ios-app/" + EXPERIMENTAL_SETTINGS_DEFAULT_LINK_METADATA_TIMEOUT_MILLISECONDS = 5000 + ANALYTICS_SETTINGS_DEFAULT_MAX_USERS_FOR_STATISTICS = 2500 ANNOUNCEMENT_SETTINGS_DEFAULT_BANNER_COLOR = "#f2a93b" @@ -692,9 +694,10 @@ func (s *MetricsSettings) SetDefaults() { } type ExperimentalSettings struct { - ClientSideCertEnable *bool - ClientSideCertCheck *string - EnablePostMetadata *bool + ClientSideCertEnable *bool + ClientSideCertCheck *string + EnablePostMetadata *bool + LinkMetadataTimeoutMilliseconds *int64 } func (s *ExperimentalSettings) SetDefaults() { @@ -709,6 +712,10 @@ func (s *ExperimentalSettings) SetDefaults() { if s.EnablePostMetadata == nil { s.EnablePostMetadata = NewBool(false) } + + if s.LinkMetadataTimeoutMilliseconds == nil { + s.LinkMetadataTimeoutMilliseconds = NewInt64(EXPERIMENTAL_SETTINGS_DEFAULT_LINK_METADATA_TIMEOUT_MILLISECONDS) + } } type AnalyticsSettings struct {