dimagi / django-cte

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

Django 3.0 AttributeError: 'QJoin' object has no attribute 'join_field' #17

Closed SebCorbin closed 4 years ago

SebCorbin commented 4 years ago

Happens with Django 3.0 and the test


======================================================================
ERROR: test_named_ctes (tests.test_cte.TestCTE)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/SebCorbin/django-cte/tests/test_cte.py", line 234, in test_named_ctes
    region_total=Sum("amount"),
  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/django/db/models/query.py", line 1078, in annotate
    clone.query.set_group_by()
  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/django/db/models/sql/query.py", line 1939, in set_group_by
    model = join.join_field.related_model
AttributeError: 'QJoin' object has no attribute 'join_field'
millerdev commented 4 years ago

Regression caused by Django 3.0.5. Probably related to https://code.djangoproject.com/ticket/31377

millerdev commented 4 years ago

@SebCorbin I would like to make a new release to get #19 out the door, but I'm holding off because I'm not very confident in this fix, which is needed to support Django 3.0.5.

The new reference to join.join_field in https://github.com/django/django/commit/10866a10fe9f0ad3ffdf6f93823aaf4716e6f27c (of https://github.com/django/django/pull/12589) is what caused the bug, but there does not appear to be a reliable way to calculate a join field for a QJoin object. For example, there could be multiple join fields (if on_clause references multiple columns), and I'm not even sure if the field returned by

https://github.com/dimagi/django-cte/blob/f3795a84b35e500082135537e2b8687b1c26cdd0/django_cte/join.py#L69

will always be a valid field object that can be used in all contexts where it may be referenced. For example, if it's a CTEColumn it will result in an error:

ValueError: cannot resolve 'cte.name' in recursive CTE setup. ...

As the author of this library I would like to see this issue resolved, but my organization (Dimagi) is still using an older version of Django, so it is not a high priority for us yet. I'm giving you full context in case you want to pursue a better solution to accelerate a new release of the library.

SebCorbin commented 4 years ago

What version are you using? As for myself I'm using it in two projects, one in 2.2 and the other in 3.0, but for the time being I can rely on master branch if needed.

Just to get the hang of this, QJoin() is your library's object, and is just there to handle aliases and conditions., right? Is there a way to get closer to the native way of handling joins?

On another note, I find this module very useful and I'd love to get it into core, I've looked at https://code.djangoproject.com/ticket/28919 but I'm not sure here to begin.

millerdev commented 4 years ago

Just to get the hang of this, QJoin() is your library's object, and is just there to handle aliases and conditions., right?

That's mostly correct. Its purpose is to facilitate joining a common table expression to a Django query with an arbitrary join condition.

Is there a way to get closer to the native way of handling joins?

Not that I know of, or at least I have not been able to imagine how to do it without compromising on the expressive power of CTEs. Django does not provide precise control over joins. Most if not all join logic is automatically derived from foreign key relationships defined on models. This is not suitable for CTEs because they are constructed as queries.

Ticket 28919 has helped to raise community awareness of and demonstrate interest in this library. I would love to see this library integrated into the core, but a possibly better outcome would be to have it recognized by the core in the form of a stable API (for things like query FROM clause elements with arbitrary joins) off of which extensions like django-cte can be built (without fear of dependence on internal implementation details that may change in non-backward-compatible ways).

I think one of best things that could be done for this project is to get the documentation to the level of Django ORM documentation. I'm not thinking of anything elaborate in terms of infrastructure. A docs folder with Github markdown files should be good enough.

nikosmichas commented 2 years ago

Hello @millerdev,

I came across this issue while I was trying to update a project to Django 4.0 As far as I understand, there is a hack in django-cte that is not considered stable enough in order to release a new version? Is there a testcase to reproduce the problematic behaviour? If there were some breaking tests, it would probably be easier to find a solution.

millerdev commented 2 years ago

@nikosmichas, I don't have a failing test case at this time, which indeed does make it more difficult to resolve this issue. Right now all the information I have to go off of is documented above on this issue or on things linked from it. I would probably start by taking another stab at writing QJoin in a less hacky way for Django 3.0 (maybe the docs or source has changed in a way that will make it easier since the last time I looked). If that fails, I might reach out to the Django ORM developers and ask for assistance there.

Any help you can provide would be much appreciated.