Fix fetching models from complex RawQuerySets in SQLite #2069

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
@AlexHill
Contributor

AlexHill commented Dec 13, 2013

SQLite will sometimes (returning columns from subqueries or views for example) return column names in the form table_alias."column_name" or simply "column_name" instead of just column_name.

This meant that RawQuerySet can't match up the columns when trying to create model instances, and raises InvalidQuery: Raw query must include the primary key.

This change introduces a column_name_converter() to the backend introspection code to mirror table_name_converter() and implements the fix on the SQLite backend.

This is the same behaviour accounted for in SQLAlchemy here: https://github.com/zzzeek/sqlalchemy/blob/4663ec98b226a7d495846f0d89c646110705bb30/lib/sqlalchemy/dialects/sqlite/base.py#L591

@apollo13

This comment has been minimized.

Show comment Hide comment
@apollo13

apollo13 Feb 13, 2014

Member

Where is the accepted ticket for this issue?

Member

apollo13 commented Feb 13, 2014

Where is the accepted ticket for this issue?

@AlexHill

This comment has been minimized.

Show comment Hide comment
@AlexHill

AlexHill Feb 14, 2014

Contributor

Hi Florian,

The ticket isn't accepted, but it's here, with some discussion: https://code.djangoproject.com/ticket/21603

Contributor

AlexHill commented Feb 14, 2014

Hi Florian,

The ticket isn't accepted, but it's here, with some discussion: https://code.djangoproject.com/ticket/21603

django/db/backends/__init__.py
+
+ The default column name converter is for case sensitive comparison.
+ """
+ return name

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Aug 4, 2014

Member

maybe it would be better to call table_name_converter() as a "reasonable default" (since most backends seem to require the same behavior)?

@timgraham

timgraham Aug 4, 2014

Member

maybe it would be better to call table_name_converter() as a "reasonable default" (since most backends seem to require the same behavior)?

This comment has been minimized.

Show comment Hide comment
@AlexHill

AlexHill Aug 5, 2014

Contributor

Sure, I'm fine with that – it would ensure compatibility with existing third-party backends that override table_name_converter.

@AlexHill

AlexHill Aug 5, 2014

Contributor

Sure, I'm fine with that – it would ensure compatibility with existing third-party backends that override table_name_converter.

+ """
+ # TODO: remove when SQLite < 3.7.15 is sufficiently old.
+ # 3.7.13 ships in Debian stable as of 2014-03-21.
+ return name.split('.')[-1].strip('"')

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Aug 4, 2014

Member

could we add a Database.sqlite_version_info < (3, 7, 15) conditional to skip the logic on newer versions?

@timgraham

timgraham Aug 4, 2014

Member

could we add a Database.sqlite_version_info < (3, 7, 15) conditional to skip the logic on newer versions?

This comment has been minimized.

Show comment Hide comment
@AlexHill

AlexHill Aug 5, 2014

Contributor

Sure 👍

@AlexHill

AlexHill Aug 5, 2014

Contributor

Sure 👍

@timgraham

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Aug 5, 2014

Member

buildbot, test this please.

Member

timgraham commented Aug 5, 2014

buildbot, test this please.

@timgraham

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Aug 6, 2014

Member

merged in 938da36, thanks.

Member

timgraham commented Aug 6, 2014

merged in 938da36, thanks.

@timgraham timgraham closed this Aug 6, 2014

@AlexHill AlexHill deleted the AlexHill:column_name_converter branch Mar 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment