[MI-3591] Add transactions in the functions to avoid race conditions #344
Conversation
…s-sync into MI-3591
| @@ -207,15 +207,35 @@ func (p *Plugin) executeLinkCommand(args *model.CommandArgs, parameters []string | |||
| return p.cmdError(args.UserId, args.ChannelId, "Unable to create new link.") | |||
| } | |||
|
|
|||
| err = p.store.SaveChannelSubscription(storemodels.ChannelSubscription{ | |||
| tx, err := p.store.BeginTx() | |||
There was a problem hiding this comment.
I don't like this BeginTx here exposed outside the store. I think it's better to have the transactions inside the store. Transactions are an storage concern, no an application concern. So the right level of abstraction is at the store. Specially in this case where you are not doing more than one store call. Why is this transaction needed at all here, and why is not inside the SaveChannelSubscrition method?
| @@ -403,72 +408,125 @@ func (s *SQLStore) MattermostToTeamsUserID(userID string) (string, error) { | |||
| } | |||
|
|
|||
| func (s *SQLStore) GetPostInfoByMSTeamsID(chatID string, postID string) (*storemodels.PostInfo, error) { | |||
| query := s.getQueryBuilder().Select("mmPostID, msTeamsLastUpdateAt").From("msteamssync_posts").Where(sq.Eq{"msTeamsPostID": postID, "msTeamsChannelID": chatID}) | |||
| tx, err := s.BeginTx() | |||
There was a problem hiding this comment.
Why do we need a transaction here? I think there is no point on having here a transaction, there is an automatically generated transaction by the fact that you are running a query. And you are running a single query here, so you don't need to generate a transaction. Specially because this single query is a Select.
| } | ||
| postInfo.MSTeamsLastUpdateAt = time.UnixMicro(lastUpdateAt) | ||
| return &postInfo, nil | ||
| } | ||
|
|
||
| func (s *SQLStore) GetPostInfoByMattermostID(postID string) (*storemodels.PostInfo, error) { | ||
| query := s.getQueryBuilder().Select("msTeamsPostID, msTeamsChannelID, msTeamsLastUpdateAt").From("msteamssync_posts").Where(sq.Eq{"mmPostID": postID}) | ||
| tx, err := s.BeginTx() |
There was a problem hiding this comment.
Same here, and probably most of the cases in this file. The transaction is only needed when you operate with multiple queries, and any of those queries is a modification query.
| postInfo.MSTeamsLastUpdateAt = time.UnixMicro(lastUpdateAt) | ||
| return &postInfo, nil | ||
| } | ||
|
|
||
| func (s *SQLStore) SetPostLastUpdateAtByMattermostID(postID string, lastUpdateAt time.Time) error { | ||
| query := s.getQueryBuilder().Update("msteamssync_posts").Set("msTeamsLastUpdateAt", lastUpdateAt.UnixMicro()).Where(sq.Eq{"mmPostID": postID}) | ||
| func (s *SQLStore) SetPostLastUpdateAtByMattermostID(postID string, lastUpdateAt time.Time, tx *sql.Tx) error { |
There was a problem hiding this comment.
This is more a matter of style, I would use the tx as the first parameter if we want to pass the tx to the store methods. Because my expectation is that every query has the tx parameter at the beginning. but that is not the case here. Anyway, I would love to hide all the transaction complexity from the rest of the application. For example, not using transactions where there are not needed, and where they are needed use something special, like a "two steps function", something like a function that starts the operation, and returns a function that finish (or rollback) the operation depending on the parameter passed (maybe something like err == nil).
ayusht2810
added
Difficulty/3:Hard
Summary
Add transactions in the functions to avoid race conditions
Ticket Link
#327
#330
Duplicate of #340