Opened 8 years ago

Closed 8 years ago

#25579 closed Bug (fixed)

Lack of type adaptation in ArrayField querying/lookups

Reported by: Matt C Owned by: Matt C
Component: contrib.postgres Version: 1.8
Severity: Normal Keywords: ArrayField query lookup
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Matt C)

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

Change History (13)

comment:1 Changed 8 years ago by Tim Graham

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.

comment:2 Changed 8 years ago by Matt C

#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.

comment:3 Changed 8 years ago by Matt C

Summary: ArrayField query/lookup regressionLack of type adaptation in ArrayField querying/lookups

comment:4 Changed 8 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

I can reproduce "can't adapt type 'Tag'" by adding list(OtherTypesArrayModel.objects.filter(tags=Tag(1))) to the PR from #25143.

comment:5 Changed 8 years ago by Matt C

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.

comment:6 Changed 8 years ago by Simon Charette

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.

comment:7 Changed 8 years ago by Matt C

Has patch: set
Owner: set to Matt C
Status: newassigned

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

comment:8 Changed 8 years ago by Matt C

Description: modified (diff)

comment:9 Changed 8 years ago by Tim Graham

Patch needs improvement: set

Left comments for improvement on the PR. Please uncheck "patch needs improvement" when you update it. Thanks!

comment:10 Changed 8 years ago by Matt C

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.

comment:11 Changed 8 years ago by Tim Graham

I think you may remove TagField.get_db_prep_value() then.

comment:12 Changed 8 years ago by Matt C

Patch needs improvement: unset

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.

comment:13 Changed 8 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In e7e5d9b3:

Fixed #25579 -- Fixed ArrayField.get_db_prep_value() to allow complex types.

Note: See TracTickets for help on using tickets.
Back to Top