Opened 7 years ago

Closed 7 years ago

#7088 closed (fixed)

QuerySet.values_list returns extra fields in Oracle

Reported by: ikelly Owned by: mtredinnick
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 7 years ago by mtredinnick

(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 follow-up: Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 7 years ago by Alex

  • Version changed from queryset-refactor to SVN

comment:4 in reply to: ↑ 2 Changed 7 years ago by ikelly

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 7 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick

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 7 years ago by ikelly

  • Resolution set to fixed
  • Status changed from new to closed

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