feat(testing): simulate multipart file upload

[TigreModerata]

Summary of Changes

Draft of tests section for post-ing files as multipart/form-data. Parameter 'files' added to simulate_requests in the testing client, as well as a file test_multipart_formdata_request of tests.

Related Issues

#1010

Pull Request Checklist

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

• [x ] Applied changes to both WSGI and ASGI code paths and interfaces (where applicable).
• [x ] Added tests for changed code.
• [ x] Prefixed code comments with GitHub nick and an appropriate prefix.
• [ x] Coding style is consistent with the rest of the framework.
• [ x] Updated documentation for changed code.
  • [x ] Added docstrings for any new classes, functions, or modules.
  • [ x] Updated docstrings for any modifications to existing code.
  • [ x] Updated both WSGI and ASGI docs (where applicable).
  • [ x] Added references to new classes, functions, or modules to the relevant RST file under docs/.
  • [ x] Updated all relevant supporting documentation files under docs/.
  • [ x] A copyright notice is included at the top of any new modules (using your own name or the name of your organization).
  • [ x] Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
• Changes (and possible deprecations) have towncrier news fragments under docs/_newsfragments/, with the file name format {issue_number}.{fragment_type}.rst. (Run towncrier --draft to ensure it renders correctly.)

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

PR template inspired by the attrs project.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b5416c1) to head (6ede18d).
Report is 14 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #2141   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         7485      7522   +37     
  Branches      1278      1286    +8     
=========================================
+ Hits          7485      7522   +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TigreModerata]

I used urllib3 and I added it to requirements, but it isn't found in the tests - where does it need to be added to be seen?

[vytas7]

Hi again @TigreModerata, and thanks for this improvement 👍
However, when it comes to urllib3, Falcon is designed to have no hard dependencies on any third part package, so we should ideally rewrite this functionality not to use urllib3 at all...
Alternatively, we could possibly catch an ImportError, and document that this optional functionality requires urllib3.

[TigreModerata]

I'll try to write it out, it's probably good exercise :)

[vytas7]

Yeah, it should be easy at least when it comes to basic functionality, check out tests/test_media_multipart.py where I mostly construct these forms by hand.

[vytas7]

Looks great so far 💯

As discussed, we just need to get rid of urllib3, or if the functionality in question is non-trivial to implement, we need to make it optional, like msgpack, jsonschema, etc.
(But of course I'd prefer if this was implemented directly in the framework.)

[vytas7]

Do we really need to add separate files? Maybe we could just use inline strings, or use existing source or image files?
Otherwise we need to surface these new files in MANIFEST.in to get them included in the sdist.

[TigreModerata]

I didn't really know... I wanted to make sure all goes well mainly with bigger stuff (the image), but now that I'm sure I'll remove them.

[vytas7]

Is there any particular reason behind this newline removal?

[TigreModerata]

I don't know what happened... was that me?

[vytas7]

I think it must have been you, yes. It is coming from you changeset, but it might be some artefact of merging things back and forth on your side, not necessarily something you actively did.

[TigreModerata]

I added the data arg to simulate_post, in such a way that if both json and (files or data) are present, a badrequest exception is thrown. If json is absent, then files and data will be processed: if files is also absent, data will be url-encoded.
I'm not sure I got all the requirements right, in particular the "body as an alias of data" (I didn't touch it because I don't know exactly how and I'm not sure if you wanted to leave that for a future version).
I also didn't remove the possibility for nested forms, just because the possibility exists in other multipart test files and I wasn't sure if you guys agreed to remove them... but it's a quick fix, if you confirm.

[I am really a beginner, so some of what you said may have gone over my head... I apologize. Since you guys are so responsive, this is a great help to me to learn/practice, I really appreciate your patience!]

[CaselIT]

thanks for the change. let's wait for @vytas7 review

[CaselIT]

so we are assuming that if provided as string it's a json.

this would mean that we cannot replace deprecate body with data.

let's see @vytas7 opinion

[TigreModerata]

ok, I hadn't really understood what you guys meant by replacing the body... but ok, that should be an easy fix. I'll try to implement it, then you can use whatever works best.

[vytas7]

(I'm in the process of terraforming this somewhat... )

[TigreModerata]

Hi @vytas7 , did something happen, can I do anything to help? I just got a notification saying "mentioned" but I'm not sure why. I've not looked at this in ages, but I have time and I'm happy to try to contribute.

[vytas7]

Hi @TigreModerata!
I just merged the master branch into your branch to start with.

I was thinking to update your PR, and to change files and data to form, similar to how json is handled. I can make this change, and you can continue from that point!

Does that sound like a good plan?

[TigreModerata]

I was thinking to update your PR, and to change files and data to form, similar to how json is handled. I can make this change, and you can continue from that point!

I'm not totally sure what needs to be done, I need to refresh my memory a bit. But yeah, I'm happy if I can contribute!

[vytas7]

Hi again @TigreModerata!

I'm not totally sure what needs to be done, I need to refresh my memory a bit.

That's why I thought it would be a good opportunity to restart this from scratch, it doesn't need to be too complex in the first iteration. I was thinking we could simply add a form parameter that can be a dict or a list/tuple of key-value pairs. If the whole form is only simple key-values, we do a URL-encoded form, otherwise encode it as multipart, roughly how browsers do; what do you think?

I have written a simple proof-of-concept implementation, but it needs more validation, tests, documentation, and a newsfragment.

But yeah, I'm happy if I can contribute!

You are very welcome to!

[CaselIT]

overall seems ok. I've not checked if the api is consistent with what was suggested before or what request etc do