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 1 commit
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
23 changes: 23 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,27 @@
__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":
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'] = 'PUT'
request.method = "PUT"
deniswvieira marked this conversation as resolved.
Show resolved Hide resolved
except Exception:
pass

class Operation:
def __init__(
self,
Expand Down Expand Up @@ -90,6 +111,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 +262,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