Admin ignores to_field on ForeignKey

[Karen Tracey]

As reported here: ​http://groups.google.com/group/django-users/browse_thread/thread/47d31f56c1924cfa#

With current (I'm running [8661]) admin, for a model specified like so:

class Inventory(models.Model):
   barcode = models.PositiveIntegerField(unique=True)
   parent = models.ForeignKey('self', to_field='barcode', blank=True, null=True)
   name = models.CharField(blank=False, max_length=20)
   def __unicode__(self):
      return self.name

The admin treats 'parent' as though it refers to the pk field, not the barcode field.

Old admin (0.96) handled this properly, that is it showed information for the related model that matched on the barcode field, not the pk field.

[Karen Tracey <kmtracey@…>]

The problem only affects display, the correct value is actually stored in the DB. The admin just doesn't show it correctly. Given a table for the specified model that contains these rows:

+----+---------+-----------+--------------------+
| id | barcode | parent_id | name               |
+----+---------+-----------+--------------------+
|  1 |      86 |      NULL | Apple iMac         | 
|  2 |      22 |      NULL | Lenovo Thinkpad    | 
|  3 |      87 |        86 | Apple Mighty Mouse | 
+----+---------+-----------+--------------------+

The select widget for 'parent' in the change page for "Apple Mighty Mouse" will have '-----' selected because no match is found for id=86. If 'parent' is listed in raw_id_fields the admin will generate an exception when it tries to get the object in the table with id=86. I'll attach a patch that fixes both of these, though I'm not very familiar with this code so someone with a clue should see if this is even the right place to fix things.

There is some resulting oddness with the raw_id_fields case and the patch. For entry, you must enter the pk value of the associated object (which is what the magnifying glass lookup will do for you if you use it), but when you hit "Save and continue editing" that value will be transformed to the to_field value for the entered pk. This could be somewhat confusing and lead people to think they need to enter the to_field values, which won't work properly.

[magneto]

i wonder if #8562 and this are related in nature?

[Karen Tracey]

From what I can tell, this is not related to #8562. The main problem there seems to be the inline forms for a one-to-one field do not have a hidden field containing the pk value for the inline-edited object. This one has to do with what is displayed for a ForeignKey field where to_field has been specified. The two don't seem to overlap.

Fixed the patch so the ForeignKeyRawIdWidget's render doesn't crash when it's actually a ManyToManyRawIdWidget being rendered. Does this need tests? I could look into writing a couple but confirmation I'm in the right neighborhood of a correct fix for this would be helpful first.

[Brian Rosner]

I suspect that the patch here doesn't solve the problem in the right place. Does this problem exist a level up in ModelChoiceField too? I was looking through tests and it appears there isn't any that test the output of a field/widget combo. It appears that is where this needs to be tested. However, this is much easier to work with in the admin like the patch provides since we are controlling the widget and the widget used in a ModelChoiceField is a standard don't care about the database widget. This is seeming a bit more tricky than what it looks.

[Karen Tracey]

Yeah, I agree fixing it at the widget level seems not quite the right place. It'd be better to have a fix in one place, that works regardless of the widget. Only...I don't know where that place is. It's the widget (or, at any rate the admin widgets) that have saved a reference to the actual related field which includes the name of to_field, and it is the widget that gets handed the current value to render. That value is currently the value stored in the database for the field, and it needs to be translated to a PK value for things to work right.

Now there is code somewhere for the "saving" case that takes a PK value and translates it to the to_field value for actual saving in the DB. It would seem to be a good symmetrical solution to have the transformation in the other direction done at the same 'level', only I'm not sure where that is. I think it is somewhere in django/db/models/fields/related.py that's doing the transform for the setting case. Maybe the corresponding get code isn't being called when it should be to transform the DB value from the to_field value to pk value? But reading through that code it's a bit mysterious to me, and I fear I had too much sun today and not enough sleep last night to puzzle through it right now....

[Chris Beaven]

Taking a step back, it seems that the choices should be using the to_field as it's key rather than the primary key. I attached a patch (with tests) which does this.

[Karen Tracey]

Replying to SmileyChris:

Taking a step back, it seems that the choices should be using the to_field as it's key rather than the primary key. I attached a patch (with tests) which does this.

I verified that works for the default case, but there's still a problem (same exception as without the patch) if that field is included in raw_id_fields. Attempting to bring up a change page for an Inventory with non-null parent, I get:

Environment:

Request Method: GET
Request URL: http://localhost:8000/admin/selfref/inventory/3/
Django Version: 1.0-beta_2-SVN-8781
Python Version: 2.5.2
Installed Applications:
['django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites',
 'django.contrib.admin',
 'django.contrib.sitemaps',
 'django.contrib.flatpages',
 'playground.selfref']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.middleware.doc.XViewMiddleware',
 'django.contrib.flatpages.middleware.FlatpageFallbackMiddleware')


