Fix Django-Ninja PUT/PATCH request with UploadedFile #397
Conversation
|
@vitalik What do you think of this proposal? |
Co-authored-by: Charlie Wilson <charlie.wilson4@gmail.com>
|
@deniswvieira you'll need to write some tests and (likely) do some codestyle fixes before this is able to be merged. I'm happy to help out if needed. |
|
@vitalik, this pull request is better than middleware approach. Please merge this. ( i can port my tests if you want ) |
| @@ -31,6 +31,28 @@ | |||
| __all__ = ["Operation", "PathView", "ResponseObject"] | |||
|
|
|||
|
|
|||
| def load_request(request: HttpRequest): | |||
There was a problem hiding this comment.
def load_request(request: HttpRequest) -> None:
| try: | ||
| request.method = "POST" | ||
| request.META['REQUEST_METHOD'] = 'POST' | ||
| request._load_post_and_files() | ||
| request.META['REQUEST_METHOD'] = original_method | ||
| request.method = original_method | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
If we do this.. why not use contextlib.supress ?
There was a problem hiding this comment.
This is more than just a style fix too. request.META['REQUEST_METHOD'] and request.method should be returned to their original method even if the file loading method fails (silently).
There was a problem hiding this comment.
with contextlib.supress(Exception):
request.method = "POST"
request.META['REQUEST_METHOD'] = 'POST'
request._load_post_and_files()
request.META['REQUEST_METHOD'] = original_method
request.method = original_methodI was thinking something like this
request.META['REQUEST_METHOD']andrequest.methodshould be returned to their original method even if the file loading method fails (silently).
I have a small question ( if you dont mind ).. Can this method actually fail ? If it does, don't you think its a problem on either django's or our side ?
There was a problem hiding this comment.
Yes, your code suggestion is what I had in mind.
I'm not quite sure if it can actually fail, and maybe we shouldn't be catching and silently swallowing an exception anyway... Yeah, this just seems like a bad idea altogether unless @deniswvieira can say why we need to catch it (and with such a broad catch at that)
|
@vitalik do you want anything added to this PR? |
| try: | ||
| request.method = "POST" | ||
| request.META['REQUEST_METHOD'] = 'POST' | ||
| request._load_post_and_files() |
There was a problem hiding this comment.
I think this should just hit request.FILES rather than manually accessing an undocumented underscored method?
| request._load_post_and_files() | |
| request.FILES |
There was a problem hiding this comment.
Correct me if i am wrong but this is probably due to how django handles FILES.
Quoting @deniswvieira,
Ask "nicely" to Django load the data for us, making he think he is loading a POST request.
The alternative is to,
Add some logic into _MultiPartBodyModel and FileModel to load the request body data like the Django does for POST request (essencially, replicate Django logic)
Which in turn adds churn
There was a problem hiding this comment.
@baseplate-admin request.FILES is a property of WSGIRequest in django that triggers _load_post_and_files if ._files doesn't exist. Which it doesn't, because it was deleted a few lines earlier in this PR ;)
https://github.com/django/django/blob/main/django/core/handlers/wsgi.py#L104-L108
|
@vitalik, What's your take on this PR? If i rebase this PR with some tests would you be willing to merge this? |
|
Hi @vitalik |
|
Hello! When you accept that patch? That problem with post/patch is annoying ( |
|
It looks like the user who originally wrote this PR is inactive now and has been for two years. I'd be willing to pick this up and add the suggested changes along with tests if that is alright. @vitalik do you have any input for this? |
|
Due to in-activeness of this pr, i have decided to create |
Request PUT or PATCH raises an error for endpoints that receives UploadedFile. This happens because:
ParamModelto be_MultiPartBodyModel);This crashes in Django Ninja while processing the request (
Operation.run()) forParamModel_MultiPartBodyModelandFileModelsince it tries to retrieve data from request.POST and request.FILES.Possible solutions I reached:
_MultiPartBodyModelandFileModelto load the request body data like the Django does for POST request (essencially, replicate Django logic)I've choose the second approach.