Opened 14 years ago

Closed 11 years ago

Last modified 10 years ago

#13721 closed New feature (fixed)

pass extra content-type values to file upload handlers

Reported by: Waldemar Kornewald Owned by: mvschaik
Component: File uploads/storage Version: dev
Severity: Normal Keywords:
Cc: timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Currently, file upload handlers can only get the actual content type and charset, but App Engine's Blobstore file service passes a few extra vales to the content type which are needed to get the uploaded file's identifier ("blob-key"). Here's a sample snippet of an HTTP upload request's POST data:

--===============0880245784==
Content-Type: message/external-body; blob-key="WEexn9L82wky30ADBOWqYA=="; access-type="X-AppEngine-BlobKey"
MIME-Version: 1.0
Content-Disposition: form-data; name="file"; filename="django-nonrel.diff"

Content-Type: application/octet-stream
MIME-Version: 1.0
Content-Length: 8522
content-type: application/octet-stream
content-disposition: form-data; name="file"; filename="django-nonrel.diff"
X-AppEngine-Upload-Creation: 2010-06-08 15:34:32.685000


--===============0880245784==--

The attached patch changes the file upload handling code such that it passes a dictionary with all extra content type arguments to the upload handler. With this I was able to successfully upload files to a model's FileField via a ModelForm.

Attachments (3)

content_type_extra.diff (9.2 KB ) - added by Waldemar Kornewald 13 years ago.
patch (with unit tests and docs)
content_type_extra.2.diff (9.3 KB ) - added by Waldemar Kornewald 13 years ago.
patch against trunk
content_type_extra.3.diff (9.9 KB ) - added by mvschaik 11 years ago.
Updated patch to apply to current master

Download all attachments as: .zip

Change History (27)

comment:1 by Waldemar Kornewald, 14 years ago

Component: UncategorizedFile uploads/storage

comment:2 by Adam Nelson, 14 years ago

It seems like you could use this definition:

def new_file(self, field_name, file_name, content_type, content_length, 
	            charset=None, content_type_extra={}):

Then you wouldn't have to check if content_type_extra is None.

comment:3 by Waldemar Kornewald, 14 years ago

Maybe it's fine with a .copy(), but then that's an unnecessary copy. Normally, I don't pass mutable objects as default arguments because that can lead to hidden, hard to track bugs, so I've just settled on always using None. Is this a show-stopper? :)

comment:4 by Adam Nelson, 14 years ago

Triage Stage: UnreviewedDesign decision needed

Good point, we should add that to the style guide:

http://www.network-theory.co.uk/docs/pytut/DefaultArgumentValues.html

Nevertheless, this ticket needs to be approved by a committer first.

comment:5 by Waldemar Kornewald, 14 years ago

Still no response from a committer? :(
Is something missing?

comment:6 by Waldemar Kornewald, 13 years ago

Has patch: set

What does it take to get this committed into trunk? This change is absolutely necessary for App Engine's Blobstore and it's a really harmless change.

comment:7 by Łukasz Rekucki, 13 years ago

Needs documentation: set
Needs tests: set
Triage Stage: Design decision neededAccepted

Probably a simple test case and some docs for people that need this. I see no obvious reason not to do this.

by Waldemar Kornewald, 13 years ago

Attachment: content_type_extra.diff added

patch (with unit tests and docs)

comment:8 by Waldemar Kornewald, 13 years ago

Needs documentation: unset
Needs tests: unset

Done. I've added a simple test case and docs. Do you need anything else before it can be committed?

comment:9 by Waldemar Kornewald, 13 years ago

Cc: wkornewald@… added

comment:10 by Julien Phalip, 13 years ago

Severity: Normal
Type: New feature

comment:11 by patchhammer, 13 years ago

Easy pickings: unset
Patch needs improvement: set

content_type_extra.diff fails to apply cleanly on to trunk

by Waldemar Kornewald, 13 years ago

Attachment: content_type_extra.2.diff added

patch against trunk

comment:12 by Waldemar Kornewald, 13 years ago

milestone: 1.31.4
Patch needs improvement: unset
Version: 1.2SVN

I've updated the patch.

comment:13 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:14 by Waldemar Kornewald, 12 years ago

Cc: wkornewald@… removed
UI/UX: unset

comment:15 by grant.callaghan@…, 11 years ago

+1. I would really like to see this patch applied

comment:16 by anonymous, 11 years ago

Easy pickings: set

comment:17 by mvschaik, 11 years ago

Owner: changed from nobody to mvschaik
Status: newassigned

by mvschaik, 11 years ago

Attachment: content_type_extra.3.diff added

Updated patch to apply to current master

comment:18 by Benjamin Kagia, 11 years ago

Patch needs improvement: set

line 34 and 35 of the patch are made redundant by the previous line:

content_type_extra = meta_data.get('content-type', (0, {}))[1]

content_type_extra is then guaranteed not to be None.

comment:19 by Benjamin Kagia, 11 years ago

@mvschaik are you still working on this? You may want to take a look at this my branch

comment:20 by Tim Graham, 11 years ago

Cc: timograham@… added
Patch needs improvement: unset

Edited version of kagia's branch: https://github.com/django/django/pull/1301

comment:21 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In b0953dc91385fb2823294a76d3c99e01c7b7e4ee:

Fixed #13721 -- Added UploadedFile.content_type_extra.

Thanks Waldemar Kornewald and mvschaik for work on the patch.

comment:22 by Tim Graham <timograham@…>, 10 years ago

In d3e33fff12266b51ab1488f7094855aef6061e9e:

Made content_type_extra optional for TemporaryUploadedFile and MemoryUploadedFile.

This provides better backwards compatibility for those constructing these
objects manually.

Refs #13721.

comment:23 by Gabe, 10 years ago

Hi guys!

I'm using Django 1.5 with Python 2.7 running in Google App Engine. I got into this project recently, the former programmers didn't use the webapp framework, but now I have to be able to make file upload possible with large files (GAE doesn't allow more than 1 MB). I tried to make blobstore work, but I couldn't, then I started looking into this stuff and found this patch. Is it possible to make this patch work with GAE?

Thanks in advance!

Gabe

comment:24 by Tim Graham, 10 years ago

Hi Gabe, this ticket trackers isn't a support channel. Please see https://docs.djangoproject.com/en/stable/faq/help/ for ways to get help. Thanks!

Note: See TracTickets for help on using tickets.
Back to Top