Template error:
In template d:\u\kmt\django\trunk\django\contrib\admin\templates\admin\includes\fieldset.html, error at line 11
   Caught an exception while rendering: Inventory matching query does not exist.
   1 : <fieldset class="module aligned {{ fieldset.classes }}">
   2 :   {% if fieldset.name %}<h2>{{ fieldset.name }}</h2>{% endif %}
   3 :   {% if fieldset.description %}<div class="description">{{ fieldset.description|safe }}</div>{% endif %}
   4 :   {% for line in fieldset %}
   5 :       <div class="form-row{% if line.errors %} errors{% endif %} {% for field in line %}{{ field.field.name }} {% endfor %} ">
   6 :       {{ line.errors }}
   7 :       {% for field in line %}
   8 :           {% if field.is_checkbox %}
   9 :               {{ field.field }}{{ field.label_tag }}
   10 :           {% else %}
   11 :               {{ field.label_tag }} {{ field.field }} 
   12 :           {% endif %}
   13 :           {% if field.field.field.help_text %}<p class="help">{{ field.field.field.help_text|safe }}</p>{% endif %}
   14 :       {% endfor %}
   15 :       </div>
   16 :   {% endfor %}
   17 : </fieldset>
   18 : 

Traceback:
File "d:\u\kmt\django\trunk\django\core\handlers\base.py" in get_response
  86.                 response = callback(request, *callback_args, **callback_kwargs)
File "d:\u\kmt\django\trunk\django\contrib\admin\sites.py" in root
  173.                 return self.model_page(request, *url.split('/', 2))
File "d:\u\kmt\django\trunk\django\views\decorators\cache.py" in _wrapped_view_func
  44.         response = view_func(request, *args, **kwargs)
File "d:\u\kmt\django\trunk\django\contrib\admin\sites.py" in model_page
  192.         return admin_obj(request, rest_of_url)
File "d:\u\kmt\django\trunk\django\contrib\admin\options.py" in __call__
  196.             return self.change_view(request, unquote(url))
File "d:\u\kmt\django\trunk\django\db\transaction.py" in _commit_on_success
  238.                 res = func(*args, **kw)
File "d:\u\kmt\django\trunk\django\contrib\admin\options.py" in change_view
  620.         return self.render_change_form(request, context, change=True, obj=obj)
File "d:\u\kmt\django\trunk\django\contrib\admin\options.py" in render_change_form
  402.         ], context, context_instance=template.RequestContext(request))
File "d:\u\kmt\django\trunk\django\shortcuts\__init__.py" in render_to_response
  18.     return HttpResponse(loader.render_to_string(*args, **kwargs), **httpresponse_kwargs)
File "d:\u\kmt\django\trunk\django\template\loader.py" in render_to_string
  107.     return t.render(context_instance)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  176.         return self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\loader_tags.py" in render
  97.         return compiled_parent.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  176.         return self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\loader_tags.py" in render
  97.         return compiled_parent.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  176.         return self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\loader_tags.py" in render
  24.         result = self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\defaulttags.py" in render
  148.                 nodelist.append(node.render(context))
File "d:\u\kmt\django\trunk\django\template\loader_tags.py" in render
  111.             return self.template.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  176.         return self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\defaulttags.py" in render
  148.                 nodelist.append(node.render(context))
File "d:\u\kmt\django\trunk\django\template\defaulttags.py" in render
  148.                 nodelist.append(node.render(context))
File "d:\u\kmt\django\trunk\django\template\defaulttags.py" in render
  246.             return self.nodelist_false.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  81.             raise wrapped

Exception Type: TemplateSyntaxError at /admin/selfref/inventory/3/
Exception Value: Caught an exception while rendering: Inventory matching query does not exist.

Original Traceback (most recent call last):
  File "d:\u\kmt\django\trunk\django\template\debug.py", line 71, in render_node
    result = node.render(context)
  File "d:\u\kmt\django\trunk\django\template\debug.py", line 87, in render
    output = force_unicode(self.filter_expression.resolve(context))
  File "d:\u\kmt\django\trunk\django\utils\encoding.py", line 49, in force_unicode
    s = unicode(s)
  File "d:\u\kmt\django\trunk\django\forms\forms.py", line 311, in __unicode__
    return self.as_widget()
  File "d:\u\kmt\django\trunk\django\forms\forms.py", line 339, in as_widget
    return widget.render(self.html_name, data, attrs=attrs)
  File "d:\u\kmt\django\trunk\django\contrib\admin\widgets.py", line 122, in render
    output.append(self.label_for_value(value))
  File "d:\u\kmt\django\trunk\django\contrib\admin\widgets.py", line 127, in label_for_value
    truncate_words(self.rel.to.objects.get(pk=value), 14)
  File "d:\u\kmt\django\trunk\django\db\models\manager.py", line 81, in get
    return self.get_query_set().get(*args, **kwargs)
  File "d:\u\kmt\django\trunk\django\db\models\query.py", line 301, in get
    % self.model._meta.object_name)
