-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix possible race condition that can lead to duplicated messages #627
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! Can we find a way to unit test this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why the <abbr>
tag is being used? Also after creation, should it be removed. Will it cause any issues if the user edits the post later?
server/handlers/handlers.go
Outdated
@@ -272,6 +272,10 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs | |||
return metrics.DiscardedReasonNotUserEvent | |||
} | |||
|
|||
if strings.HasSuffix(msg.Text, "<abbr title=\"generated-from-mattermost\"></abbr>") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, let me move it to a constants
@sbishel the reason to use There is not problem in editions, because editions are not going to be detecting this, it only affects the creations, and for editions, normally we already have the information in the server anyway, so it is not that relevant. Also, creations is overwhelmingly bigger than editions, so this optimization is very relevant. Also, I was thinking about the posibility of include the "remote-id" or any other "server side code", to be sure that if you have 2 mattermost instances connected to the same microsoft instance, you are replicating properly everything as expected. It is not a use case that we support, but there is not harm on adding that extra data, I guess. |
@lieut-data The tests are already verifying that the messages generated are the right ones with the new string, and also I added a new tests for verifying that the message gets discarded after that text is found in the message. |
…ble in the future
…-message-duplication
This PR follows 2 approaches to avoid possible race conditions on the message sent to MS Teams from mattermost and receiving the "echo" too soon. 1 is, double check if the message is already associated with a mattermost message. Right before creating the new message.
The other strategy applied is to introduce a non-visible html label (an empty
abbr
lable, with thetitle
generated-from-mattermost
that tags the message itself as something generated from mattermost without affect the look and feel of the message in MSTeams side. Then we check if that "tag" exists in the message, and if that is the case, we discard the message as it was generated from mattermost.