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 breaks with empty file names and multiple upload handlers #2109

Closed
skrivanos opened this issue Nov 21, 2014 · 6 comments
Closed
Labels
Milestone

Comments

@skrivanos
Copy link

skrivanos commented Nov 21, 2014

When there are multiple upload handlers defined and one of them raises StopFutureHandlers (in my case, MemoryFileUploadHandler and TemporaryFileUploadHandler) the self.file property is never set on the remaining handlers (since this is usually done in the new_file method).

This causes issues when iterating over the handlers later trying to complete the file:

class FileUploadParser():
        for i, handler in enumerate(upload_handlers):
            file_obj = handler.file_complete(counters[i])
            if file_obj:
                return DataAndFiles(None, {'file': file_obj})
        raise ParseError("FileUpload parse error - "
                         "none of upload handlers can handle the stream")
class File():
    def __bool__(self):
        return bool(self.name)

With an empty filename, since bool(file_obj) == False, it'll continue the iteration even the handler that issued StopFutureHandlers earlier. Since the file property wasn't set on the remaining handlers handler.file_complete will raise an AttributeError:

AttributeError:'TemporaryFileUploadHandler' object has no attribute 'file'
@tomchristie
Copy link
Member

tomchristie commented Nov 21, 2014

Any chance you confirm if this issue also affect Django core. We're using the same file parsing as from core so would be good to know if there's also a bigger issue to be fixed?

@skrivanos
Copy link
Author

skrivanos commented Nov 21, 2014

Django's parser skips files without a filename completely:

file_name = disposition.get('filename')
if not file_name:
    continue

I guess the question is whether or not to allow raw uploads without a filename. In my opinion there's a use case for where file names might be set beforehand since we're handling raw data.

@tomchristie
Copy link
Member

tomchristie commented Nov 26, 2014

Also confirmed by #2137.

@tomchristie tomchristie modified the milestone: 3.0.1 Release Dec 1, 2014
@brianmay
Copy link

brianmay commented Dec 2, 2014

Have I understood this correctly?

diff --git a/rest_framework/parsers.py b/rest_framework/parsers.py
index ccb82f0..cf5de4a 100644
--- a/rest_framework/parsers.py
+++ b/rest_framework/parsers.py
@@ -256,12 +256,15 @@ class FileUploadParser(BaseParser):
         chunks = ChunkIter(stream, chunk_size)
         counters = [0] * len(upload_handlers)

+        new_handlers = []
         for handler in upload_handlers:
             try:
                 handler.new_file(None, filename, content_type,
                                  content_length, encoding)
+                new_handlers.append(handler)
             except StopFutureHandlers:
                 break
+        upload_handlers = new_handlers

         for chunk in chunks:
             for i, handler in enumerate(upload_handlers):

Unfortunately this doesn't solve #2137, but I do at least get a cleaner error message now. Will followup with the other ticket if this patch looks ok.

@tomchristie
Copy link
Member

tomchristie commented Dec 2, 2014

Not sure @brianmay, havn't investigated this ticket yet, myself.

@tomchristie
Copy link
Member

tomchristie commented Dec 2, 2014

Thanks for the reports & investigation folks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants