Opened 6 years ago

Closed 6 years ago

#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 Aymeric Augustin)

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:

Change History (12)

comment:1 Changed 6 years ago by Baptiste Mispelon

Cc: bmispelon@… added
Resolution: needsinfo
Status: newclosed


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.


comment:2 Changed 6 years ago by anonymous

Resolution: needsinfo
Status: closednew

comment:3 Changed 6 years ago by benjie@…

Resolution: fixed
Status: newclosed

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

Resolution: fixed
Status: closednew

comment:5 Changed 6 years ago by anonymous

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

comment:6 Changed 6 years ago by Baptiste Mispelon

Needs tests: set
Triage Stage: UnreviewedAccepted

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/ might be a good starting point).

Finally, while digging in I found the following comment in (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 6 years 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()
>>> Compound.objects.filter(reference_reagent_id__isnull=0).count()

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


comment:8 Changed 6 years ago by Baptiste Mispelon

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)?


comment:9 Changed 6 years 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.

comment:10 Changed 6 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:11 Changed 6 years ago by Aymeric Augustin

Description: modified (diff)

comment:12 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 0268aba96bf91de3d172cd6ce2e44c3ce33ef7a0:

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

Thanks Benjie Chen.

Note: See TracTickets for help on using tickets.
Back to Top