Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

query: process RHS according to its type + tests #106

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

ortholeviator
Copy link
Contributor

Field.get_db_prep_lookup merely "prepares" the right hand side operand,
and returns a (sql, params) pair. For a plain value params will simply
be the value itself; compound expressions with as_sql method, however,
are never designed to work with get_db_prep_lookup and instead needs
their as_sql method called directly to retrieve the pair.

The error message has been changed from time to time across Django
versions. Related issues include:

  • "can't adapt type 'CombinedExpression'" in admin filter #89 "can't adapt type 'CombinedExpression'" in admin filter

    Since Django 1.10 removed Field.get_db_prep_lookup,
    Lookup.get_db_prep_lookup implementation is simply as follows.

    def get_db_prep_lookup(self, value, connection):
        return ('%s', [value])

    CombinedExpression is an expression type returned by combining
    exprssions with operators, such as __add__ or bitor.

    They are never meant to be passed to the SQL engine directly;
    instead, it has an as_sql method that returns what we want.

    process_rhs first applies any bilateral transforms on both sides,
    and if it finds either as_sql or _as_sql method on the RHS,
    calls it and wraps the SQL in the pair with parentheses; otherwise,
    it diverts to the usual get_db_prep_lookup as above.

  • 'CombinedExpression' object is not iterable when using an admin filter on Django 1.8 #64 'CombinedExpression' object is not iterable when using an admin filter on Django 1.8

    Same as above, except Lookup.get_db_prep_lookup is as follows.

    def get_db_prep_lookup(self, value, connection):
        return (
            '%s', self.lhs.output_field.get_db_prep_lookup(
                self.lookup_name, value, connection, prepared=True))

    Following is the relevant part of Field.get_db_prep_lookup.

    def get_db_prep_lookup(self, lookup_type, value, connection,
                           prepared=False):
        """
        Returns field's value prepared for database lookup.
        """
        if not prepared:
            value = self.get_prep_lookup(lookup_type, value)
            prepared = True
        if hasattr(value, 'get_compiler'):
            value = value.get_compiler(connection=connection)
        if hasattr(value, 'as_sql') or hasattr(value, '_as_sql'):
            # If the value has a relabeled_clone method it means the
            # value will be handled later on.
            if hasattr(value, 'relabeled_clone'):
                return value
            if hasattr(value, 'as_sql'):
                sql, params = value.as_sql()
            else:
                sql, params = value._as_sql(connection=connection)
            return QueryWrapper(('(%s)' % sql), params)
    
        if lookup_type in ('month', 'day', 'week_day', 'hour', 'minute',
                           'second', 'search', 'regex', 'iregex', 'contains',
                           'icontains', 'iexact', 'startswith', 'endswith',
                           'istartswith', 'iendswith'):
            return [value]

    Normally it is supposed to return the parameters for the SQL
    "prepared statement" as a list; however, if the value has either
    as_sql or _as_sql, it either returns the value directly if it
    also has relabeled_clone (which is the case for expressions) or
    wraps it in QueryWrapper. This is not a desired behavior, and
    using them would result in TypeError saying the object was not
    iterable since they were not lists in the first place.

  • 'SQLEvaluator' object is not iterable when using admin filter on Django 1.7 #61 'SQLEvaluator' object is not iterable when using admin filter on Django 1.7

    Same as above, except SQLEvaluator emerges from the SQL compiler
    wrapping expressions for internal use.

    • Update expressions django.db.models.sql.compiler.SQLUpdateCompiler.as_sql:

      ...
      if hasattr(val, 'evaluate'):
          val = SQLEvaluator(val, self.query, allow_joins=False)
      ...```
    • filter() resolution django.db.models.sql.query.Query.add_filter:

      ...
      elif isinstance(value, ExpressionNode):
          # If value is a query expression, evaluate it
          value = SQLEvaluator(value, self, reuse=can_reuse)
          having_clause = value.contains_aggregate
      ...```

This commit also causes "regression" for the following issue; its
legitimacy, however, is ambiguous considering that the lookup
essentially "hijacks" the original definition of 'exact'.

#26 admin BitFieldListFilter not working as expected

It should be noted, however, that the original behavior is still
possible to achieve by wrapping the int in a BitHandler. Fixing
this correctly would involve treating int as Bit as before while
showing a deprecation warning. Meanwhile filter(flags=Value(4)) will
find a record of which flags is exactly 4 across all versions.

