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

Reverse relation is lost for abstract models with custom querysets #652

Closed
leamingrad opened this issue Jun 23, 2021 · 6 comments · Fixed by #730
Closed

Reverse relation is lost for abstract models with custom querysets #652

leamingrad opened this issue Jun 23, 2021 · 6 comments · Fixed by #730
Assignees
Labels
bug Something isn't working

Comments

@leamingrad
Copy link
Contributor

leamingrad commented Jun 23, 2021

What's wrong

The reverse relation to a concrete subclass of an abstract model that uses a custom queryset is not being populated by django-stubs.

A full set of test cases that illustrate the situation are in a commit here that I added on top of the latest master.

Since an example is worth a thousand words:

from django.db import models

class AbstractLogQuerySet(models.QuerySet):
    pass

class Transaction(models.Model):
    pass

class AbstractLog(models.Model):
    objects = AbstractLogQuerySet.as_manager()
    class Meta:
        abstract=True

class TransactionLog(AbstractLog):
    transaction = models.ForeignKey(Transaction, on_delete=models.CASCADE, related_name="logs")

Transaction().logs
# myapp/models:13: error: "Transaction" has no attribute "logs"

Interestingly, the problem seems to be specific to querysets being used to make managers:

  • An abstract model with the default manager works fine
  • An abstract model with a custom manager works fine
  • An abstract model with a custom manager and queryset fails as well

I'm happy to help with a fix, but to be honest I have no idea where to start.

System information

I've seen this on multiple Python/Django/mypy/django-stubs versions but the test cases are written against:

  • python version: 3.9.2
  • django version:3.2.4
  • mypy version: 0.910
  • django-stubs version: HEAD

(Apologies if this is a duplicate - I looked and there seem to be similar issues but not this exact one)

@leamingrad leamingrad added the bug Something isn't working label Jun 23, 2021
@sobolevn
Copy link
Member

sobolevn commented Jun 23, 2021

I'm happy to help with a fix, but to be honest I have no idea where to start.

Great to hear! I would be happy to help.

  1. Start with a failing test somewhere here: https://github.com/typeddjango/django-stubs/tree/master/tests/typecheck/db/models Your example would be a great starting point
  2. Then, look at the plugin itself, the execution path you are interested in is:

Ask any questions you have! 👍

@leamingrad
Copy link
Contributor Author

I've managed to make a bit of progress on this, but would appreciate any help as I am a bit stuck.

From what I can tell, the root of the problem is when the AddRelatedManagers transformer is dealing with the TransactionLog class in the example above, it does not have an objects property.

This should be being set by the AddManagers transformer, but it fails to do so because it is never able to find the type info for the manager class when the from_queryset method is involved (I have switched my examples to use that as it generally seems to be more supported than .from_manager).

I have managed to make some progress in the commit here by making sure that we always look for generated managers. This makes things work when we change the example above to:

from django.db import models

class AbstractLogQuerySet(models.QuerySet):
    pass

class Transaction(models.Model):
    pass

# Declare the queryset manager outside the model, and use from_queryset
AbstractLogQSManager = models.Manager.from_queryset(AbstractLogQuerySet)

class AbstractLog(models.Model):
    objects = AbstractLogQSManager()
    class Meta:
        abstract=True

class TransactionLog(AbstractLog):
    transaction = models.ForeignKey(Transaction, on_delete=models.CASCADE, related_name="logs")

Transaction().logs
# No output - so everything is fine

However, it still does not work when the manager declaration is inline in the AbstractLog class. When I move the declaration of objects inside TransactionLog, everything works as expected, but I guess that is because the AddManagers transformation does not need to do anything as the objects property is declared explicitly?

I guess my confusion boils down to: "is there a way to make sure that create_new_manager_class_from_from_queryset_method" is called when a manager is declared from a queryset within an abstract class rather than globally?

@sobolevn
Copy link
Member

sobolevn commented Aug 1, 2021

You try to defer() if objects's type are not yet analyzed. This might help.

@RJPercival
Copy link
Contributor

@leamingrad, all of your test cases now pass on master. Could you please re-test to confirm the issue you encountered is now fixed?

@leamingrad
Copy link
Contributor Author

Thanks for this - just reran my tests locally that were reproducing the issue and I can confirm that this is now fixed 🎉

@sobolevn
Copy link
Member

As someone living in Leningrad, I appreciate your username @leamingrad 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

3 participants