If autolinking would make posts too long, don't #154

elyscape · 2021-03-30T23:37:06Z

Summary

Substitutions that increase message length have the potential to grow a post past the ability of the database to store it. This manifests to users as their posts inexplicably failing. To avoid this fail state, we check to see if the post-substitution message is too long and, if so, don't apply the changes.

Unfortunately, the actual maximum post size is not currently accessible to plugins. In the meantime, we use the smallest value for which the server guarantees support, which is exposed through model.POST_MESSAGE_MAX_RUNES_V1 and is in practice 4000 runes.

Ticket Link

None.

mattermod · 2021-03-30T23:37:10Z

Hello @elyscape,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

hanzei

Good catch on this issue.:+1:

hanzei · 2021-03-31T05:27:18Z

server/autolinkplugin/plugin.go

@@ -208,6 +209,11 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,
 			return nil, ""
 		}

+		if runes := utf8.RuneCountInString(postText); runes > model.POST_MESSAGE_MAX_RUNES_V1 {


For newer Mattermost instances or the one that updates, this will be longer:

This release includes support for post messages longer than the default of 4000 characters, but may require a manual database migration. This migration is entirely optional, and need only be done if you want to enable post messages up to 16383 characters. For many installations, no migration will be required, or the old limit remains sufficient.

https://docs.mattermost.com/administration/important-upgrade-notes.html

I'm hesitant to cap at the old limit as that might render the plugin useless for some instances.

This is fair, but we currently have no way to find the actual limit from a plug-in, and that excerpt notes that upgrading from 4000 characters may require a manual migration, which we can't expect every deployment to have performed.

From looking at the server code base the UpdatePost RPC method does return an model.post.is_valid.msg.app_error error. (https://github.com/mattermost/mattermost-server/blob/ee3f986da0d9fbc69769dcec0c66d8493589757a/model/post.go#L299) Can we check for the specific error and return the original message in that case?

The problem is that this function is effectively the handler for the MessageWillBePosted() and MessageWillBeUpdated() hooks, meaning we can alter the Post object but need to allow persistence to be handled further down the chain.

I think the ideal solution is to first provide a way for plugins to get the result of GetMaxPostSize(), and then have the plugin:

Retrieve that value

Update the Post object

Call Post.IsValid() with its value

Revert the object if the result is false

That is a concern to me as it unexpectedly renders the plugin useless in some cases that previously worked. I'm 2/5 that it's not worth breaking that cases for the bug fix that the PR includes.

@hanzei What would you suggest as a path forward?

From looking at the server code base the UpdatePost RPC method does return an model.post.is_valid.msg.app_error error. (mattermost/mattermost-server@ee3f986/model/post.go#L299) Can we check for the specific error and return the original message in that case?

@elyscape Do you think it's feasible to refactor the code and use that approach?

I do not. This function is effectively the handler for the MessageWillBePosted and MessageWillBeUpdated hooks, meaning we can alter the Post object but need to allow persistence to be handled further down the chain. From the documentation:

To reject a post, return an non-empty string describing why the post was rejected. To modify the post, return the replacement, non-nil *model.Post and an empty string. To allow the post without modification, return a nil *model.Post and an empty string. To dismiss the post, return a nil *model.Post and the const DismissPostError string.

We could call UpdatePost from MessageHasBeenPosted or MessageHasBeenUpdated, because those are events that fire after a message occurs, but not here.

@hanzei As a side complication, even once we get access to the result of GetMaxPostSize(), we'll need a fallback path for versions of Mattermost that don't support it.

levb · 2021-04-01T01:58:27Z

server/autolinkplugin/plugin.go

@@ -208,6 +209,11 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,
 			return nil, ""
 		}

+		if runes := utf8.RuneCountInString(postText); runes > model.POST_MESSAGE_MAX_RUNES_V1 {


I like the simplicity of the current implementation. 0/5 we add it in "as is". We should probably open a ticket to make the MaxPostSize available to plugins, and to update Autolink once it's done; until the, we document the limitation? cc @aaronrothschild @justinegeffen

elyscape · 2021-04-01T18:54:47Z

I updated the PR to fix the tests and also include the post ID logged when a message would be too long.

Edit: Well, that broke the build differently because we aren't populating Post.Id in the test. I'll fix that once I figure out if that field is always expected to have a value and, if not, what I should do.

Edit 2: Turns out that the field is never expected to have a value. Oh well.

Substitutions that increase message length have the potential to grow a post past the ability of the database to store it. This manifests to users as their posts inexplicably failing. To avoid this fail state, we check to see if the post-substitution message is too long and, if so, don't apply the changes. Unfortunately, the actual maximum post size is not currently accessible to plugins. In the meantime, we use the smallest value for which the server guarantees support, which is exposed through model.POST_MESSAGE_MAX_RUNES_V1 and is in practice 4000 runes.

elyscape · 2021-04-01T20:40:11Z

Okay, tests fixed.

If the first return value of MessageWillBeUpdated is nil, the update is rejected, with the second return value providing an explanation for the rejection. To allow the update to occur without any changes, the function should return the value of newPost that was passed in to it. We don't reject any post updates, but if we did then even then we would have to provide a rejection reason. As such, we don't need to alter the logic of ProcessPost, and can instead simply put a check on its return values.

levb · 2021-04-13T14:04:03Z

@elyscape @hanzei @iomodo we discussed it offline, and the proposal is to implement both failure modes ("unchanged" and "error") for this feature. It can be triggered by a new Link-level flag (UseOriginalOnError: true? ). The current master behavior should be the default.

elyscape · 2021-04-16T19:16:20Z

@levb Sounds good. I'll figure out how to implement that in the next week.

elyscape · 2021-04-26T21:06:49Z

@levb @hanzei @iomodo I started looking into this and realized that I don't think there's actually a good way to implement it. As I understand it, the proposal is to implement this setting on a per-Autolink basis. The problem is that multiple Autolinks can apply to a single message, and processing applies all of them at once. If multiple Autolinks apply to a message but have differing values for UseOriginalOnError, it's unclear what should happen. I could have it try again with only the Autolinks that have UseOriginalOnError set to false, but that seems like it would provide a bad user experience. In my opinion, this should probably be a plugin-level setting. Thoughts?

mattermod · 2021-05-07T01:01:15Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

levb · 2022-03-30T13:44:08Z

0/5 change the semantics, and just let the processing stop at the point where it'd have exceeded the max post size. I understand that it is unpredictable in the sense of what links will be processed, but IMO it can be a simple, sufficent solution?

hanzei · 2023-02-23T20:39:37Z

@elyscape Coming back to this PR: The solution in #154 (comment) seems good to me. Do you still want to work on it?

mattermost-community#192) * Revert "Update main.go (mattermost-community#154)" This reverts commit be4a281d0cc791d10e6e5ae917b325b2f054e475. * Revert "[MM-33506] Use embed package to include plugin manifest (mattermost-community#145)" This reverts commit ca9ee3c17c6920a636a33f378e17395afd6f329f. * Revert "Don't generate manifest.ts (mattermost-community#127)" This reverts commit 18d30b50bc7ba800c9f05bfd82970781db0aea3e. * install-go-tools target, adopt gotestsum * bring back make apply + automatic versioning * Update build/manifest/main.go Co-authored-by: Michael Kochell <[email protected]> * suppress git describe error when no tags match * make version/release notes opt-in * fix whitespace in Makefile * document version management options --------- Co-authored-by: Michael Kochell <[email protected]>

* [MM-98] Update plugin with respect to phase 1 upgrades * [MM-98] Fix lint * [MM-98] Update Readme * [MM-98] Review fixes regarding constant for golangci-lint * Sync with playbooks: install-go-tools, gotestsum, and dynamic versions (#192) * Revert "Update main.go (#154)" This reverts commit be4a281d0cc791d10e6e5ae917b325b2f054e475. * Revert "[MM-33506] Use embed package to include plugin manifest (#145)" This reverts commit ca9ee3c17c6920a636a33f378e17395afd6f329f. * Revert "Don't generate manifest.ts (#127)" This reverts commit 18d30b50bc7ba800c9f05bfd82970781db0aea3e. * install-go-tools target, adopt gotestsum * bring back make apply + automatic versioning * Update build/manifest/main.go Co-authored-by: Michael Kochell <[email protected]> * suppress git describe error when no tags match * make version/release notes opt-in * fix whitespace in Makefile * document version management options --------- Co-authored-by: Michael Kochell <[email protected]> * Fetch plugin logs from server (#193) Co-authored-by: Jesse Hallam <[email protected]> --------- Co-authored-by: Jesse Hallam <[email protected]> Co-authored-by: Michael Kochell <[email protected]> Co-authored-by: Ben Schumacher <[email protected]>

elyscape requested a review from levb as a code owner March 30, 2021 23:37

hanzei added the 2: Dev Review Requires review by a core committer label Mar 31, 2021

hanzei requested changes Mar 31, 2021

View reviewed changes

levb approved these changes Apr 1, 2021

View reviewed changes

elyscape force-pushed the handle-long-posts branch from 5bb970b to bfa2b6c Compare April 1, 2021 18:54

elyscape force-pushed the handle-long-posts branch from bfa2b6c to 4eadd40 Compare April 1, 2021 20:05

elyscape force-pushed the handle-long-posts branch from 4eadd40 to 7c930fa Compare April 1, 2021 20:38

elyscape requested a review from hanzei April 1, 2021 20:55

hanzei removed their request for review April 13, 2021 13:28

levb self-requested a review April 13, 2021 14:04

mattermod added the Lifecycle/1:stale label May 7, 2021

hanzei self-assigned this Feb 21, 2023

hanzei added Work In Progress Not yet ready for review and removed 2: Dev Review Requires review by a core committer labels Feb 23, 2023

hanzei removed the request for review from levb February 23, 2023 20:40

hanzei removed their assignment Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If autolinking would make posts too long, don't #154

If autolinking would make posts too long, don't #154

elyscape commented Mar 30, 2021

mattermod commented Mar 30, 2021

hanzei left a comment

hanzei Mar 31, 2021

elyscape Mar 31, 2021

hanzei Mar 31, 2021

elyscape Mar 31, 2021

elyscape Mar 31, 2021

hanzei Apr 1, 2021

elyscape Apr 6, 2021

hanzei Apr 7, 2021

elyscape Apr 7, 2021

elyscape Apr 12, 2021

levb Apr 1, 2021

elyscape commented Apr 1, 2021 •

edited

Loading

elyscape commented Apr 1, 2021

levb commented Apr 13, 2021

elyscape commented Apr 16, 2021

elyscape commented Apr 26, 2021

mattermod commented May 7, 2021

levb commented Mar 30, 2022

hanzei commented Feb 23, 2023

If autolinking would make posts too long, don't #154

Are you sure you want to change the base?

If autolinking would make posts too long, don't #154

Conversation

elyscape commented Mar 30, 2021

Summary

Ticket Link

mattermod commented Mar 30, 2021

hanzei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elyscape commented Apr 1, 2021 • edited Loading

elyscape commented Apr 1, 2021

levb commented Apr 13, 2021

elyscape commented Apr 16, 2021

elyscape commented Apr 26, 2021

mattermod commented May 7, 2021

levb commented Mar 30, 2022

hanzei commented Feb 23, 2023

elyscape commented Apr 1, 2021 •

edited

Loading