Code

Opened 4 years ago

Last modified 5 months 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

Description

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)

Attachments (0)

Change History (23)

comment:1 Changed 4 years ago by paulos

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to duplicate
  • Status changed from new to closed

comment:2 Changed 4 years ago by paulos

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Seems to be related to #13223.

comment:3 Changed 4 years ago by julien

  • Triage Stage changed from Unreviewed to Design 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 3 years ago by SmileyChris

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 3 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 3 years ago by sirpengi

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 3 years ago by SmileyChris

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

comment:8 Changed 3 years ago by ramiro

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 3 years ago by ramiro (previous) (diff)

comment:9 Changed 3 years ago by SmileyChris

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 3 years ago by dokterbob

  • 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 3 years ago by russellm

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 3 years ago by anonymous

  • Severity set to Normal
  • Type set to Bug

comment:13 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:14 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:15 Changed 17 months ago by bmihelac

  • Cc bmihelac@… added

comment:16 Changed 17 months ago by bmihelac

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

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

What do you think?

comment:17 Changed 16 months ago by aaugustin

  • Status changed from reopened to new

comment:18 Changed 16 months ago by apollo13

  • Triage Stage changed from Design decision needed to Accepted

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

  • Has patch set
  • Patch needs improvement set

comment:20 Changed 5 months ago by bmihelac

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

comment:21 Changed 5 months ago by carljm

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 5 months ago by bmihelac

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

comment:23 Changed 5 months ago by bmihelac

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.