Missing relabeled_clone methods are causing fails in subqueries

[mrAdm]

There are two models:

from django.db import models
from django.contrib.postgres.fields import HStoreField


class A(Model):
    test = models.CharField(max_length=100)
    hstore = HStoreField()


class B(Model):
    a = models.ForeignKey(A)

Execute the query:

print B.objects.filter(a__in=A.objects.filter(hstore__field=1))

I get the following exception:

Traceback (most recent call last):
  File "/vagrant/manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python2.7/dist-packages/django/core/management/__init__.py", line 338, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python2.7/dist-packages/django/core/management/__init__.py", line 312, in execute
    django.setup()
  File "/usr/local/lib/python2.7/dist-packages/django/__init__.py", line 18, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/usr/local/lib/python2.7/dist-packages/django/apps/registry.py", line 115, in populate
    app_config.ready()
  File "/vagrant/st/apps.py", line 18, in ready
    print str(B.objects.filter(a__in=A.objects.filter(hstore__field=1)).query)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/query.py", line 679, in filter
    return self._filter_or_exclude(False, *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/query.py", line 697, in _filter_or_exclude
    clone.query.add_q(Q(*args, **kwargs))
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.py", line 1301, in add_q
    clause, require_inner = self._add_q(where_part, self.used_aliases)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.py", line 1328, in _add_q
    current_negated=current_negated, connector=connector, allow_joins=allow_joins)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.py", line 1150, in build_filter
    value, lookups, used_joins = self.prepare_lookup_value(value, lookups, can_reuse, allow_joins)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.py", line 1007, in prepare_lookup_value
    value.query.bump_prefix(self)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.py", line 868, in bump_prefix
    self.change_aliases(change_map)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/query.py", line 792, in change_aliases
    self.where.relabel_aliases(change_map)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/sql/where.py", line 287, in relabel_aliases
    self.children[pos] = child.relabeled_clone(change_map)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/lookups.py", line 178, in relabeled_clone
    new.lhs = new.lhs.relabeled_clone(relabels)
  File "/usr/local/lib/python2.7/dist-packages/django/db/models/lookups.py", line 76, in relabeled_clone
    return self.__class__(self.lhs.relabeled_clone(relabels))
  File "/usr/local/lib/python2.7/dist-packages/django/contrib/postgres/fields/hstore.py", line 76, in __init__
    super(KeyTransform, self).__init__(*args, **kwargs)
TypeError: __init__() takes exactly 3 arguments (1 given)

[Tim Graham]

Reproduced with the attached test.

[Anssi Kääriäinen]

Seems like a case where KeyTransform will need a custom relabeled_clone method. We should check other new lookups, transforms and expressions to see if they contain this same bug.

This is quite common bug, I have seen a variation of missing or broken relabeled_clone method a dozen times. The aliases of a query are not relabeled for normal queries, so normal testing doesn't spot these issues. The author needs to manually write a test for subquery support, and this is too easy to forget.

Any ideas how we could automate testing of expression-likes so that we automatically check relabeled_clone() support? The optimal solution would be that whenever a new expression-like is introduced, we automatically test that for relabeled_clone(). But this needs some sort of magic autodetection, and I don't have any ideas how we could do this without ugly hacks.

The alternate solution is to always test subquery support manually.

[Josh Smeaton]

Any ideas how we could automate testing of expression-likes so that we automatically check relabeled_clone() support?

I've had similar thoughts about when we introduce a new field. What are all the tests that should be written? I've got two ideas. The first would be a test code generator. The better idea would be a test case builder. I'm not sure how feasible either would be though.

So far the "gold standard" for expression tests is in expressions_case. If we can generalise the tests there enough (or copy elsewhere then generalise), we might be able to create a list of expressions, and pass each one into the generalised test case to run through common failure scenarios. Not sure if this is practical though, considering the inputs and outputs to each expression can be wildly different.

[Marc Tamlyn]

I would love a better list of all the things we should have (and generally a much more accurate set of lookup/transform/expression to field mappings). A possible answer would be to have a TestCase mixin with many test_foo methods raising NotImplementedError. We may also want a metaclass though to be able to explicitly remove test_bar where the feature bar does not apply, rather than skipping it. This would have the advantage that when you add a test to the base mixin for a new generic lookup/expression/whatever then it gets tested on all field types, and you see the ones it fails on (and on which dbs).

AFAIK there are not relabeled_clone methods in contrib.postgres at present, so any Transform therein which takes args is potentially suspect.

[Andriy Sokolovskiy]

So, as a conclusion, the goal is:

• At first, to create testcase with fields->lookup/transform/expressions mapping
• Fix problematic fields

, right?

I don't fully understand an idea of "A possible answer would be to have a TestCase mixin with many test_foo methods raising NotImplementedError", if you are proposing to create basic tests with declared mapping.
Or this will be a basic testcase, which will be used in different tests modules?

After we will formalize plan, I want to take this ticket.

[Tim Graham]

I think solving the reported failure would be sufficient for this ticket. We could open a new one for the more general method.

My understanding of the mixin idea is to use mixin that would be used in a test subclass for each expressions to ensure the test writer implements all the basic ways an expression should be tested.

[Anssi Kääriäinen]

I think the base Transform class is actually broken (see ​https://github.com/django/django/blob/master/django/db/models/lookups.py#L58 __init__ and relabeled_clone()). So, we really need some safety net for this. Maybe we could test directly the relabeled_clone() method for expressions?

[Andriy Sokolovskiy]

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

[Andriy Sokolovskiy]

After several discussions we decided to not making base mixins for now, because it is not so simple formalize common things for all transfroms/functions/aggregates/etc, so I fixed the actual problem.

[Anssi Kääriäinen]

For now, I think the best we can do is do some documentation for common problematic cases.

The patch looks good to me.

[Tim Graham <timograham@…>]

In 08232ef:

Fixed #24744 - Fixed relabeled_clone for the Transform

[Tim Graham <timograham@…>]

In b4b1375:

[1.8.x] Fixed #24744 - Fixed relabeled_clone for the Transform

Backport of 08232ef84d4959826ad5136f183c9fc5bedf0599 from master