prepare_lookup_value should treat 0 as False for __isnull

[benjie@…]

In contrib.admin.util, there is a prepare_lookup_value function

In this function, there is a clause

    if value.lower() in ('', 'false'):
            value = False

I think it should be

    if value.lower() in ('', 'false', '0'):
          value = False

Note that in admin already converts a limit_choices_to={'aisnull': False} to aisnull=0 when formatting a query string for lookup on the remote model. Therefore, clearly it should also handle isnull=0 correctly. Currently the lookup popup box on remote model uses isnull=0 and does not actually filter.

See the following pull request: ​https://github.com/django/django/pull/982

[Baptiste Mispelon]

Hi,

Could you provide some more information about this issue?
It's not clear to me if you're describing a bug or suggesting an improvement.

If it's the former, can you include some regression tests in the pull request?

If it's the latter, you need to explain why you think this improvement is worthwile and address the potential backwards-compatibility issues.

Also note that there is a comment on line 40 that you need to keep in sync with what the code does.

Thanks.

[benjie@…]

Django admin uses prepare_lookup_value to parse somefield__isnull=0 in the change list URL. Unfortunately, prepare_lookup_value parses the above to be {"somefield__isnull": True} filter rule, resulting in a changelist showing objects you don't want. The pull request (now with correct comments, thanks) fixes this.

This is a bug, because Django admin uses the above as a filter when handling a field with limits_choices_to param. E.g.

  class B:
    ...
    a = models.ForeignKey(A, limits_choices_to={"somefield__isnull":False})

In this case, Django admin creates a lookup URL on A with querystring=somefield__isnull=0 some where in the DOM. Some javascript then translates this into a popup window, showing the changelist view of A, with somefield__isnull=0 in the URL. Therefore, in this case, your popup window is not what it should be, based on the definition in the model.

[anonymous]

Sorry, I am having problems with this ticket submission thing... Hence the wrong status changes...

[Baptiste Mispelon]

OK, this is indeed a bug.

I don't know the admin too well, so could you explain why you chose to fix admin.utils.prepare_lookup_value instead of admin.widgets.url_params_from_lookup_dict for example? In particular, does this change cause any backwards-compatibility issue?

Also, as noted in my previous comment, you will need to write a regression test if you want your pull request to be merged (tests/admin_views/tests.py might be a good starting point).

Finally, while digging in I found the following comment in ​https://github.com/django/django/blob/master/django/db/models/fields/__init__.py#L644-L647 (found through a comment in url_params_from_lookup_dict)

It seems related to your issue so don't forget to amend it if you change the behavior described in the comment.

Thanks for your report.

[benjie@…]

Okay, thanks for the comments.

Yes, there definitely are two possible fixes for this behavior. One is to change admin.widgets.url_params_from_lookup_dict, to convert False to "false" rather than "0". The other fix is the one I included. I chose my fix because it would also make how change list view parses filters from URL similar to how the queryset in models work. For example, in queryset, the following two queries give the same results

>>> Compound.objects.filter(reference_reagent_id__isnull=False).count()
67
>>> Compound.objects.filter(reference_reagent_id__isnull=0).count()
67

I can't imagine anything that depended on admin.utils.prepare_lookup_value not to parse field__isnull=0 to mean the field should not be None. A simple search in the admin code shows that admin.utils.prepare_lookup_value is only used in the changelist view, in filtering objects and determine what default values to use for the defined filters.

I think the comment you found in url_params_from_lookup_dict , and the behavior in db.models.fields.BooleanField , indicate that in general boolean values are translated to '0' and '1', not 'true' and 'false'. My fix does not change that but in fact conforms to that behavior. So no changes to comments in db.models.fields.BooleanField is necessary I believe. If anything, I think it may be questionable that admin.utils.prepare_lookup_value actually handles the string literal "false". But changing that behavior could have backward incompatibility issues.

I added the necessary tests. See the latest commit on the pull request, also here

​https://github.com/benjiec/django/commit/df062cee87f7da852a3d834d4836a03058770f01

Thanks.

[Baptiste Mispelon]

Looking good.

I think the comment in the test_limit_choices_to_null shouldn't be there (I don't understand what it means, even in the context of the previous test that you copied).

I don't know the admin well so I'll ask for a review before marking this as "ready for checkin".

Could you pull the latest changes from master and create a pull request on github (that will make it easier to review)?

Thanks.

[benjie@…]

I updated the comments, and also clarified the comments from the test I copied. So they should be clear now.

Made a new pull request.

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