Skip to content

Fix invalid INNER JOIN added on LEFT OUTER JOIN for django 5.2#130

Merged
millerdev merged 2 commits into
dimagi:mainfrom
us77ipis:main
Nov 20, 2025
Merged

Fix invalid INNER JOIN added on LEFT OUTER JOIN for django 5.2#130
millerdev merged 2 commits into
dimagi:mainfrom
us77ipis:main

Conversation

@us77ipis
Copy link
Copy Markdown
Contributor

The invalid INNER JOIN was generated for another relation that is used within the join condition, which incorrectly reduces the result set. With Django 5.2 the issue can be fixed by only updating the join types if the CTE join is an INNER JOIN.

The invalid INNER JOIN was generated for another relation that is used
within the join condition, which incorrectly reduces the result set.
Copy link
Copy Markdown
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this change behavior in a backward-incompatible way for CTE joins that used LOUTER?

@us77ipis
Copy link
Copy Markdown
Contributor Author

It could... LEFT OUTER JOINs such as the one from the new test case return more result rows after this fix. However, I would say that the new behavior is the correct one, since it aligns with the behavior of LEFT OUTER JOINs directly on the db: the left outer join always includes all rows from the left side in the join result set. With the fix, django-cte would align with this behavior even if the join condition requires implicit joining of an additional third relation table.

The previous query (as before the fix) can be achieved e.g. by additionally adding an __isnull=False filter() on the pk of the third table to the queryset, which would also implicitly convert that join of the third table to an INNER JOIN.

@millerdev millerdev merged commit fd3c009 into dimagi:main Nov 20, 2025
17 checks passed
@millerdev
Copy link
Copy Markdown
Contributor

Using _join_type=LOUTER is an experimental feature, and this only affects Django 5.2+. Not sure if we should bump the major version when releasing this?

@millerdev
Copy link
Copy Markdown
Contributor

@us77ipis didn't think of this until after merging this PR, sorry. Can you create a quick follow-up that adds a description of this change in a new unreleased section of the CHANGELOG?

us77ipis added a commit to us77ipis/django-cte that referenced this pull request Nov 25, 2025
@us77ipis
Copy link
Copy Markdown
Contributor Author

Using _join_type=LOUTER is an experimental feature, and this only affects Django 5.2+. Not sure if we should bump the major version when releasing this?

The issue also affects previous Django versions, but for Django 5.2 there is this fix. I'm also unsure about bumping the major version.

Can you create a quick follow-up that adds a description of this change in a new unreleased section of the CHANGELOG?

See PR #131.

millerdev added a commit that referenced this pull request Jan 2, 2026
Update CHANGELOG with breaking change from PR #130
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants