Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Django-Ninja PUT/PATCH request with UploadedFile #397

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix Django-Ninja PUT/PATCH request with UploadedFile #397

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe4e360
Fix Django-Ninja PUT/PATCH request with content-type multipart/form-data
Mar 18, 2022
6f7594c
Maintain the original request method
deniswvieira Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions ninja/operation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,28 @@
__all__ = ["Operation", "PathView", "ResponseObject"]


def load_request(request: HttpRequest):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def load_request(request: HttpRequest) -> None:

"""
Loads a non-json PUT/PATCH request as a POST, reading data from request body
and placing them in request.POST and request.FILES.
This is required to fix the behaviour when we have a PUT/PATCH endpoint that also
receives a File as parameter, forcing the data to be sent as multipart/form-data and
making the ParamModel look up at request.POST and request.FILES
"""
if request.method in ("PUT", "PATCH") and request.content_type != "application/json":
original_method = request.method
if hasattr(request, '_post'):
del request._post
del request._files
try:
request.method = "POST"
request.META['REQUEST_METHOD'] = 'POST'
request._load_post_and_files()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just hit request.FILES rather than manually accessing an undocumented underscored method?

Suggested change
request._load_post_and_files()
request.FILES

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

request.META['REQUEST_METHOD'] = original_method
request.method = original_method
except Exception:
pass
Comment on lines +47 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this.. why not use contextlib.supress ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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_method

I was thinking something like this

request.META['REQUEST_METHOD'] and request.method should 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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


class Operation:
def __init__(
self,
Expand Down Expand Up @@ -90,6 +112,7 @@ def __init__(
view_func._ninja_contribute_to_operation(self) # type: ignore

def run(self, request: HttpRequest, **kw: Any) -> HttpResponseBase:
load_request(request)
error = self._run_checks(request)
if error:
return error
Expand Down Expand Up @@ -240,6 +263,7 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
self.is_async = True

async def run(self, request: HttpRequest, **kw: Any) -> HttpResponseBase: # type: ignore
load_request(request)
error = self._run_checks(request)
if error:
return error
Expand Down