`Field.get_db_prep_lookup` merely "prepares" the right hand side operand,
and returns a (sql, params) pair.  For a plain value `params` will simply
be the value itself; compound expressions with `as_sql` method, however,
are never designed to work with `get_db_prep_lookup` and instead needs
their `as_sql` method called directly to retrieve the pair.

The error message has been changed from time to time across Django
versions.  Related issues include:

 * disqus#89 "can't adapt type 'CombinedExpression'" in admin filter

   Since Django 1.10 removed `Field.get_db_prep_lookup`,
   `Lookup.get_db_prep_lookup` implementation is simply as follows.

   ```python
   def get_db_prep_lookup(self, value, connection):
       return ('%s', [value])
   ```

   `CombinedExpression` is an expression type returned by combining
   exprssions with operators, such as `__add__` or `bitor`.

   They are never meant to be passed to the SQL engine directly;
   instead, it has an `as_sql` method that returns what we want.

   `process_rhs` first applies any bilateral transforms on both sides,
   and if it finds either `as_sql` or `_as_sql` method on the RHS,
   calls it and wraps the SQL in the pair with parentheses; otherwise,
   it diverts to the usual `get_db_prep_lookup` as above.

 * disqus#64 'CombinedExpression' object is not iterable when using an admin filter on Django 1.8

   Same as above, except `Lookup.get_db_prep_lookup` is as follows.

   ```python
   def get_db_prep_lookup(self, value, connection):
       return (
           '%s', self.lhs.output_field.get_db_prep_lookup(
               self.lookup_name, value, connection, prepared=True))
   ```

   Following is the relevant part of `Field.get_db_prep_lookup`.

   ```python
   def get_db_prep_lookup(self, lookup_type, value, connection,
                          prepared=False):
       """
       Returns field's value prepared for database lookup.
       """
       if not prepared:
           value = self.get_prep_lookup(lookup_type, value)
           prepared = True
       if hasattr(value, 'get_compiler'):
           value = value.get_compiler(connection=connection)
       if hasattr(value, 'as_sql') or hasattr(value, '_as_sql'):
           # If the value has a relabeled_clone method it means the
           # value will be handled later on.
           if hasattr(value, 'relabeled_clone'):
               return value
           if hasattr(value, 'as_sql'):
               sql, params = value.as_sql()
           else:
               sql, params = value._as_sql(connection=connection)
           return QueryWrapper(('(%s)' % sql), params)

       if lookup_type in ('month', 'day', 'week_day', 'hour', 'minute',
                          'second', 'search', 'regex', 'iregex', 'contains',
                          'icontains', 'iexact', 'startswith', 'endswith',
                          'istartswith', 'iendswith'):
           return [value]
   ```

   Normally it is supposed to return the parameters for the SQL
   "prepared statement" as a `list`; however, if the value has either
   `as_sql` or `_as_sql`, it either returns the value directly if it
   also has `relabeled_clone` (which is the case for expressions) or
   wraps it in `QueryWrapper`.  This is not a desired behavior, and
   using them would result in `TypeError` saying the object was not
   iterable since they were not lists in the first place.

 * disqus#61 'SQLEvaluator' object is not iterable when using admin filter on Django 1.7

   Same as above, except `SQLEvaluator` emerges from the SQL compiler
   wrapping expressions for internal use.

    - Update expressions:
      django.db.models.sql.compiler.SQLUpdateCompiler.as_sql:

      ...
      if hasattr(val, 'evaluate'):
          val = SQLEvaluator(val, self.query, allow_joins=False)
      ...

    - filter() resolution:
      django.db.models.sql.query.Query.add_filter:

      ...
      elif isinstance(value, ExpressionNode):
          # If value is a query expression, evaluate it
          value = SQLEvaluator(value, self, reuse=can_reuse)
          having_clause = value.contains_aggregate
      ...

This commit also causes "regression" for the following issue; its
legitimacy, however, is ambiguous considering that the lookup
essentially "hijacks" the original definition of 'exact'.

 disqus#26 admin BitFieldListFilter not working as expected

It should be noted, however, that the original behavior is still
possible to achieve by wrapping the `int` in a `BitHandler`.  Fixing
this correctly would involve treating `int` as `Bit` as before while
showing a deprecation warning.  Meanwhile `filter(flags=Value(4))` will
find a record of which `flags` is exactly `4` across all versions.
@timabbott timabbott merged commit ef7130b into disqus:master Apr 6, 2020
@timabbott
Copy link
Collaborator

This seems like a step forward, merged, thanks @ortholeviator!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants