Opened 5 years ago

Closed 22 months ago

Last modified 9 months ago

#13721 closed New feature (fixed)

pass extra content-type values to file upload handlers

Reported by: wkornewald Owned by: mvschaik
Component: File uploads/storage Version: master
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 wkornewald 4 years ago.
patch (with unit tests and docs)
content_type_extra.2.diff (9.3 KB) - added by wkornewald 4 years ago.
patch against trunk
content_type_extra.3.diff (9.9 KB) - added by mvschaik 2 years ago.
Updated patch to apply to current master

Download all attachments as: .zip

Change History (27)

comment:1 Changed 5 years ago by wkornewald

  • Component changed from Uncategorized to File uploads/storage
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by adamnelson

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 Changed 5 years ago by wkornewald

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 Changed 5 years ago by adamnelson

  • Triage Stage changed from Unreviewed to Design 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 Changed 5 years ago by wkornewald

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

comment:6 Changed 4 years ago by wkornewald

  • 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 Changed 4 years ago by lrekucki

  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Design decision needed to Accepted

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

Changed 4 years ago by wkornewald

patch (with unit tests and docs)

comment:8 Changed 4 years ago by wkornewald

  • 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 Changed 4 years ago by wkornewald

  • Cc wkornewald@… added

comment:10 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:11 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

content_type_extra.diff fails to apply cleanly on to trunk

Changed 4 years ago by wkornewald

patch against trunk

comment:12 Changed 4 years ago by wkornewald

  • milestone changed from 1.3 to 1.4
  • Patch needs improvement unset
  • Version changed from 1.2 to SVN

I've updated the patch.

comment:13 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:14 Changed 3 years ago by wkornewald

  • Cc wkornewald@… removed
  • UI/UX unset

comment:15 Changed 2 years ago by grant.callaghan@…

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

comment:16 Changed 2 years ago by anonymous

  • Easy pickings set

comment:17 Changed 2 years ago by mvschaik

  • Owner changed from nobody to mvschaik
  • Status changed from new to assigned

Changed 2 years ago by mvschaik

Updated patch to apply to current master

comment:18 Changed 2 years ago by kagia

  • 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 Changed 2 years ago by kagia

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

comment:20 Changed 22 months ago by timo

  • Cc timograham@… added
  • Patch needs improvement unset

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

comment:21 Changed 22 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In b0953dc91385fb2823294a76d3c99e01c7b7e4ee:

Fixed #13721 -- Added UploadedFile.content_type_extra.

Thanks Waldemar Kornewald and mvschaik for work on the patch.

comment:22 Changed 14 months ago by Tim Graham <timograham@…>

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 Changed 9 months ago by Gabe

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 Changed 9 months ago by timo

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