falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 937 forks source link

feat(testing): simulate multipart file upload #2141

Open TigreModerata opened 1 year ago

TigreModerata commented 1 year ago

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.

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[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2141 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 63 63 Lines 7485 7522 +37 Branches 1278 1286 +8 ========================================= + Hits 7485 7522 +37 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TigreModerata commented 1 year ago

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 commented 1 year ago

Hi again @TigreModerata, and thanks for this improvement :+1: 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 commented 1 year ago

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

vytas7 commented 1 year ago

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.

TigreModerata commented 1 year ago

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.

Now I just string a body together with the info coming from files, but it's probably pretty ugly (but it works, at least).

TigreModerata commented 1 year ago

Hi, I'm back, thanks for all the feedback! Getting back to work on this today, sorry for the long delay (holiday + new job).

CaselIT commented 1 year ago

Hi, I'm back, thanks for all the feedback! Getting back to work on this today, sorry for the long delay (holiday + new job).

Thanks for the update, I'll take a look this week. (Good luck on your new job!)

TigreModerata commented 1 year ago

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!]

vytas7 commented 1 year ago

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

TigreModerata commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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!