DoesNotExist: Inventory matching query does not exist.

[Chris Beaven]

Replying to kmtracey:

Replying to SmileyChris:

Taking a step back, it seems that the choices should be using the to_field as it's key rather than the primary key. I attached a patch (with tests) which does this.

I verified that works for the default case, but there's still a problem (same exception as without the patch) if that field is included in raw_id_fields. Attempting to bring up a change page for an Inventory with non-null parent, I get: [...]

Ok, I'm uploading a fix for that too...

[Karen Tracey]

I still get the same exception in the raw_id_fields case, even with 8648.2.diff. I did apply the right fix because the new test fails:

======================================================================
FAIL: Doctest: regressiontests.admin_widgets.models.__test__.WIDGETS_TESTS
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\u\kmt\django\trunk\django\test\_doctest.py", line 2180, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for regressiontests.admin_widgets.models.__test__.WIDGETS_TESTS
  File "D:\u\kmt\django\trunk\tests\regressiontests\admin_widgets\models.py", line unknown line number, in WIDGETS_TESTS


----------------------------------------------------------------------
File "D:\u\kmt\django\trunk\tests\regressiontests\admin_widgets\models.py", line ?, in regressiontests.admin_widgets.mod
els.__test__.WIDGETS_TESTS
Failed example:
    print w.render('test', core.parent_id, attrs={})
Exception raised:
    Traceback (most recent call last):
      File "D:\u\kmt\django\trunk\django\test\_doctest.py", line 1267, in __run
        compileflags, 1) in test.globs
      File "<doctest regressiontests.admin_widgets.models.__test__.WIDGETS_TESTS[35]>", line 1, in <module>
        print w.render('test', core.parent_id, attrs={})
      File "d:\u\kmt\django\trunk\django\contrib\admin\widgets.py", line 122, in render
        output.append(self.label_for_value(value))
      File "d:\u\kmt\django\trunk\django\contrib\admin\widgets.py", line 127, in label_for_value
        truncate_words(self.rel.to.objects.get(pk=value), 14)
      File "d:\u\kmt\django\trunk\django\db\models\manager.py", line 81, in get
        return self.get_query_set().get(*args, **kwargs)
      File "d:\u\kmt\django\trunk\django\db\models\query.py", line 301, in get
        % self.model._meta.object_name)
    DoesNotExist: Inventory matching query does not exist.


----------------------------------------------------------------------
Ran 1 test in 0.070s

FAILED (failures=1)
Destroying test database...

The ForeignKeyRawIDWidget is assuming that value is a PK value here:

    def label_for_value(self, value):
        return '&nbsp;<strong>%s</strong>' % \
            truncate_words(self.rel.to.objects.get(pk=value), 14)

So I see the test for this case but not the fix?

[Chris Beaven]

Apologies. I'm cherrypicking between two changes on the same branch and left out the admin change :P

[Karen Tracey]

Ah, OK. Yes, with .3 now the display is correct for the raw_id_fields case. But...the magnifying-glass icon can't be used to select a new value, because it returns the PK value. If you use the magnifying-glass icon to select a different related object, it fills in the PK value for the object you choose, not the to_field value. This, I fear, will lead to an incorrect relation being saved in the case where the PK values overlap the to_field values.

In cases where they don't, when you select any of the save alternatives you get the same traceback as before (except the code in admin/widgets.py is now obj = self.rel.to.objects.get(**{key: value})). The admin is actually attempting to re-display the form with an error message "Select a valid choice. That choice is not one of the available choices." but the invalid value in the raw select widget leads to an exception. This is actually a problem independent of this ticket (I can recreate it by just typing in an out-of-range PK value in a raw_id field that has no to_field specified) so this part may belong in a new ticket.

But the fact that choosing a new related object by using the looking glass icon results in a PK value, not a to_field value, being filled in for the field needs to be solved along with the rest of the to_field stuff, I think.

[Karen Tracey]

Opened #8746 to cover making admin more robust in the face of bad input placed in a raw id widget.

[Brian Rosner]

Karen, can you try the patch I attached to see if it fixes that last issue in getting this to work.

[Karen Tracey]

With 868.4.diff I cannot bring up the list of Inventories -- I get:

Environment:

