Admin inlines with file/image field fails to save_as

[paulos]

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

Result:

• 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/models.py

  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/admin.py

  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
  
  admin.site.register(Foo, FooAdmin)
  

Traceback:

Django Version: 1.2.3
Python Version: 2.6.5
Installed Applications:
['django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites',
 'django.contrib.messages',
 'django.contrib.admin',
 'django.contrib.admindocs',
 'bugtest.dummy',
]
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware')

File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/core/handlers/base.py" 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/options.py" 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/decorators.py" 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/cache.py" 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/sites.py" 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/decorators.py" 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/decorators.py" 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/decorators.py" 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/transaction.py" 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/options.py" 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/models.py" in __init__
  704.                                                 queryset=qs)
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/forms/models.py" 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/formsets.py" in __init__
  47.         self._construct_forms()
File "/usr/local/lib/python2.6/dist-packages/Django-1.2.3-py2.6.egg/django/forms/formsets.py" 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/models.py" 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/models.py" 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/subclassing.py" 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/subclassing.py" 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/__init__.py" 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/__init__.py" 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/__init__.py" in get_prep_value
  479.         return int(value)

[paulos]

Seems to be related to #13223.

[Julien Phalip]

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".

[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...).

[mortas.11@…]

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?

[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.

[Chris Beaven]

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

[Ramiro Morales]

So, this ticket report and comments seem to be actually about three different issues about the admin Edit view of a model with inlines of instances of a model that has a FK to it and contains a FileField or ImageField:

1. Pressing the "Save as" button reports back a validation error and the following:
   1. The image/file fields of inlines already existing in the DB/FS will be 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.

2. 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.

3. When the error of item 1 happens, the save_as_new button gets replaced with a save_and_add_another

I propose to limit this ticket to item 1.

Item 2 is already covered by #13223.

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

[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)

[Mathijs de Bruin]

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. :)

[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.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[bmihelac]

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

​https://github.com/django/django/pull/697

What do you think?

[Florian Apolloner]

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.

[bmihelac]

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

[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.

[bmihelac]

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

[bmihelac]

Reworked pull request in:

​https://github.com/django/django/pull/2246