Opened 9 years ago

Closed 9 years ago

#25064 closed Bug (fixed)

Join.as_sql() emits invalid SQL if get_joining_columns() returns an empty tuple

Reported by: Alex Hill Owned by: nobody
Component: Database layer (models, ORM) Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Alex Hill)

Join.as_sql() gets its join conditions from two places: the get_joining_columns() and get_extra_restrictions().

The SQL for extra restrictions is unconditionally prefixed with AND, which means invalid SQL is emitted if no joining columns are specified. The fix to make the AND conditional is a one-liner and allows users to override the default join conditions instead of adding to them.

Change History (11)

comment:1 by Alex Hill, 9 years ago

Description: modified (diff)
Summary: Allow get_joining_columns to return empty tupleJoin.as_sql() emits invalid SQL if get_joining_columns() returns an empty tuple

comment:2 by Tim Graham, 9 years ago

Is a patch forthcoming? If not, details on how to reproduce the issue will be helpful.

comment:3 by Alex Hill, 9 years ago

Patch forthcoming – sorry, got pulled away before fleshing this out.

comment:4 by Alex Hill, 9 years ago

Has patch: set

Here's the patch and tests: https://github.com/django/django/pull/4978

The patch is against master, but I would love to get this fixed in 1.8 and would be happy to backport it.

comment:5 by Anssi Kääriäinen, 9 years ago

Triage Stage: UnreviewedAccepted

I don't think this should be backpatched as this isn't an omission in a new feature (the Join type is completely internal, so its addition doesn't count as a new feature).

Otherwise I don't see any harm in doing the changes.

comment:6 by Anssi Kääriäinen, 9 years ago

Patch needs improvement: set

Patch to Join seems OK to me, but the tests need some improvement.

comment:7 by Alex Hill, 9 years ago

Thanks. Fixed the tests in line with your feedback, rebased and squashed.

comment:8 by Alex Hill, 9 years ago

Patch needs improvement: unset

I've updated the example used in the tests which should hopefully clarify things a bit, and made the recommended style edits.

Version 0, edited 9 years ago by Alex Hill (next)

comment:9 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:10 by Tim Graham, 9 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Marked RFC too soon as there is a test failure on Oracle.

comment:11 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 98bcdfa8:

Fixed #25064 -- Allowed empty join columns.

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