Request Method: GET
Request URL: http://localhost:8000/admin/selfref/inventory/
Django Version: 1.0-beta_2-SVN-8818
Python Version: 2.5.2
Installed Applications:
['django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites',
 'django.contrib.admin',
 'django.contrib.sitemaps',
 'django.contrib.flatpages',
 'playground.selfref']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.middleware.doc.XViewMiddleware',
 'django.contrib.flatpages.middleware.FlatpageFallbackMiddleware')


Template error:
In template d:\u\kmt\django\trunk\django\contrib\admin\templates\admin\change_list.html, error at line 34
   Caught an exception while rendering: getattr(): attribute name must be string
   24 : {% if cl.has_filters %}
   25 : <div id="changelist-filter">
   26 : <h2>{% trans 'Filter' %}</h2>
   27 : {% for spec in cl.filter_specs %}
   28 :    {% admin_list_filter cl spec %}
   29 : {% endfor %}
   30 : </div>
   31 : {% endif %}
   32 : {% endblock %}
   33 : 
   34 : {% block result_list %} {% result_list cl %} {% endblock %}
   35 : {% block pagination %}{% pagination cl %}{% endblock %}
   36 : </div>
   37 : </div>
   38 : {% endblock %}
   39 : 

Traceback:
File "d:\u\kmt\django\trunk\django\core\handlers\base.py" in get_response
  86.                 response = callback(request, *callback_args, **callback_kwargs)
File "d:\u\kmt\django\trunk\django\contrib\admin\sites.py" in root
  173.                 return self.model_page(request, *url.split('/', 2))
File "d:\u\kmt\django\trunk\django\views\decorators\cache.py" in _wrapped_view_func
  44.         response = view_func(request, *args, **kwargs)
File "d:\u\kmt\django\trunk\django\contrib\admin\sites.py" in model_page
  192.         return admin_obj(request, rest_of_url)
File "d:\u\kmt\django\trunk\django\contrib\admin\options.py" in __call__
  188.             return self.changelist_view(request)
File "d:\u\kmt\django\trunk\django\contrib\admin\options.py" in changelist_view
  656.         ], context, context_instance=template.RequestContext(request))
File "d:\u\kmt\django\trunk\django\shortcuts\__init__.py" in render_to_response
  18.     return HttpResponse(loader.render_to_string(*args, **kwargs), **httpresponse_kwargs)
File "d:\u\kmt\django\trunk\django\template\loader.py" in render_to_string
  107.     return t.render(context_instance)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  176.         return self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\loader_tags.py" in render
  97.         return compiled_parent.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  176.         return self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\loader_tags.py" in render
  97.         return compiled_parent.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  176.         return self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\loader_tags.py" in render
  24.         result = self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  71.             result = node.render(context)
File "d:\u\kmt\django\trunk\django\template\loader_tags.py" in render
  24.         result = self.nodelist.render(context)
File "d:\u\kmt\django\trunk\django\template\__init__.py" in render
  768.                 bits.append(self.render_node(node, context))
File "d:\u\kmt\django\trunk\django\template\debug.py" in render_node
  81.             raise wrapped

Exception Type: TemplateSyntaxError at /admin/selfref/inventory/
Exception Value: Caught an exception while rendering: getattr(): attribute name must be string

Original Traceback (most recent call last):
  File "d:\u\kmt\django\trunk\django\template\debug.py", line 71, in render_node
    result = node.render(context)
  File "d:\u\kmt\django\trunk\django\template\__init__.py", line 915, in render
    dict = func(*args)
  File "d:\u\kmt\django\trunk\django\contrib\admin\templatetags\admin_list.py", line 238, in result_list
    'results': list(results(cl))}
  File "d:\u\kmt\django\trunk\django\contrib\admin\templatetags\admin_list.py", line 233, in results
    yield list(items_for_result(cl,res))
  File "d:\u\kmt\django\trunk\django\contrib\admin\templatetags\admin_list.py", line 225, in items_for_result
    result_id = repr(force_unicode(getattr(result, cl.to_field)))[1:]
TypeError: getattr(): attribute name must be string

{% result_list cl %} is highlighted on line 34. The admin.py I have is:

from django.contrib import admin
from selfref.models import Inventory

class InventoryAdmin(admin.ModelAdmin):
    list_display = ('name', 'barcode', 'parent')
    raw_id_fields = ('parent',)

admin.site.register(Inventory, InventoryAdmin)

but the error persists even if I don't customize the ModelAdmin at all (admin.site.register(Inventory)).

[Brian Rosner]

Let's try again.

[Karen Tracey]

Much better! The magnifying glass lookup returns the to_field value and save works properly. Also verified the default widget still works fine. It's looking good to me.

[Brian Rosner]

(In [8823]) Fixed #8648 -- Admin no longer ignores to_field. Thanks for the help Karen Tracey and SmileyChris.

[Jacob]

Milestone 1.0 deleted