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.

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

FileUploadParser should use more descriptive error when filename not included. #3610

Closed
yuyou opened this issue Nov 6, 2015 · 16 comments
Closed

Comments

@yuyou
Copy link

yuyou commented Nov 6, 2015

Hi

The FileUploadParser code (line 181) in rest_framework.parsers.py seems to work differently as it should be.

The line does not work in my Python 2.7.8 (OSX El Capitan):
if file_obj:

I modified and works like this:
if file_obj is not None:

Could someone confirm this buggy behavior under other situations?

Thanks.

Regards,

Yu

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 7, 2015

Do you have a test project we could test against ?

@tomchristie
Copy link
Member

tomchristie commented Nov 7, 2015

How are you replicating an issue and exactly what behaviour do you see?

@BraisGabin
Copy link

BraisGabin commented Nov 8, 2015

I just create an test project that reproduce this issue: https://github.com/BraisGabin/drf_3_3_test
The code works with drf 3.2.5 but it doesn't with 3.3.1.

To reproduce the error just launch the tests with the two different versions ./manage.py test.

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 9, 2015

Do you have the command you use to make the post as well ?

@yuyou
Copy link
Author

yuyou commented Nov 9, 2015

Thanks to BraisGabin for creating a test project. I am not sure what "command" do you mean? Django command or terminal command... but I cloned the test project and run a CURL command to reproduce the error.

curl -XPOST -H "Content-type: image/jpg" --data-binary @./app/test_assets/image.jpg -v "http://127.0.0.1:8000/media/"
* Hostname was NOT found in DNS cache
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 8000 (#0)
> POST /media/ HTTP/1.1
> User-Agent: curl/7.35.0
> Host: 127.0.0.1:8000
> Accept: */*
> Content-type: image/jpg
> Content-Length: 14109
> Expect: 100-continue
> 
* Done waiting for 100-continue
* HTTP 1.0, assume close after body
< HTTP/1.0 400 BAD REQUEST
< Date: Mon, 09 Nov 2015 09:46:15 GMT
< Server: WSGIServer/0.1 Python/2.7.6
< Vary: Accept, Cookie
< X-Frame-Options: SAMEORIGIN
< Content-Type: application/json
< Allow: POST, OPTIONS
< 
* Closing connection 0
{"detail":"FileUpload parse error - none of upload handlers can handle the stream"}

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 9, 2015

I suspect the content type here isn't correct as you're sending a image/jpg instead of a multipart/form

Forget about this comment I was thinking about them multipart content.

@yuyou
Copy link
Author

yuyou commented Nov 9, 2015

According to the spec., the parser is supposed to support direct binary upload. And it works if that line in question (line 181) is fixed from "if file_obj:" to "if file_obj is not None:".

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 9, 2015

@yuyou does it work with the change you mention ? I mean do you have a non empty file in the end ?

@yuyou
Copy link
Author

yuyou commented Nov 9, 2015

@xordoquy yes, confirmed. the CURL script got "HTTP/1.0 200 OK" after I locally made that change and the server received a correct instance of Django UploadedFile (in this case, it is InMemoryUploadedFile object).

Tested under
Django (1.8.6)
djangorestframework (3.3.1)

@tomchristie tomchristie added this to the 3.3.2 Release milestone Nov 9, 2015
@BraisGabin
Copy link

BraisGabin commented Nov 9, 2015

Hmmm, probably related with #3374. I had made a research in that issue and I had found a problem at the same line.

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 9, 2015

@BraisGabin I think it's a bit different the content type seems consistent with sending file.

yuyou added a commit to yuyou/django-rest-framework that referenced this issue Nov 23, 2015
@gregmuellegger
Copy link
Contributor

gregmuellegger commented Dec 8, 2015

I found to have the same issue. I think the issue results from the implementation of Django's File.__bool__ method. It makes the File evaluate to False in the if statement if file_obj:. The __bool__ method only tests for the filename. Therefore all file uploads without a filename are rejected.

The restframework tests for the parsers are testing these cases and explicitly don't allow uploads without a filename.

If you add a filename via the HTTP_CONTENT_DISPOSITION header, it will work. I think the raised ParseError message FileUpload parse error - none of upload handlers can handle the stream is misleading in this case and should be changed, but after all it's not a bug.

@tomchristie
Copy link
Member

tomchristie commented Dec 9, 2015

Good digging @gregmuellegger!

@yuyou
Copy link
Author

yuyou commented Dec 9, 2015

Very good explanation and thanks to @gregmuellegger. I tested with CURL command:
curl -XPOST -H "Content-type: image/jpg" --data-binary @./app/test_assets/image.jpg -v -H 'Content-Disposition: attachment;filename="image.jpg"' "http://127.0.0.1:8000/media/"

and it works. So shall I close this issue?

@tomchristie
Copy link
Member

tomchristie commented Dec 9, 2015

So shall I close this issue?

Thanks, but no - we still have behavior here we should improve. 😄

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.4 Release, 3.4.0 Release Feb 11, 2016
@BraisGabin
Copy link

BraisGabin commented Jul 21, 2016

I did a git bisect and I found that this issue was added in 566812a. The test of my sample code https://github.com/BraisGabin/drf_3_3_test starts to fail with that commit.

@tomchristie tomchristie changed the title Is it a bug in FileUploadParser? FileUploadParser should use more descriptive error when filename not included. Aug 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants