Opened 8 years ago

Closed 8 years ago

#7088 closed (fixed)

QuerySet.values_list returns extra fields in Oracle

Reported by: Ian Kelly Owned by: Malcolm Tredinnick
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

values_list queries are failing in Oracle with results like this one:

Failed example:
    Article.objects.values_list('headline')
Expected:
    [(u'Article 5',), (u'Article 6',), (u'Article 4',), (u'Article 2',), (u'Article 3',), (u'Article 7',), (u'Article 1',)]
Got:
    [[u'Article 5', u'', None], [u'Article 6', u'', None], [u'Article 4', u'', None], [u'Article 2', u'', None], [u'Article 3', u'', None], [u'Article 7', u'', None], [u'Article 1', u'', None]]

It seems that Query.results_iter is getting the list of fields to pass to resolve_columns from self.model._meta.fields. For basic queries, this is fine, but it's wrong for values and values_list queries, and it's incomplete for select_related queries (this last appears to be a bug in trunk as well).

To fix this, I think that the Query object needs to maintain a list of field objects inside or alongside the Query.select and Query.related_select_cols lists.

Change History (6)

comment:1 Changed 8 years ago by Malcolm Tredinnick

(In [7469]) queryset-refactor: Make sure the right list of fields is passed to the
Query.resolve_columns() method for those backends that provide it (e.g. Oracle).

Refs #7088

comment:2 Changed 8 years ago by Malcolm Tredinnick

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Something to think about: After this change and [7470], I'm becoming tempted to change the map() call in resolve_columns() to a zip() so that any other errors don't just fail silently. As far as I can work out, we should be passing in enough fields for everything but the extra select columns at the front.

comment:3 Changed 8 years ago by Alex Gaynor

Version: queryset-refactorSVN

comment:4 in reply to:  2 Changed 8 years ago by Ian Kelly

Replying to mtredinnick:

Something to think about: After this change and [7470], I'm becoming tempted to change the map() call in resolve_columns() to a zip() so that any other errors don't just fail silently. As far as I can work out, we should be passing in enough fields for everything but the extra select columns at the front.

It's not clear to me that if the results were truncated due to missing fields, that would necessarily cause an error. In fact, between [7438] and [7443], the results were getting truncated, and it was just producing bogus results rather than erroring.

I'm ambivalent as to whether the map gets replaced with zip (although note that it will eventually need to be changed in some way, since I hear that map(None) is going away in Python 3), but if we want some error-checking here, I think it should just be in the form of an explicit assertion on the relative lengths of the two sequences.

comment:5 Changed 8 years ago by Malcolm Tredinnick

Owner: changed from nobody to Malcolm Tredinnick

Ian: I had left this open until you had a chance to confirm the fix actually fixed stuff (ignore the refactoring comment, because that was a throwaway thought for now). Based on the fact that you're filing yet more bugs for me, does this mean we've fixed this issue and can close the ticket?

comment:6 Changed 8 years ago by Ian Kelly

Resolution: fixed
Status: newclosed

Yes, this particular issue appears to be resolved. I'll go ahead and close the ticket.

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