[Django] #25579: ArrayField query/lookup regression
Django
----------------------------------+-------------------------------------
Reporter: freshquiz | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Severity: Normal | Keywords: ArrayField query lookup
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+-------------------------------------
This commit:
[https://github.com/django/django/commit/39d95fb6ada99c59d47fa0eae6d3128abafe2d58]
replaces the `get_prep_value` method with `get_db_prep_value` and I
believe this causes a regression with regards to querying on an
ArrayField.
`get_db_prep_value` is never called during querying/lookup, hence the
array items/values are passed directly to the low-level database Python
wrappers without first being transformed (from Python objects to SQL-
friendly parameters).
See this question: [http://stackoverflow.com/questions/33250371
/arrayfield-class-missing-query-lookup-methods]
Could both the `get_prep_value` and `get_db_prep_value` methods be
overridden with duplicate/shared logic to overcome this issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/25579>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: freshquiz | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: ArrayField query | Triage Stage:
lookup | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Looks similar to #25143 but I think it's a separate issue. I'm not sure
about the characterization of the issue as a regression since `ArrayField`
was never present in a stable release without the commit you pointed out,
but it might be okay to backport anyway under the "bug in a new feature"
rationale.
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: ArrayField query | Triage Stage:
lookup | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by freshquiz):
#25143 is to do with converting DB query result values to Python values,
whilst this is to do with converting Python values to DB values, for the
purpose of lookups/querying.
I'll change the title to remove the word regression.
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: freshquiz | Owner:
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: ArrayField query | Triage Stage:
lookup | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
I can reproduce "can't adapt type 'Tag'" by adding
`list(OtherTypesArrayModel.objects.filter(tags=Tag(1)))` to the PR from
#25143.
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by freshquiz):
I've created a pull request for this, but I'm not sure if I need to change
any of the configuration (e.g. "Has patch" and whether I should take
ownership) for this ticket, or if that's done by the core devs.
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.postgres | Version: 1.8
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
Please assign this ticket to yourself to prevent parallel work, check
''Has Patch'' and preferably link to your PR.
The only thing you can't do is mark your own patch as ''Ready for
checkin''.
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: contrib.postgres | Version: 1.8
Severity: Normal | Resolution:
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by freshquiz):
* status: new => assigned
* owner: => freshquiz
* has_patch: 0 => 1
Comment:
Thanks @charettes.
I wasn't sure if linking to the pull request was necessary, because it
seemed to happen automatically in the details box at the top, but here is
the link anyway:
[https://github.com/django/django/pull/6278]
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: freshquiz | Owner: freshquiz
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by freshquiz:
Old description:
> This commit:
> [https://github.com/django/django/commit/39d95fb6ada99c59d47fa0eae6d3128abafe2d58]
> replaces the `get_prep_value` method with `get_db_prep_value` and I
> believe this causes a regression with regards to querying on an
> ArrayField.
>
> `get_db_prep_value` is never called during querying/lookup, hence the
> array items/values are passed directly to the low-level database Python
> wrappers without first being transformed (from Python objects to SQL-
> friendly parameters).
>
> See this question: [http://stackoverflow.com/questions/33250371
> /arrayfield-class-missing-query-lookup-methods]
>
> Could both the `get_prep_value` and `get_db_prep_value` methods be
> overridden with duplicate/shared logic to overcome this issue?
New description:
This commit:
[https://github.com/django/django/commit/39d95fb6ada99c59d47fa0eae6d3128abafe2d58]
replaces the `get_prep_value` method with `get_db_prep_value` and I
believe this causes a regression with regards to querying on an
ArrayField.
`get_db_prep_value` is never called during querying/lookup, hence the
array items/values are passed directly to the low-level database Python
wrappers without first being transformed (from Python objects to SQL-
friendly parameters).
See this question: [http://stackoverflow.com/questions/33250371
/arrayfield-class-missing-query-lookup-methods]
Could both the `get_prep_value` and `get_db_prep_value` methods be
overridden with duplicate/shared logic to overcome this issue?
Patch pull request: [https://github.com/django/django/pull/6278]
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: freshquiz | Owner: freshquiz
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left comments for improvement on the PR. Please uncheck "patch needs
improvement" when you update it. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: freshquiz | Owner: freshquiz
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by freshquiz):
With the introduction of this commit:
[https://github.com/django/django/commit/2495023a4cae28f494d0a6172abfac3a47a0b816]
...I realised that the solution may lie at a higher level.
The `tests.postgres_tests.models.TagField` class has overridden the
`get_db_prep_value()` method, with the only change being that it ignores
the `prepared` keyword-arg and always calls `get_prep_value()` on the
input value.
This was introduced because the author of that test code would have
encountered the type adaptation issue which is what this ticket is about.
So I can't use the `TagField` class to write tests for a solution to this
ticket, because the problem is solved by that class.
I.e. Take away the `TagField`'s overriding implementation of
`get_db_prep_value()` and the problem of this ticket arises.
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: freshquiz | Owner: freshquiz
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
I think you may remove `TagField.get_db_prep_value()` then.
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: freshquiz | Owner: freshquiz
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by freshquiz):
* needs_better_patch: 1 => 0
Comment:
Deleted `TagField.get_db_prep_value()`, but didn't add any new tests,
because one of the tests from
[https://github.com/django/django/commit/2495023a4cae28f494d0a6172abfac3a47a0b816]
failed after deleting that method (as I said it would).
I also took a different approach to solving it, re-using existing code as
much as possible.
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: freshquiz | Owner: freshquiz
Component: contrib.postgres | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: ArrayField query | Triage Stage: Accepted
lookup |
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:"e7e5d9b338cabaafc61b7a0c55ff395b533d8c9e" e7e5d9b3]:
{{{
#!CommitTicketReference repository=""
revision="e7e5d9b338cabaafc61b7a0c55ff395b533d8c9e"
Fixed #25579 -- Fixed ArrayField.get_db_prep_value() to allow complex
types.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25579#comment:13>