From e1b0644b0db02f3ec6b238b96199737951ee4755 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 12 Aug 2021 20:15:03 +0530 Subject: [PATCH] Using native JSON operations to add thread participants (#18093) Automatic Merge --- store/sqlstore/adapters.go | 138 ++++++++++++++++++++++++++++++++ store/sqlstore/adapters_test.go | 21 +++++ store/sqlstore/store.go | 98 ----------------------- store/sqlstore/thread_store.go | 24 ++++-- store/storetest/thread_store.go | 14 +++- 5 files changed, 188 insertions(+), 107 deletions(-) create mode 100644 store/sqlstore/adapters.go create mode 100644 store/sqlstore/adapters_test.go diff --git a/store/sqlstore/adapters.go b/store/sqlstore/adapters.go new file mode 100644 index 0000000000..b19b60e27d --- /dev/null +++ b/store/sqlstore/adapters.go @@ -0,0 +1,138 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package sqlstore + +import ( + "bytes" + "database/sql/driver" + "encoding/json" + "fmt" + "strconv" + "strings" + + "github.com/dyatlov/go-opengraph/opengraph" + "github.com/mattermost/gorp" + "github.com/mattermost/mattermost-server/v6/model" + "github.com/mattermost/mattermost-server/v6/shared/i18n" + "github.com/mattermost/mattermost-server/v6/shared/mlog" + "github.com/pkg/errors" +) + +type jsonArray []string + +func (a jsonArray) Value() (driver.Value, error) { + var out bytes.Buffer + if err := out.WriteByte('['); err != nil { + return nil, err + } + + for i, item := range a { + if _, err := out.WriteString(strconv.Quote(item)); err != nil { + return nil, err + } + // Skip the last element. + if i < len(a)-1 { + out.WriteByte(',') + } + } + + if err := out.WriteByte(']'); err != nil { + return nil, err + } + return out.Bytes(), nil +} + +type TraceOnAdapter struct{} + +func (t *TraceOnAdapter) Printf(format string, v ...interface{}) { + originalString := fmt.Sprintf(format, v...) + newString := strings.ReplaceAll(originalString, "\n", " ") + newString = strings.ReplaceAll(newString, "\t", " ") + newString = strings.ReplaceAll(newString, "\"", "") + mlog.Debug(newString) +} + +type JSONSerializable interface { + ToJson() string +} + +type mattermConverter struct{} + +func (me mattermConverter) ToDb(val interface{}) (interface{}, error) { + switch t := val.(type) { + case model.StringMap: + return model.MapToJson(t), nil + case map[string]string: + return model.MapToJson(model.StringMap(t)), nil + case model.StringArray: + return model.ArrayToJson(t), nil + case model.StringInterface: + return model.StringInterfaceToJson(t), nil + case map[string]interface{}: + return model.StringInterfaceToJson(model.StringInterface(t)), nil + case JSONSerializable: + return t.ToJson(), nil + case *opengraph.OpenGraph: + return json.Marshal(t) + } + + return val, nil +} + +func (me mattermConverter) FromDb(target interface{}) (gorp.CustomScanner, bool) { + switch target.(type) { + case *model.StringMap: + binder := func(holder, target interface{}) error { + s, ok := holder.(*string) + if !ok { + return errors.New(i18n.T("store.sql.convert_string_map")) + } + b := []byte(*s) + return json.Unmarshal(b, target) + } + return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true + case *map[string]string: + binder := func(holder, target interface{}) error { + s, ok := holder.(*string) + if !ok { + return errors.New(i18n.T("store.sql.convert_string_map")) + } + b := []byte(*s) + return json.Unmarshal(b, target) + } + return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true + case *model.StringArray: + binder := func(holder, target interface{}) error { + s, ok := holder.(*string) + if !ok { + return errors.New(i18n.T("store.sql.convert_string_array")) + } + b := []byte(*s) + return json.Unmarshal(b, target) + } + return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true + case *model.StringInterface: + binder := func(holder, target interface{}) error { + s, ok := holder.(*string) + if !ok { + return errors.New(i18n.T("store.sql.convert_string_interface")) + } + b := []byte(*s) + return json.Unmarshal(b, target) + } + return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true + case *map[string]interface{}: + binder := func(holder, target interface{}) error { + s, ok := holder.(*string) + if !ok { + return errors.New(i18n.T("store.sql.convert_string_interface")) + } + b := []byte(*s) + return json.Unmarshal(b, target) + } + return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true + } + + return gorp.CustomScanner{}, false +} diff --git a/store/sqlstore/adapters_test.go b/store/sqlstore/adapters_test.go new file mode 100644 index 0000000000..f95fb0f0f1 --- /dev/null +++ b/store/sqlstore/adapters_test.go @@ -0,0 +1,21 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package sqlstore + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestJSONArray(t *testing.T) { + input := []string{"a", "b"} + + out, err := jsonArray(input).Value() + require.NoError(t, err) + outBuf, ok := out.([]byte) + require.True(t, ok) + assert.Equal(t, []byte(`["a","b"]`), outBuf) +} diff --git a/store/sqlstore/store.go b/store/sqlstore/store.go index eb452655c9..31b636a741 100644 --- a/store/sqlstore/store.go +++ b/store/sqlstore/store.go @@ -6,7 +6,6 @@ package sqlstore import ( "context" dbsql "database/sql" - "encoding/json" "fmt" "os" "path/filepath" @@ -17,7 +16,6 @@ import ( "time" sq "github.com/Masterminds/squirrel" - "github.com/dyatlov/go-opengraph/opengraph" "github.com/go-sql-driver/mysql" "github.com/golang-migrate/migrate/v4" "github.com/golang-migrate/migrate/v4/database" @@ -33,7 +31,6 @@ import ( "github.com/mattermost/mattermost-server/v6/db/migrations" "github.com/mattermost/mattermost-server/v6/einterfaces" "github.com/mattermost/mattermost-server/v6/model" - "github.com/mattermost/mattermost-server/v6/shared/i18n" "github.com/mattermost/mattermost-server/v6/shared/mlog" "github.com/mattermost/mattermost-server/v6/store" ) @@ -155,22 +152,12 @@ type SqlStore struct { metrics einterfaces.MetricsInterface } -type TraceOnAdapter struct{} - // ColumnInfo holds information about a column. type ColumnInfo struct { DataType string CharMaximumLength int } -func (t *TraceOnAdapter) Printf(format string, v ...interface{}) { - originalString := fmt.Sprintf(format, v...) - newString := strings.ReplaceAll(originalString, "\n", " ") - newString = strings.ReplaceAll(newString, "\t", " ") - newString = strings.ReplaceAll(newString, "\"", "") - mlog.Debug(newString) -} - func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlStore { store := &SqlStore{ rrCounter: 0, @@ -1567,91 +1554,6 @@ func resetReadTimeout(dataSource string) (string, error) { return config.FormatDSN(), nil } -type mattermConverter struct{} - -func (me mattermConverter) ToDb(val interface{}) (interface{}, error) { - - switch t := val.(type) { - case model.StringMap: - return model.MapToJson(t), nil - case map[string]string: - return model.MapToJson(model.StringMap(t)), nil - case model.StringArray: - return model.ArrayToJson(t), nil - case model.StringInterface: - return model.StringInterfaceToJson(t), nil - case map[string]interface{}: - return model.StringInterfaceToJson(model.StringInterface(t)), nil - case JSONSerializable: - return t.ToJson(), nil - case *opengraph.OpenGraph: - return json.Marshal(t) - } - - return val, nil -} - -func (me mattermConverter) FromDb(target interface{}) (gorp.CustomScanner, bool) { - switch target.(type) { - case *model.StringMap: - binder := func(holder, target interface{}) error { - s, ok := holder.(*string) - if !ok { - return errors.New(i18n.T("store.sql.convert_string_map")) - } - b := []byte(*s) - return json.Unmarshal(b, target) - } - return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true - case *map[string]string: - binder := func(holder, target interface{}) error { - s, ok := holder.(*string) - if !ok { - return errors.New(i18n.T("store.sql.convert_string_map")) - } - b := []byte(*s) - return json.Unmarshal(b, target) - } - return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true - case *model.StringArray: - binder := func(holder, target interface{}) error { - s, ok := holder.(*string) - if !ok { - return errors.New(i18n.T("store.sql.convert_string_array")) - } - b := []byte(*s) - return json.Unmarshal(b, target) - } - return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true - case *model.StringInterface: - binder := func(holder, target interface{}) error { - s, ok := holder.(*string) - if !ok { - return errors.New(i18n.T("store.sql.convert_string_interface")) - } - b := []byte(*s) - return json.Unmarshal(b, target) - } - return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true - case *map[string]interface{}: - binder := func(holder, target interface{}) error { - s, ok := holder.(*string) - if !ok { - return errors.New(i18n.T("store.sql.convert_string_interface")) - } - b := []byte(*s) - return json.Unmarshal(b, target) - } - return gorp.CustomScanner{Holder: new(string), Target: target, Binder: binder}, true - } - - return gorp.CustomScanner{}, false -} - -type JSONSerializable interface { - ToJson() string -} - func convertMySQLFullTextColumnsToPostgres(columnNames string) string { columns := strings.Split(columnNames, ", ") concatenatedColumnNames := "" diff --git a/store/sqlstore/thread_store.go b/store/sqlstore/thread_store.go index 5e47bcc9e3..8b5cb7bcff 100644 --- a/store/sqlstore/thread_store.go +++ b/store/sqlstore/thread_store.go @@ -6,6 +6,7 @@ package sqlstore import ( "context" "database/sql" + "strconv" "time" sq "github.com/Masterminds/squirrel" @@ -658,14 +659,21 @@ func (s *SqlThreadStore) MaintainMembership(userId, postId string, opts store.Th } if opts.UpdateParticipants { - thread, getErr := s.get(trx, postId) - if getErr != nil { - return nil, getErr - } - if thread != nil && !thread.Participants.Contains(userId) { - thread.Participants = append(thread.Participants, userId) - if _, err = s.update(trx, thread); err != nil { - return nil, err + if s.DriverName() == model.DatabaseDriverPostgres { + if _, err2 := trx.Exec(`UPDATE Threads + SET participants = participants || $1::jsonb + WHERE postid=$2 + AND NOT participants ? $3`, jsonArray([]string{userId}), postId, userId); err2 != nil { + return nil, err2 + } + } else { + // CONCAT('$[', JSON_LENGTH(Participants), ']') just generates $[n] + // which is the positional syntax required for appending. + if _, err2 := trx.Exec(`UPDATE Threads + SET Participants = JSON_ARRAY_INSERT(Participants, CONCAT('$[', JSON_LENGTH(Participants), ']'), ?) + WHERE PostId=? + AND NOT JSON_CONTAINS(Participants, ?)`, userId, postId, strconv.Quote(userId)); err2 != nil { + return nil, err2 } } } diff --git a/store/storetest/thread_store.go b/store/storetest/thread_store.go index dd82d5a61b..983e2f8398 100644 --- a/store/storetest/thread_store.go +++ b/store/storetest/thread_store.go @@ -330,18 +330,30 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { IncrementMentions: false, UpdateFollowing: true, UpdateViewedTimestamp: false, - UpdateParticipants: false, + UpdateParticipants: true, } tm, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, opts) require.NoError(t, e) require.Equal(t, int64(0), tm.LastViewed) + // No update since array has same elements. + th, e := ss.Thread().Get(newPosts[0].Id) + require.NoError(t, e) + assert.ElementsMatch(t, model.StringArray{newPosts[0].UserId, newPosts[1].UserId}, th.Participants) + opts.UpdateViewedTimestamp = true _, e = ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, opts) require.NoError(t, e) m2, err2 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err2) require.Greater(t, m2.LastViewed, int64(0)) + + // Adding a new participant + _, e = ss.Thread().MaintainMembership("newuser", newPosts[0].Id, opts) + require.NoError(t, e) + th, e = ss.Thread().Get(newPosts[0].Id) + require.NoError(t, e) + assert.ElementsMatch(t, model.StringArray{newPosts[0].UserId, newPosts[1].UserId, "newuser"}, th.Participants) }) t.Run("Thread membership 'viewed' timestamp is updated properly for new membership", func(t *testing.T) {