Skip to content

Commit

Permalink
admin: use BitHandler instead of bitor operator
Browse files Browse the repository at this point in the history
`FieldListFilter.__init__` populates `used_parameters` as follows.

    for p in self.expected_parameters():
        if p in params:
            value = params.pop(p)
            self.used_parameters[p] = prepare_lookup_value(p, value)

`FieldListFilter.expected_parameters` simply returns a singleton list
containing `self.lookup_kwarg`, which is simply a copy of `field_path`.

`used_parameters` therefore has at most one entry with key set to
`field_path` and value from the parameters, since `prepare_lookup_value`
returns the value as-is unless the key ends with either `'__in'` or
`'__isnull'`.

`BitHandler` "represents an array of bits, each as a `Bit` object."
It is one of two classes (the other being `Bit`) that is accepted by the
specialized `exact` lookup.  Here, we would only like to filter out
records which has all of the bits in the given integer parameter set.
The lookup uses the value in the given `BitHandler` as a mask, in the
form `field & mask = mask`.

The keys are not used here, since they are simply used for named bit
enumeration and manipulation.  This is justified for the following
reasons:

1. The user may supply an integer with multiple bits set which `Bit`
cannot handle.
2. The class is unlikely to change to adopt a strict behavior to filter out anything not in `_keys`
3. This commit only aims to fix the bug and it would be out of the scope.
   Later version may make the `keys` parameter of `BitHandler optional
   or introduce some new wrapper object or a custom lookup for achieving
   this.

This is a bit more efficient implementation compared to the intended one
since instead of doing `field = field | mask`, it instead chooses the
straightforward approach of `field & mask = mask`.  SQL databases are
more likely to recognize the last pattern for optimization, although they
should in all way be equivalent.

Plus, empty keyword arguments for `filter` essentially means a no-op, so
in this case we just return `queryset` directly since they are meant to
be immutable anyway.

Fixes disqus#61, disqus#64, and disqus#89
  • Loading branch information
ortholeviator committed Apr 6, 2020
1 parent 785c7e7 commit 4588fd5
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions bitfield/admin.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import six

from django.db.models import F
from django.core.exceptions import ValidationError
from django.utils.translation import ugettext_lazy as _
from django.contrib.admin import FieldListFilter
from django.contrib.admin.options import IncorrectLookupParameters

from bitfield import Bit
from bitfield import Bit, BitHandler


class BitFieldListFilter(FieldListFilter):
Expand All @@ -23,9 +22,14 @@ def __init__(self, field, request, params, model, model_admin, field_path):
field, request, params, model, model_admin, field_path)

def queryset(self, request, queryset):
filter = dict((p, F(p).bitor(v)) for p, v in six.iteritems(self.used_parameters))
filter_kwargs = dict(
(p, BitHandler(v, ()))
for p, v in six.iteritems(self.used_parameters)
)
if not filter_kwargs:
return queryset
try:
return queryset.filter(**filter)
return queryset.filter(**filter_kwargs)
except ValidationError as e:
raise IncorrectLookupParameters(e)

Expand Down

0 comments on commit 4588fd5

Please sign in to comment.