Fix missing related managers on some models

[ljodal]

I was seeing an issue where some related managers were missing from some
models. Traced the issue down to this line, where it appears that if we
hit a relation with a non-default(?) reverse manager the iteration
stopped. I think this is supposed to be a continue statement instead.
It appears to work in the project I'm working in at least.

[sobolevn]

Thank you! Can you please create a unit test for this?

[ljodal]

I'll try, but I'm honestly not entirely sure how to reproduce it. It just happened on a few of our models, but I don't see anything special about them. Will give it a try though :)

[ljodal]

@sobolevn As far as I can tell this is related to the AddRelatedManagers model initializer being called multiple times, but I'm unable to reproduce the issue I'm seeing in our project (it's happening on two specific models with 10s of foreign key references and I'm not able to isolate why).

Basically it appears to me that the branch where it's returning is supposed to short-circuit the logic that ends up generating the type info and store it here:

https://github.com/typeddjango/django-stubs/blob/master/mypy_django_plugin/transformers/models.py#L391-L395

But as it's returning, not continuing the loop, it's not short-circuiting just that one field, but all remaining fields on the model.

I see that we hit the same branch for django.contrib.auth.models as well, but I'm not able to isolate any side-effects from that.

Any chance we could get this out without a test case? Alternatively any pointers to where I can start looking to get a reproducer?

[ljodal]

Finally managed to reproduce it. Don't ask my why it's failing or what's going on, but this test case fails without the fix and works with the fix.

[ljodal]

Without the fix these attributes will be missing, as it returns after adding the .test attributes

[sobolevn]

Thanks!

[flaeppe]

Finally managed to reproduce it. Don't ask my why it's failing or what's going on, but this test case fails without the fix and works with the fix.

I think I can explain somewhat. Reverse managers should be created on the fly (dynamically if you will) to avoid one having to annotate them, i.e. related_set: SomeManager[RelatedModel]. The bottom line idea is to reuse the default_manager type for a given model used in reverse, e.g. if there is a model with 2 FKs to different models (forget about custom managers used reversed for a moment) the reverse manager for those 2 related models should use the same manager type.

This is a long sentence but, essentially, with the return statement it'd "finish" establishing the reverse default managers once it encounters the first reverse relation with a default_manager type set. So having a continue there should be correct to also get a type on any other reverse relations.

So, I think, the "return bug" would appear if a model has > 1 reverse relations. e.g. Model1 has reverse relations test and test2.

I don't know if this "explanation" helps at all, just thought I might try to clarify.

[ljodal]

Thanks for the explanation, it makes it a bit clearer at least!

So, I think, the "return bug" would appear if a model has > 1 reverse relations. e.g. Model1 has reverse relations test and test2.

This isn't entirely true, as it was working on some of our models with > 1 reverse relation, but not all, so there's something else as well that has to be in place to trigger the bug. Anyway, should be fixed now, the hardest part here was reproducing it 😅

[flaeppe]

This isn't entirely true, as it was working on some of our models with > 1 reverse relation, ...

True, processing order of the reverse relations might be involved here too, i.e. in which order relations are iterated over (since it'll return on the first one having declared a default manager for reverse relations)

Anyways, nice catch and that you figured out a repro 👍

[christianbundy]

I'm bumping into this but I don't think it's released in 1.10.0 -- are there any plans to release 1.10.1 soon?

[rnegron]

Bumping as this is not available in release 1.10.1. Would be included in a future 1.10.2 release, any chance for that soon?

[aleksanb]

I still have most related managers missing after the latest upgrades to the type checker code.

The patch we used to have to apply to an older version of django-mypy was this one: HyreAS@2ec7384.

But i'm not quite sure as to how i would apply this to the current state of the code.

Some related managers do get set up, so it seems like django-mypy just gives up at a certain point and stops adding more related managers.

Our models are on the form

class MyModel(SomeMixin, SomeAbstractClassThatItselfInheritsFromModelsModel):
  mode_fields = ...

I'll try to create a repro, but haven't had any luck making a reduced test case actually fail on ci before.

[aleksanb]

This is a dump of what i get with some extra logging added:

no manager <ManyToOneRel: hyre__payment.salesorderrow> django.db.models.manager.RelatedManager TypeInfo(
  Name(django.db.models.manager.RelatedManager)
  Bases(django.db.models.manager.Manager[_T`1])
  Mro(django.db.models.manager.RelatedManager, django.db.models.manager.Manager, django.db.models.manager.BaseManager, builtins.object)
  Names(
    add
    clear
    related_val (builtins.tuple[builtins.int, ...])
    remove
    set))

When adding logging here:

Unsure where to go from here.