Opened 10 years ago

Last modified 7 years ago

#14760 new Bug

Admin inlines with file/image field fails to save_as

Reported by: paulos Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Normal Keywords:
Cc: mortas.11@…, mathijs@…, bmihelac@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


How to reproduce:

  • declare an admin inline which "model" property referes to a model containing an image field and another char field
  • declare a parent ModelAdmin having save_as=True
  • in the admin, create and save a new parent with one or more inlines
  • try to save_as


  • the form does not validate, image or file field will be empty with error: "This field is required.", extra inlines gone.
  • if you try to upload other file admin blows up with ValueError: invalid literal for int() with base 10:

Test case:

  • dummy/
    from django.db import models
    class Foo(models.Model):
        name = models.CharField(max_length=30)
    class Bar(models.Model):
        parent = models.ForeignKey(Foo)
        title = models.CharField(max_length=30)
        img = models.ImageField(upload_to='testpath')
  • dummy/
    from django.contrib import admin
    from bugtest.dummy.models import *
    class BarInline(admin.StackedInline):
        model = Bar
    class FooAdmin(admin.ModelAdmin):
        inlines = (BarInline,)
        save_as = True, FooAdmin)


Django Version: 1.2.3
Python Version: 2.6.5
Installed Applications:
Installed Middleware:

File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/core/handlers/" in get_response
  100.                     response = callback(request, *callback_args, **callback_kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/contrib/admin/" in wrapper
  239.                 return self.admin_site.admin_view(view)(*args, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/utils/" in _wrapped_view
  76.                     response = view_func(request, *args, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/views/decorators/" in _wrapped_view_func
  69.         response = view_func(request, *args, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/contrib/admin/" in inner
  190.             return view(request, *args, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/utils/" in _wrapper
  21.             return decorator(bound_func)(*args, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/utils/" in _wrapped_view
  76.                     response = view_func(request, *args, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/utils/" in bound_func
  17.                 return func(self, *args2, **kwargs2)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/db/" in _commit_on_success
  299.                     res = func(*args, **kw)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/contrib/admin/" in add_view
  792.                                   prefix=prefix, queryset=inline.queryset(request))
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/forms/" in __init__
  704.                                                 queryset=qs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/forms/" in __init__
  429.         super(BaseModelFormSet, self).__init__(**defaults)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/forms/" in __init__
  47.         self._construct_forms()
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/forms/" in _construct_forms
  97.             self.forms.append(self._construct_form(i))
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/forms/" in _construct_form
  717.         form = super(BaseInlineFormSet, self)._construct_form(i, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/forms/" in _construct_form
  448.                 connection=connections[self.get_queryset().db])
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/db/models/fields/" in inner
  53.             return func(*args, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/db/models/fields/" in inner
  53.             return func(*args, **kwargs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/db/models/fields/" in get_db_prep_lookup
  306.             value = self.get_prep_lookup(lookup_type, value)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/db/models/fields/" in get_prep_lookup
  292.             return self.get_prep_value(value)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/db/models/fields/" in get_prep_value
  479.         return int(value)

Change History (23)

comment:1 Changed 10 years ago by paulos

Resolution: duplicate
Status: newclosed

comment:2 Changed 10 years ago by paulos

Resolution: duplicate
Status: closedreopened

Seems to be related to #13223.

comment:3 Changed 10 years ago by Julien Phalip

Triage Stage: UnreviewedDesign decision needed

By its nature, a FileField or ImageField can only have one file or image attached to a given object. This means you would have to upload a separate file or image for each new object. By clicking "Save as new", you seem to expect that the file or image would be somehow duplicated and attached to the new object. The problem is that the file or image should also physically be duplicated on the filesystem - currently this is not the case so you have to manually re-upload the file or image for the new object.

Some design discussion needs to happen to decide whether the file or image should be duplicated on the filesystem or not when clicking "Save as new".

comment:4 Changed 10 years ago by Chris Beaven

There's nothing in the "nature" of a FileField stating that you can't have multiple fields attached to the same file.

In fact, there's logic in the delete method to avoid the underlying file getting deleted if any other fields in the same model reference it (granted, the very fact the underlying file is deleted is now under consideration as a bug, but that's an aside...).

comment:5 Changed 10 years ago by mortas.11@…

Cc: mortas.11@… added

Also, when I reproduced this bug, I've noticed that save_as_new button also disappears (replaced with save_and_add_another).

This bug was discribed in #3374.
It's quite old so may be it should be considered with this ticket together?

comment:6 Changed 10 years ago by Shu Zong Chen

Re save_as_new button disappearing: I think that's by design. The admin view changes from change_view to add_view (that is, it puts you on a view where it appears that you've tried to add a new instance). The add_view has a different set of buttons.

comment:7 Changed 10 years ago by Chris Beaven

It's a side-effect of the current design, but it's still a bug not a "by design" feature.

comment:8 Changed 9 years ago by Ramiro Morales

So, this ticket report and comments seem to be actually about three different issues involving:

  • The admin Edit view of a model with inlines of instances of a model that has a FK to it and that contains a FileField or ImageField.
  • A ModelAdmin with save_as=True.

The issues are:

  1. Pressing the "Save as" button reports back a validation error and:
    1. The image/file fields of inlines already existing in the DB/FS are empty and show "This field is required." message error.
    2. Values of other fields of the already existing inlines are preserved.
    3. All the information filled by the user in extra inlines is lost.
  1. If, in the resulting admin Add view, new files are selected from the local FS for the fields reporting validation errors. You get a 'ValueError?: invalid literal for int() with base 10' error.
  1. When the error of item 1 happens, the save_as_new button gets replaced with a save_and_add_another button.

I propose to limit this ticket to item 1.

Item 2 is already covered by #13223. That ticket seems to suggest the problem isn't specific to inlines containing Image/FileField's.

Item 3 is already covered by #5681, please see ticket:5681#comment:4 for a different opinion about it.

Last edited 9 years ago by Ramiro Morales (previous) (diff)

comment:9 Changed 9 years ago by Chris Beaven

Thanks for clarifying this, ramiro.

Regarding item 3, I looked over that other ticket yesterday and I'm happy enough with your opinion considering it works like that now (while discussing here I had forgotten I was monkey-fixing this for a client on a 1.0 branch)

comment:10 Changed 9 years ago by Mathijs de Bruin

Cc: mathijs@… added

Any news on the design decision here? Whether the 'physical' file gets duplicated or not, it'd be great to see this issue fixed when 1.3 comes out - or some time soon thereafter.

When making this decision, I reckon it is important to consider the consequences of both strategies:

  1. Only the reference gets duplicated: this might conflict slightly with the users' expectation of the saved object getting duplicated. However, it seems to me there are no further consequences. Unlike the related inline objects, which are in fact duplicated, file objects are not usually changed in place. Moreover, copying a file object might be an 'expensive' operation in that it takes up CPU while doing so, will take up disk space _and_ will potentially hurt client cache efficiency.
  2. The stored file object will be duplicated and the new object will contain a reference to this new file. As stated above, this will imply a loss of efficiency with regards to storage, CPU and - potentially - server traffic. However, it is the only solution fully consistent with the expected behaviour from the user side. Also, it is the only behaviour that allows for in-place editing of file objects in a consistent manner.

Generally, I think it would be wise to go with option 2 by default as the other option might cause unexpected errors due to unconsistent behaviour. This is potentially harmful as it might lead to dataloss when files are somehow edited in-place. Option 1 might, however, be a neat optional feature - perhaps to be included in a 1.3.X release.

Just a thought. :)

comment:11 Changed 9 years ago by Russell Keith-Magee

To remove any illusions -- this *will not* happen for 1.3. 1.3 is now a release candidate; the only fixes being applied at this point are for critical bugs.

comment:12 Changed 9 years ago by anonymous

Severity: Normal
Type: Bug

comment:13 Changed 8 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:14 Changed 8 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:15 Changed 8 years ago by bmihelac

Cc: bmihelac@… added

comment:16 Changed 8 years ago by bmihelac

This pull request addresses assigning initial values for FileFields and its subclasses:

What do you think?

comment:17 Changed 7 years ago by Aymeric Augustin

Status: reopenednew

comment:18 Changed 7 years ago by Florian Apolloner

Triage Stage: Design decision neededAccepted

Moving out of DDN since it is a valid issue, the safer solution would be to copy the file if the storage backends would allow this.

comment:19 Changed 7 years ago by Tim Graham

Has patch: set
Patch needs improvement: set

comment:20 Changed 7 years ago by bmihelac

timo, I can rework PR for master if it is acceptable to not copy files.

comment:21 Changed 7 years ago by Carl Meyer

I don't see any need to copy files here. Since we made the decision to remove file-deletion from FileField entirely I think the design approach is that a FileField does not "own" the file, it just references it, so IMO there is no problem having FileField on multiple model instances referencing the same file. Unless someone is doing something unusual, any updates to any of those models will involve uploading a new file anyway, not changing the existing one in-place.

comment:22 Changed 7 years ago by bmihelac

Good, I can make new PR based on this ASAP.

comment:23 Changed 7 years ago by bmihelac

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