Code

#20182 closed Bug (fixed)

prepare_lookup_value should treat 0 as False for __isnull

Reported by: benjie@… Owned by: nobody
Component: contrib.admin Version: 1.5
Severity: Normal Keywords:
Cc: bmispelon@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by aaugustin)

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

Attachments (0)

Change History (12)

comment:1 Changed 16 months ago by bmispelon

  • Cc bmispelon@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

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.

comment:2 Changed 16 months ago by anonymous

  • Resolution needsinfo deleted
  • Status changed from closed to new

comment:3 Changed 16 months ago by benjie@…

  • Resolution set to fixed
  • Status changed from new to closed

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.

comment:4 Changed 16 months ago by anonymous

  • Resolution fixed deleted
  • Status changed from closed to new

comment:5 Changed 16 months ago by anonymous

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

comment:6 Changed 16 months ago by bmispelon

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:7 Changed 16 months ago by 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.

comment:8 Changed 16 months ago by bmispelon

  • Needs tests unset

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.

comment:9 Changed 15 months ago by 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

comment:10 Changed 14 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 14 months ago by aaugustin

  • Description modified (diff)

comment:12 Changed 14 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 0268aba96bf91de3d172cd6ce2e44c3ce33ef7a0:

Fixed #20182 - admin lookup should treat 0 as False for isnull

Thanks Benjie Chen.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.