* MM-7633: Optimize memory utilization during file uploads
Refactored the file upload code to reduce redundant buffering and stream
directly to the file store. Added tests.
Benchmark results:
```
levs-mbp:mattermost-server levb$ go test -v -run nothing -bench Upload -benchmem ./app
...
BenchmarkUploadFile/random-5Mb-gif-raw-ish_DoUploadFile-4 10 122598031 ns/op 21211370 B/op 1008 allocs/op
BenchmarkUploadFile/random-5Mb-gif-raw_UploadFileTask-4 100 20211926 ns/op 5678750 B/op 126 allocs/op
BenchmarkUploadFile/random-5Mb-gif-UploadFiles-4 2 1037051184 ns/op 81806360 B/op 3705013 allocs/op
BenchmarkUploadFile/random-5Mb-gif-UploadFileTask-4 2 933644431 ns/op 67015868 B/op 3704410 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-raw-ish_DoUploadFile-4 100 13110509 ns/op 6032614 B/op 8052 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-raw_UploadFileTask-4 100 10729867 ns/op 1738303 B/op 125 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-UploadFiles-4 2 925274912 ns/op 70326352 B/op 3718856 allocs/op
BenchmarkUploadFile/random-2Mb-jpg-UploadFileTask-4 2 995033336 ns/op 58113796 B/op 3710943 allocs/op
BenchmarkUploadFile/zero-10Mb-raw-ish_DoUploadFile-4 30 50777211 ns/op 54791929 B/op 2714 allocs/op
BenchmarkUploadFile/zero-10Mb-raw_UploadFileTask-4 50 36387339 ns/op 10503920 B/op 126 allocs/op
BenchmarkUploadFile/zero-10Mb-UploadFiles-4 30 48657678 ns/op 54791948 B/op 2719 allocs/op
BenchmarkUploadFile/zero-10Mb-UploadFileTask-4 50 37506467 ns/op 31492060 B/op 131 allocs/op
...
```
https://mattermost.atlassian.net/browse/MM-7633https://github.com/mattermost/mattermost-server/issues/7801
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
- [x] Added or updated unit tests (required for all new features)
- [ ] Added API documentation (required for all new APIs)
- [ ] All new/modified APIs include changes to the drivers
*N/A*???
- [x] Includes text changes and localization file ([.../i18n/en.json](https://github.com/mattermost/mattermost-server/blob/master/i18n/en.json)) updates
Overview of changes:
- api4
- Replaced `uploadFile` handler with `uploadFileStream` that reduces
unnecessary buffering.
- Added/refactored tests for the new API.
- Refactored apitestlib/Check...Status functions.
- app
- Added App.UploadFileTask, a more efficient refactor of UploadFile.
- Consistently set `FileInfo.HasPreviewImage`
- Added benchmarks for the new and prior implementations
- Replaced passing around `*image.Image` with `image.Image` in the
existing code.
- model
- Added a more capable `client4.UploadFiles` API to match the new server
API’s capabilities.
- I18n
- Replaced `api.file.upload_file.bad_parse.app_error` with a more generic
`api.file.upload_file.read_request.app_error`
- plugin
- Removed type `plugin.multiPluginHookRunnerFunc` in favor of using
`func(hooks Hooks) bool` explicitly, to help with testing
- tests
- Added test files for testing images
Still remaining, but can be separate PRs - please let me know the preferred
course of action
- Investigate JS client API - how does it do multipart?
- Performance loss from old code on (small) image processing?
- Deprecate the old functions, change other API implementations to use
UploadFileTask
Definitely separate future PRs - should I file tickets foe these?
- Only invoke t.readAll() if there are indeed applicable plugins to run
- Find a way to leverage goexif buffer rather than re-reading
Suggested long-term improvements - should I file separate tickets for these?
- Actually allow uploading of large (GB-sized) files. This may require a
change in how the file is passed to plugins.
- (Many) api4 tests should probably be subtests and share a server setup -
will be much faster
- Performance improvements in image processing (goexif/thumbnail/preview)
(maybe use https://mattermost.atlassian.net/browse/MM-10188 for this)
Questions:
1. I am commiting MBs of test images, are there better alternatives? I can
probably create much less dense images that would take up considerably less
space, even at pretty large sizes
2. I18n: Do I need to do anything special for the string change? Or just wait
until it gets picked up and translated/updated?
3. The image dimensions are flipped in resulting FileInfo to match the actual
orientation. Is this by design? Should add a test for it, perhaps?
4. What to do in the case of partial success? S3 but not DB, some files but not
others? For now, just doing what the old code did, I think.
5. Make maxUploadDrainBytes configurable? Also, should this be the systemic
behavior of all APIs with non-empty body? Otherwise dropped altogether?
Check all other ioutil.ReadAll() from sockets. Find a way to set a total
byte limit on request Body?
* WIP - Fixed for GetPluginsEnvironment() changes
* WIP - PR feedback
1. Refactored checkHTTPStatus to improve failure messages
2. Use `var name []type` rather than `name := ([]type)(nil)`
3. Replaced single-letter `p` with a more intention-revealing `part`
4. Added tests for full image size, `HasPreviewImage`
* WIP - rebased (c.Session->c.App.Session)
* WIP - PR feedback: eliminated use of Request.MultipartReader
Instead of hacking the request object to use r.MultipartReader now have own
functions `parseMultipartRequestHeader` and `multipartReader` eliminating the
need to hack the request object to use Request.MultipartReader limitations.
* WIP - PR feedback: UploadFileX with functional options
* WIP - PR feedback: style
* WIP - PR feedback: errors cleanup
* WIP - clarified File Upload benchmarks
* WIP - PR feedback: display the value of erroneous formname
* WIP - PR feedback: fixed handling of multiple channel_ids
* WIP - rebased from master - fixed tests
* PR Feedback
* PR feedback - moved client4.UploadFiles to _test for now
* testlib: introduce and leverage
This doesn't yet factor out the individual test helpers: many packages
still rely on `api4` directly to do this, but now wire up the test store
setup through this package. `app` and `store`, in particular, don't use
`testlib` because of circular dependencies at the moment.
* cmd: command_test.go: use api4 testlib
* cmd: plugin_test.go: remove dependence on test-config.json
* cmd: config_test.go use configured database settings
* ensure test-(te|ee) exit with status code
* test-server: run all tests, deprecating test-te/test-ee
* cmd/mattermost/commands: fix unit tests
Instead of relying on (and modifying) a config.json found in the current path, explicitly create a temporary one from defaults for each test. This was likely the source of various bugs over time, but specifically allows us to override the SqlSettings to point at the configured test database for all tests simultaneously.
* wrap run/check into a test helper
It was insufficient to set a config for each invocation of CheckCommand or RunCommand: some tests relied on the config having changed in a subsequent assertion. Instead, create a new test helper embedding api4.TestHelper. This has the nice advantage of cleaning up all the teardown.
* additional TestConfigGet granularity
* customized config path to avoid default location
* be explicit if the storetest initialization fails
* generate safe coverprofile names in the presence of subtests
* additional TestConfigShow granularity
* fix permission_test.go typo
* fix webhook tests
* actually flag.Parse() to skip database setup on os.Execed tests
* fix recent regression in #9962, not caught by unit tests
Config Checks at StartUp Part1
Config Checks; Tests for TLS Server
HSTS header implementation + tests
make gofmt happy with new go version...
make gofmt happy with new go version #2...
fix logic bug
fix typo
Fix unnecessary code block
* Testing caching for emojis
* MM-10117 Add support to add/delete and activate/deactivate plugins via CLI
* Removing old work
* MM-10117 Moved files and addedd plugin test
* MM-10117 Renamed commands to enable/disable and updated add test
* MM-10117 Finished plugin test and improved error message for plugin commands
* MM-10117 Fixing plugin directories for test
* MM-10117 Renamed commands and updated commands to support multiple plugins
* MM-10117 Updating removed to deleted textclear
* MM-10117 Fixing nil pointer error for listing plugins
* MM-10117 Removing fileReader close
* MM-10117 Declaring error for GetPlugins
* MM-10117 Removing unnecessary nil check
* Implement experimental REST API endpoints for plugins
* Updates per feedback and rebase
* Update tests
* Further updates
* Update extraction of plugins
* Use OS temp dir for plugins instead of search path
* Fail extraction on paths that attempt to traverse upward
* Update pluginenv ActivePlugins()
* PLT-7356 Split emoji test 1 into smaller test files
Prevent test files from splitting into multiple posts.
Reducing the number of emoji in each file (test-emoticons1.md through test-emoticons4.md) so each post is <4000 characters. Adding two new files to accommodate the emoji overflow: test-emoticons5.md and test-emoticons6.md
* PLT-7356 Split emoji test 2 into smaller test files
Prevent test files from splitting into multiple posts.
Reducing the number of emoji in each file (test-emoticons1.md through test-emoticons4.md) so each post is <4000 characters. Adding two new files to accommodate the emoji overflow: test-emoticons5.md and test-emoticons6.md
* PLT-7378 Split emoji test 3 into smaller test files
Prevent test files from splitting into multiple posts.
Reducing the number of emoji in each file (test-emoticons1.md through test-emoticons4.md) so each post is <4000 characters. Adding two new files to accommodate the emoji overflow: test-emoticons5.md and test-emoticons6.md
* PLT-7378 Split emoji test 4 into smaller test files
Prevent test files from splitting into multiple posts.
Reducing the number of emoji in each file (test-emoticons1.md through test-emoticons4.md) so each post is <4000 characters. Adding two new files to accommodate the emoji overflow: test-emoticons5.md and test-emoticons6.md
* PLT-7356 and 7378 Create test-emoticons5.md
Prevent test files from splitting into multiple posts.
Reducing the number of emoji in each file (test-emoticons1.md through test-emoticons4.md) so each post is <4000 characters. Adding two new files to accommodate the emoji overflow: test-emoticons5.md and test-emoticons6.md
* PLT-7356 and 7378 Create test-emoticons6.md
Prevent test files from splitting into multiple posts.
Reducing the number of emoji in each file (test-emoticons1.md through test-emoticons4.md) so each post is <4000 characters. Adding two new files to accommodate the emoji overflow: test-emoticons5.md and test-emoticons6.md
* Updated with new emoji set for testing
Changed test-emoticons4.md from Symbols and formatting tests to Flags, Special, and formatting tests.
* Updated with new emoji set for testing
Updated test-emoticons3.md from Places to Objects and Symbols.
* Updated with new emoji set for testing
Updated test-emoticons2.md from Objects to Food, Activity, and Travel.
* Updated with new emoji set for testing
Updated test-emoticons1.md to new emoji set, still containing the intro, People, and Nature sections.
* Refactored emoji.json to support multiple aliases and emoji categories
* Added custom category to emoji.jsx and stabilized all fields
* Removed conflicting aliases for :mattermost: and :ca:
* fixup after store changes
* Added emoji reactions
* Removed reactions for an emoji when that emoji is deleted
* Fixed incorrect test case
* Renamed ReactionList to ReactionListView
* Fixed 👍 and 👎 not showing up as possible reactions
* Removed text emoticons from emoji reaction autocomplete
* Changed emoji reactions to be sorted by the order that they were first created
* Set a maximum number of listeners for the ReactionStore
* Removed unused code from Textbox component
* Fixed reaction permissions
* Changed error code when trying to modify reactions for another user
* Fixed merge conflicts
* Properly applied theme colours to reactions
* Fixed ESLint and gofmt errors
* Fixed ReactionListContainer to properly update when its post prop changes
* Removed unnecessary escape characters from reaction regexes
* Shared reaction message pattern between CreatePost and CreateComment
* Removed an unnecessary select query when saving a reaction
* Changed reactions route to be under /reactions
* Fixed copyright dates on newly added files
* Removed debug code that prevented all unit tests from being ran
* Cleaned up unnecessary code for reactions
* Renamed ReactionStore.List to ReactionStore.GetForPost
* Slack Import unit tests (for some functions).
Covers the completely self contained functions in the Slack Importer
that are straightforward to unit test.
JSON processing functions are not yet unit tested.
* Unit tests for Slack Import JSON parsing.
This tests the 3 functions for parsing JSON, using a sample set of
Slack data for posts, channels, and users.
* PLT-3552 Partially-fixed an invalid test case
* PLT-3551 Removed an invalid test case
* PLT-3482 Added a test case for multiple lists separated by text
* PLT-3482 Updated marked to fix multiple lists separated by text
* Removed MattermostLexer since it was exactly the same as the default marked one
* Updated list autonumbering to always start numbering from the first item's number
