dimagi / django-cte

Common Table Expressions (CTE) for Django
Other
334 stars 46 forks source link

Fix dj42 issues 66 #78

Closed pgammans closed 7 months ago

pgammans commented 1 year ago

This add support for Django 4.2 to pass the current test.

In Django 4.2 the SQL of a CTE is build but the columns are automatically asigned an alias this cases the CTE references to fail. This commit attempts to choose beter column aliases so they match the CTE names. ie Unlike Django using col1, col2 for un-aliased columns we use the same as the field name.

Whist this passes the test it is a invasive set of changes, i did think i'd manages a ultra simple set of changes

The test added is to ensure we don't break the column renaming fix in django 4.2. As the simple removal with_col_aliases in https://github.com/dimagi/django-cte/issues/66#issuecomment-1659145386 does to make django_cte pass it test

Fixes: #66

adamanthil commented 1 year ago

Our team is waiting on this change to update to Django 4.2. Posting to watch. What is required to get this merged?

millerdev commented 1 year ago

As noted in the PR description, this is an invasive set of changes. I have not had time to review it in depth, but I can say that I plan to have a version of django-cte that supports Django 4.2 before 3.2 EOL (April 2024).

millerdev commented 1 year ago

Unlike Django using col1, col2 for un-aliased columns we use the same as the field name

How is the outcome of this PR different from the solution mentioned in https://github.com/dimagi/django-cte/issues/66#issuecomment-1659145386? That is, if CTE fields are aliased using the same name as the field name, how is that different from overriding and passing with_col_aliases=False to the CTE query compiler?

If they are functionally the same, then I think I would prefer to override and pass with_col_aliases=False since that is a less invasive solution, although I think it may be incomplete.

One thing I am unclear on is whether with_col_aliases=False causes column aliases set at the query level (outside of the compiler) to be stripped? If it does, then I can see how that would be problematic.

coltonpark5 commented 7 months ago

As noted in the PR description, this is an invasive set of changes. I have not had time to review it in depth, but I can say that I plan to have a version of django-cte that supports Django 4.2 before 3.2 EOL (April 2024).

@millerdev With 3.2 now EOL. Just curious to know if you have an idea of the timeline to support 4.2?

millerdev commented 7 months ago

@coltonpark5 Except for a few edge cases, #66 is fixed by https://github.com/dimagi/django-cte/pull/81, which was released in v1.3.2. I also reported a bug in Django, but it seems the dev team is not able to maintain compatibility with projects like django-cte that depend on ORM internals.

A better solution may be possible if proper ORDER BY $index support were implemented for a future version of Django, but in the mean time queries will need to be adjusted to make them work with Django 4.2.

I'm going to close this PR since https://github.com/dimagi/django-cte/pull/81 was the preferred fix.