[Django] #24744: hstore lookup inside subquery
Django
----------------------------------+--------------------
Reporter: mrAdm | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Severity: Normal | Keywords: hstore
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24744>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
----------------------------------+------------------------------------
Reporter: mrAdm | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* severity: Normal => Release blocker
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
Reproduced with the attached test.
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:1>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* Attachment "24744-test.diff" added.
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:2>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jarshwah):
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:3>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:4>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* cc: me@… (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:5>
Django
----------------------------------+------------------------------------
Reporter: mrAdm | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:6>
Django
----------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:7>
Django
----------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* owner: => coldmind
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:8>
Django
----------------------------------+------------------------------------
Reporter: mrAdm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: hstore | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:9>
Django
----------------------------------+------------------------------------
Reporter: mrAdm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: hstore | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/4766
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:10>
Django
----------------------------------+------------------------------------
Reporter: mrAdm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: hstore | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:11>
Django
Reporter: mrAdm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: hstore | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by akaariai):
* stage: Accepted => Ready for checkin
Comment:
For now, I think the best we can do is do some documentation for common
problematic cases.
The patch looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: mrAdm | Owner: coldmind
Component: contrib.postgres | Version: 1.8
Severity: Release blocker | Resolution: fixed
Keywords: hstore | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"08232ef84d4959826ad5136f183c9fc5bedf0599" 08232ef]:
{{{
#!CommitTicketReference repository=""
revision="08232ef84d4959826ad5136f183c9fc5bedf0599"
Fixed #24744 - Fixed relabeled_clone for the Transform
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: mrAdm | Owner: coldmind
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: hstore | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"b4b13759f82e9e09951bb72875b1c6e384dca6a9" b4b1375]:
{{{
#!CommitTicketReference repository=""
revision="b4b13759f82e9e09951bb72875b1c6e384dca6a9"
[1.8.x] Fixed #24744 - Fixed relabeled_clone for the Transform
Backport of 08232ef84d4959826ad5136f183c9fc5bedf0599 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24744#comment:14>