Simplify multipart spec
🤔 What does this MR do?
Related to !33277 (comment 399666499)
https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/lib/gitlab/middleware/multipart_spec.rb is quite hard to get for new readers. This MR aims to refactor the file to have a better code organization and give a better clarity on what's going on.
Please note, that we introduced feature tests a few days ago, so this file has already a good coverage.
⚔ Design choices
- The
multipart.rb
is responsible to read the upload params from Workhorse and transform them into anUploadedFile
. There are 3 aspects that need to be tested:- The parameters can be sent in two "modes"
-
local
: in this mode, the uploaded file is in the file system, apath
parameter indicates its location -
remote
: in this mode, the uploaded file has been put in object storage by Workhorse and here we receive theremote_id
, which is simply the object storage key.
-
- The parameters can be at the root of the request or nested. Imagine that the file upload is a field of a form, the param name can be:
-
file
. That's a "root" parameter -
user[avatar]
. That's a nested parameter -
user[friend][avatar]
. That's a deeply nested parameter. - The upload request can come with multiple files, we can even have a mix of parameters at different nested levels.
-
- In
local
mode, thepath
that is sent is verified against a known list of allowed directories.
- The parameters can be sent in two "modes"
- You can imagine that setup code for those tests is quite heavy, to help us with the de-clutter the
multipart_spec.rb
file, this MR uses 3 auxiliary files:-
MultipartHelpers
A spec helper that has a collection of functions to help us:-
#post_env
: builds aRack::MockRequest
that will be sent to the middleware -
#upload_parameters_for
: builds the parameters set for a given upload -
#expect_uploaded_files
: puts expectations on thedouble
to verify the params ->UploadedFile
transformation.
-
-
spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb
A collection of shared contexts. Their main aim is to build a temporary file and keep care of its deletion at the end of the example run.-
with one temporary file for multipart
: this context creates one temporary file. See the comments in the code for more details -
with two temporary files for multipart
: creates two temporary files -
with three temporary files for multipart
: creates three temporary files - All the contexts setup some functions so that the test code can access the temporary files.
-
-
spec/support/shared_examples/lib/gitlab/middleware/multipart_shared_examples.rb
provides the following:-
handling all upload parameters conditions
shared example giving a full set of examples with different parameters conditions (nested or not, single or multiple)
-
-
spec/lib/gitlab/middleware/multipart/handler_spec.rb
for the following:- Check the allowed_paths
- Check that under different conditions, the packages uploader path is included or not
-
- We use
let
vars instead oflet_it_be
for two reasons:- We mainly use
String
s andHash
es only. Usinglet_it_be
will not bring the usual benefits - All these vars should be evaluated at "example run" time. If we keep the var state between example runs, we can run into issues.
- We mainly use
- To simplify things further, the
Dir.tmpdir
directory is now accepted directly by the multipart Handler instead of theUploadedFile.from_params
function. - All the examples from the original spec have been re-used/re-implemented except this one where we stub all the calls on the file system. I don't fully understand what we're trying to do here, if we stub all the file system calls, what are we testing?
🤷 - More examples have been added:
- added an example where the JWT header would point to a wrong parameter name.
🗒 Notes to reviewers/maintainers
- You might have an easier time reviewing the whole file than the diff.
Screenshots
n / a
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
- [-] Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
- [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by David Fernandez