Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not pass with_col_aliases=True to CTE compiler #81

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

millerdev
Copy link
Contributor

@millerdev millerdev commented Nov 8, 2023

Django 4.2 began passing this argument, which broke CTE queries because columns could not be referenced via <cte>.col.<column_name> syntax after the compiler added aliases. This change makes all django-cte tests pass on Django 4.2.

As demonstrated in test_heterogeneous_filter_in_cte, it is possible to create CTEs with window functions that produce mal-formed SQL due to compiler-added aliases. The same is true with UNION queries that use ORDER BY and select_related. Making those queries work probably requires a change to Django ORM internals.

Workaround for #66.

Django 4.2 began passing this argument, which broke CTE queries because columns could not be referenced via `<cte>.col.<column_name>` syntax after the compiler added aliases. This change makes all django-cte tests pass on Django 4.2, but it is unclear what other effects it may have.
- Add new Django versions, remove old
- Remove Python 3.7, which is EOL
Fix regression in Django 4.2 without undoing the fix for UNION with ORDER BY and select_related.
...in CTE with window function that triggers Django's auto-aliasing.
Copy link
Contributor

@gherceg gherceg left a comment

Choose a reason for hiding this comment

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

This change makes all django-cte tests pass on Django 4.2, but it is unclear what other effects it may have.

Could you update the description of this PR to explain the rationale behind these changes? I think you have a better understanding of what effects this change may have now than you did when this PR was first created.

Should there be a note about this in any documentation as well? Or do you plan to do that in a separate PR?

@millerdev
Copy link
Contributor Author

Description updated.

Should there be a note about this in any documentation as well?

Yes, I plan to make a note about this in the change log when it gets released.

@millerdev millerdev merged commit ec0637d into master Nov 20, 2023
15 checks passed
@millerdev millerdev deleted the dm/dj42-workaround branch November 20, 2023 21:16
@jrief
Copy link

jrief commented Nov 21, 2023

Just migrated my project from Django-4.1.13 to Django-4.2.7 and tested with the current main branch.

I can confirm that CTE-queries now work as expected.

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.

3 participants