MM-38982: fixes SVG uploading

[koox00]

Summary

Uploading SVG files was "almost" treated like an image in some places,
resulting in not showing the SVG on the client.
This is happening because when we fail to generate a mini preview for an
image, we prepend "invalid-" to the MimeType.

This commit adds a check to guard against SVG files in methods that make
no sense for SVGs.

Ticket Link

https://mattermost.atlassian.net/browse/MM-38982

Release Note

Fixes uploading SVG files

[mm-cloud-bot]

No Kubernetes clusters available at the moment, please contact the Mattermost Cloud Team or wait a bit.

[streamer45]

Thanks @koox00, I am thinking the core issue here is that we are messing with the mime-type which we shouldn't.

@cpoile I assume that was a temporary solution and that we figured out the problem eventually so does it still make sense to keep that logic?

[koox00]

Thanks @koox00, I am thinking the core issue here is that we are messing with the mime-type which we shouldn't.

That's true, but also some methods are too specific for images, that assume a thumb file and a preview one, which don't exist for SVGs

[cpoile]

@streamer45 I think you're right, #18445 was a bit of a temporary fix to try to stop community from getting OOM. I'm not sure of the final result of that investigation, or whether we can remove the fix yet.
@koox00 Am I right in assuming that even if we removed the invalid- mime prefix we would still need these changes?

[koox00]

@koox00 Am I right in assuming that even if we removed the invalid- mime prefix we would still need these changes?

@cpoile not that the app is going to crash without the changes, but IMO, yes either way these changes would be good to have

e.g the following makes no sense for SVGs, does it?

	if info.IsImage() {
		if limitErr := checkImageResolutionLimit(info.Width, info.Height, *a.Config().FileSettings.MaxImageResolution); limitErr != nil {
			return nil, model.NewAppError("uploadData", "app.upload.upload_data.large_image.app_error",
				map[string]interface{}{"Filename": us.Filename, "Width": info.Width, "Height": info.Height}, "", http.StatusBadRequest)
		}
		nameWithoutExtension := info.Name[:strings.LastIndex(info.Name, ".")]
		info.PreviewPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_preview.jpg"
		info.ThumbnailPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_thumb.jpg"
		imgData, fileErr := a.ReadFile(uploadPath)
		if fileErr != nil {
			return nil, fileErr
		}
		a.HandleImages([]string{info.PreviewPath}, []string{info.ThumbnailPath}, [][]byte{imgData})
	}

I've also considered to change fileInfo.IsImage to return false for SVG files, but I don't know what is the expected.

[streamer45]

@agnivade Do you have more context on the original OOM issue that caused some of this code to be introduced?

[agnivade]

Yeah, the original OOM issue was due to passing the image preview as part of *model.Post to all plugins. Therefore, serializing the huge byte slice caused a large allocation. Then there was another unrelated issue caused by a model API change, but that didn't cause any memory problem.

We can revert this one if it does not make sense any more.

[koox00]

@agnivade, @cpoile what do you propose to do?
so should I close this PR or keep it?
revert this one #18445,
or just re-assign the ticket?

[agnivade]

I'll defer to Claudio/CPoile on this.

[streamer45]

@koox00 I think we can remove the "invalid" mime type fix on top of this PR and proceed.

[koox00]

@koox00 I think we can remove the "invalid" mime type fix on top of this PR and proceed.

@streamer45 done

[cpoile]

Thanks @koox00 !

[streamer45]

There are a couple more places where we check against image/svg+xml. I'd suggest using the nice util you added here. Other than that, it looks great, thanks 👍

[jgilliam17]

@koox00 Sometimes when I add a single .svg file it is not shown, but if the file is part of multi-attachment post it is shown.
On the image, the window svg was posted twice (see red lines) and it failed to be shown, but preview is there when it is posted later in a multi-attachment post

[koox00]

@koox00 Sometimes when I add a single .svg file it is not shown, but if the file is part of multi-attachment post it is shown.
On the image, the window svg was posted twice (see red lines) and it failed to be shown, but preview is there when it is posted later in a multi-attachment post

The previews are shown now, right? I assumed the posts in both screenshots are the same (the timestamp matches):

[jgilliam17]

@koox00 I still don't see the single svg posts on Chrome and Safari. They are visible on Firefox

[koox00]

@jgilliam17 OK the issue for not showing are CSS rules, (which this PR doesn't change).
I'll raise a webapp PR as well.

[jgilliam17]

@koox00 do we have to wait for webapp PR before merging this one?

[koox00]

@koox00 do we have to wait for webapp PR before merging this one?

@jgilliam17 we can merge this one, and fix the webapp in another one

[jgilliam17]

Just waiting for E2Es to finish, will merge after report posts and if everything looks good.

[koox00]

@koox00 do we have to wait for webapp PR before merging this one?

@jgilliam17 I've added this ticket for webapp https://mattermost.atlassian.net/browse/MM-44470

[jgilliam17]

E2E report looks good.

[mm-cloud-bot]

Test server destroyed