#24744 closed Bug (fixed)
Missing relabeled_clone methods are causing fails in subqueries
Reported by: | mrAdm | Owned by: | Andriy Sokolovskiy |
---|---|---|---|
Component: | contrib.postgres | Version: | 1.8 |
Severity: | Release blocker | Keywords: | hstore |
Cc: | me@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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)
Attachments (1)
Change History (15)
comment:1 by , 10 years ago
Severity: | Normal → Release blocker |
---|---|
Summary: | hstore lookup inside subquery → hstore lookup fails inside subquery |
Triage Stage: | Unreviewed → Accepted |
by , 10 years ago
Attachment: | 24744-test.diff added |
---|
comment:2 by , 10 years ago
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.
comment:3 by , 10 years ago
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.
comment:4 by , 9 years ago
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.
comment:5 by , 9 years ago
Cc: | added |
---|
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.
comment:6 by , 9 years ago
Summary: | hstore lookup fails inside subquery → Missing relabeled_clone methods are causing fails in subqueries |
---|
comment:7 by , 9 years ago
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.
comment:8 by , 9 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:9 by , 9 years ago
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?
comment:11 by , 9 years ago
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.
comment:12 by , 9 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
For now, I think the best we can do is do some documentation for common problematic cases.
The patch looks good to me.
Reproduced with the attached test.