Admin changelist shows no rows with a non-zero count where ForeignKeys are used in list_display and data is bad

[afitzpatrick]

If you have a record with a foreign key pointing to a non-existent object and that foreign key is included is list_display, the record will not be shown in the admin changelist.

The qs.select_related() call on line 210 of admin/views/main.py in Django 1.0.2-final won't return any rows with bad FK references, as expected.

However, whilst admin tool knows that there are X records to display, select_related() will return (X - number_of_bad_fks) for display. The tool should be able to figure out whether X != (X - number_of_bad_fks) and say "Hey, I'm about to tell you there are 10 records but only show you 5 and not explain why...".

I've been plagued by this problem whilst working on a fairly large application where the data is getting hacked about by a bunch of people. I wrote a little hacky patch for 1.0.2, which I'm sure reviewers won't like. I'll be happy to update for HEAD and fix the way I raise the exception if there's a better way to do this:

--- main.py	2009-07-11 18:24:11.450376926 +0100
+++ /home/afitzpatrick/main-patch.py	2009-07-11 18:24:07.138416529 +0100
@@ -197,8 +197,6 @@ class ChangeList(object):
 
         # Use select_related() if one of the list_display options is a field
         # with a relationship.
-        result_count = len(qs)
-
         if self.list_select_related:
             qs = qs.select_related()
         else:
@@ -212,9 +210,6 @@ class ChangeList(object):
                         qs = qs.select_related()
                         break
 
-        if len(qs) != result_count:
-             raise ValueError( 'select_related() returned %d records but %d were expected; there is likely a foreign key mismatch' % (len(qs), result_count) )
-
         # Set ordering.
         if self.order_field:
             qs = qs.order_by('%s%s' % ((self.order_type == 'desc' and '-' or ''), self.order_field))

To replicate the problem, assume something like this:

class Country(models.Model):
	iso = models.CharField(max_length=2, primary_key=True)

class Company(models.Model):
	name = models.CharField(max_length=128)
	country = models.ForeignKey(Country)

class CompanyAdmin(admin.ModelAdmin):
	list_display = ['name', 'company']`

Create a single record in the Company table where the country_id field references a Country that doesn't exist, then try to view all Company objects in the admin tool. It'll say "1 Companys" but none will be shown. Django's generated query code (below) will return no records, as it should:

SELECT `app_company`.`id`, `app_country`.`iso` INNER JOIN `app_company` ON (`app_country`.`id` = `app_company`.`iso`)

[Alex Gaynor]

Marking as invalid. If your data model is invalid this is a problem with your data, either mark the ForeignKey as nullable (if that's whats in your DB), or correct your data. Django be expeected to give correct answers in the presence of invalid data, garbage in garbage out :)

[afitzpatrick]

I absolutely agree re garbage in garbage out: if there's a problem with the data, the admin tool should not be expected to work properly.

However, if there's an obvious data problem, the admin tool shouldn't say "there's ten records" but only display nine. It just doesn't make any sense: clearly something is very wrong and we shouldn't pretend it's OK.

No need to handle it gracefully: just make it throw a dirty great exception, rather than pretending everything is OK. Either data is OK, or it's not. At the moment the admin tool handles shades of grey, which is unnecessary and confusing. No?

[Karen Tracey]

I don't know about rejecting this so fast. It's very confusing behavior for people who happen to have broken data like this (I know I've debugged it at least once on the user's list, unless there's some other way to get more results listed than displayed). Sure, best fix is to only have valid data but saying that doesn't really help anyone who happens to have gotten themselves into (or inherited) a broken situation. I don't think it's totally absurd to consider improving the admin behavior here.

However, I don't like the proposed fix of making the admin generate an exception...bad data should not make the admin fall over and die. I'd prefer some sort of error message noting the anomaly and hopefully pointing towards how to fix it. (Actually I'd prefer a display of all records highlighting the broken ones as broken, but that seems like too much special-case code for what is hopefully not all THAT common a problem.)

Wasn't going to reopen because I certainly don't have time to do anything useful with this, but in the meantime it's been reopened. I'll just note we prefer to avoid open/close/reopen/close/etc fights in tickets by raising the issue on the dev list -- this gives it a wider audience than people who follow the tracker closely. Right now is probably a bad time to raise an issue like this given the focus on finishing up 1.1, but after 1.1 goes out, and if you can come up with a better answer than making the admin generate an exception, you might want to pursue that avenue to see if there's wider support than just me for improving admin's behavior in this situation.

[afitzpatrick]

@kmtracey. Thanks for your comments. I agree that it would be great if the admin tool could highlight these bad records, though I suspect that might be beyond its remit. I have joined the developer list and could look at writing that code post 1.1 if there's appetite for it, and if we can establish what the correct behaviour should be.

[rlaager@…]

I like the idea of displaying an error. This bit us and it was very hard to track down. A simple error message would've saved a lot of time.

[anonymous]

Adding a note to the ​docs may be enough to save developers some grief (this would have helped me). Maybe something along the lines of: "Inconsistent row counts may be caused by missing foreign key values or a foreign key field incorrectly set to nullable=False."

[Julien Phalip]

The admin's codebase is already large and complicated enough taking care of all normal business that I think we shouldn't add more code taking care of problems that are out of the admin's control in the first place. We've got to draw the line somewhere.

I think the right approach is, like it's been suggested, to improve the documentation -- maybe by adding a "Troubleshoot" section which could grow over time when new problems of this kind pop up in the future.

[Julien Phalip]

A similar "issue" was reported in #15999, so one could argue that this is actually a frequently asked question :)
There already is an FAQ for the admin: ​http://docs.djangoproject.com/en/dev/faq/admin/ so it could be referenced there.

The FAQ doesn't have much visibility though. That should probably be improved too so that users don't miss it.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Tim Graham]

Adding doc patch

[holdenweb]

Would it perhaps improve the patch to point out that this situation occurs because Django models are declaring integrity constraints that are not implemented at the database level. This would be helpful for those who may not realize it's even possible to do that, though experienced admins will understand the issue.

[Tim Graham <timograham@…>]

In d08096317ab598b7a350d61b9b8396a3be7b8c79:

Fixed #11460 - Added a FAQ regarding missing rows in the admin.