Add fallback related manager in final iteration of AddRelatedManagers #993

aleksanb · 2022-06-13T15:53:50Z

If a django model has a Manager class that cannot be resolved statically
(if it is generated in a way where we cannot import it, like objects = my_manager_factory()), we fallback to the default related manager, so
you at least get a base level of working type checking.

Closes #981, #969

sobolevn

Thanks!

sobolevn · 2022-06-13T16:00:23Z

mypy_django_plugin/transformers/models.py

                except helpers.IncompleteDefnException as exc:
                    if not self.api.final_iteration:
                        raise exc
                    else:
                        continue

+                default_manager = related_model_info.names.get("_default_manager")


Any chance we can test that? 🤔

I don't know if this change is correct though, i see there's some test diff as well.

And i've been unable to create a test-case that fails on main but passes here.

What should happen if there's no default manager? Do we need to run any of the code further down as well?

aleksanb · 2022-06-14T10:56:28Z

mypy_django_plugin/transformers/models.py

@@ -345,15 +345,19 @@ def run_with_model_cls(self, model_cls: Type[Model]) -> None:
                    related_manager_info = self.lookup_typeinfo_or_incomplete_defn_error(
                        fullnames.RELATED_MANAGER_CLASS
                    )  # noqa: E501
-                    default_manager = related_model_info.names.get("_default_manager")


I guess the reason we raise an exception here if it's not found is that the code is supposed to run multiple iterations over a project to actually finish?

But it never runs again.

aleksanb · 2022-06-14T11:22:41Z

https://gitter.im/mypy-django/Lobby?at=62a86e2ef8daa71e07cbe011. Perhaps @flaeppe can chime in, I see you've written some of this code.

flaeppe · 2022-06-14T12:01:54Z

https://gitter.im/mypy-django/Lobby?at=62a86e2ef8daa71e07cbe011. Perhaps @flaeppe can chime in, I see you've written some of this code.

Be aware of that the traceback you've linked points to a manager in a package that isn't exporting types (i.e. there's no type hints in djmoney)

I would suspect that's why there's some misbehaviour.

aleksanb · 2022-06-14T12:04:10Z

Be aware of that the traceback you've linked points to a manager in a package that isn't exporting types (i.e. there's no type hints in djmoney)

I would suspect that's why there's some misbehaviour.

yes this seems to be the case!
Do you know if there's something i can do to surpress this error?

aleksanb · 2022-06-14T12:06:43Z

The weird thing is that moneyed, the package, doesn't actually export any models that subclass django's models.Model.

flaeppe · 2022-06-14T12:12:12Z

django-money is a Django extension of pymoneyed. pymoneyed has no stuff related to Django. While pymoneyed is typed and django-money isn't.

I could have a quick peek on the code later on, if you want

aleksanb · 2022-06-14T12:26:11Z

I tried adding even more debug logging:

which gives

cls <class 'hyre.payment.models.models.SalesOrderRow'>
meta hyre__payment.salesorderrow
default_manager hyre__payment.SalesOrderRow.objects
dmclass <class 'djmoney.models.managers.money_manager.<locals>.MoneyManager'>
the default manager is <class 'djmoney.models.managers.money_manager.<locals>.MoneyManager'>
CHECK: adding default manager for <class 'hyre.payment.models.models.SalesOrderRow'> djmoney.models.managers.money_manager.<locals>.MoneyManager
lookin up typeinfo for  djmoney.models.managers.money_manager.<locals>.MoneyManager
fail No 'djmoney.models.managers.money_manager.<locals>.MoneyManager' found info is none

It thinks the default manager for some reason is MoneyManager.

aleksanb · 2022-06-14T12:33:58Z

I could have a quick peek on the code later on, if you want

That would be nice. I cannot share much of the code i'm working on, but i'm really not sure as to why this interaction with Money changes what the default_manager is.

aleksanb · 2022-06-14T12:36:08Z

There we have it!
When i remove all my MoneyField fields from my model,

class SalesOrderRow(ReprMixin, TimestampedModel):
    # base_amount = MoneyField(max_digits=10, decimal_places=2, default_currency='NOK')
    # vat_amount = MoneyField(max_digits=10, decimal_places=2, default_currency='NOK')
    # total_amount = MoneyField(max_digits=10, decimal_places=2, default_currency='NOK')
    ... # other fields down here

i get

cls <class 'hyre.payment.models.models.SalesOrderRow'>
meta hyre__payment.salesorderrow
default_manager hyre__payment.SalesOrderRow.objects
dmclass <class 'django.db.models.manager.Manager'>
the default manager is <class 'django.db.models.manager.Manager'>
CHECK: adding default manager for <class 'hyre.payment.models.models.SalesOrderRow'> django.db.models.manager.Manager
lookin up typeinfo for  django.db.models.manager.Manager
SUCCES django.db.models.manager.Manager[hyre.payment.models.models.SalesOrderRow]

And the project sets up relatedmanagers correctly.

flaeppe · 2022-06-14T12:43:34Z

Interesting, could it be some compatibility problem in django-money? I've used that package quite a lot, but haven't had any issues similar to yours.

I looked quickly through django-money and found these two things below, could that be related? Could your codebase perhaps be affected by it?

Also, are you relying on having django-money in your INSTALLED_APPS? Otherwise I'd suggest trying to remove it from there and see if that'd resolve your problem as well (if I'm reading stuff correctly that would remove triggering the "auto-patching" of default managers)

flaeppe · 2022-06-14T12:50:25Z

Nevermind, seems that apps.ready won't affect it. It's not possible to turn off when I looked a bit further.

https://github.com/django-money/django-money/blob/c6c80d438e0f6138de37fc108917996407d98dca/djmoney/models/fields.py#L331-L344

In any case, I don't think this is an issue for django-stubs? And adding a fallback to always add a default manager, which I think changed here, feels like it could introduce quite a bit of false positives(?)

Might be reasonable if it we changed this here to emit an error if the manager couldn't be resolved and then attach a default manager. That way it's possible to explicitly ignore any error but still get all the django defaults of a related manager.

flaeppe · 2022-06-14T12:56:19Z

mypy_django_plugin/transformers/models.py

+                if not default_manager:
+                    self.add_new_node_to_model_class(
+                        attname, Instance(related_manager_info, [Instance(related_model_info, [])])
+                    )


What if we move this piece inside the else block on line 351. Together with emitting an error? I think that feels most correct, as the model's manager can't be resolved, an error is displayed, and the default manager type attached still includes defaults from Django.

Emitting an error there feels quite helpful IMO, for debugging etc.

This seems like a fair approach.
Do we have any prior art on emitting errors/warnings for certain nodes?

ctx.api.fail it seems.

aleksanb · 2022-06-14T12:58:20Z

This seems to be it exactly (django-money creating unreferenceable default managers).

In any case, I don't think this is an issue for django-stubs? And adding a fallback to always add a default manager, which I think changed here, feels like it could introduce quite a bit of false positives(?)

Largest downside is that one cannot use django-money toghether with django-stubs then, even though it is django-stubs that is behaving unexpectedly by patching all default managers.

sobolevn · 2022-06-14T12:58:27Z

Might be reasonable if it we changed this here to emit an error if the manager couldn't be resolved and then attach a default manager. That way it's possible to explicitly ignore any error but still get all the django defaults of a related manager.

Seems reasonable. But, this will require a complete test suite! 👍

flaeppe · 2022-06-14T13:00:53Z

Might be reasonable if it we changed this here to emit an error if the manager couldn't be resolved and then attach a default manager. That way it's possible to explicitly ignore any error but still get all the django defaults of a related manager.

Seems reasonable. But, this will require a complete test suite! 👍

I think, in theory, that's quite simple to trigger? Just put a random name or whatever that can't be resolved as a model's manager?

Problematic part might be that since django's setup is run, it might validate that all managers are correct..

aleksanb · 2022-06-14T13:02:49Z

I think, in theory, that's quite simple to trigger? Just put a random name or whatever that can't be resolved as a model's manager?

I can try making a failing test where the manager is the return value of some function perhaps, so the symbol doesn't exist in scope?

flaeppe · 2022-06-14T13:03:39Z

This seems to be it exactly (django-money creating unreferenceable default managers).

In any case, I don't think this is an issue for django-stubs? And adding a fallback to always add a default manager, which I think changed here, feels like it could introduce quite a bit of false positives(?)

Largest downside is that one cannot use django-money toghether with django-stubs then, even though it is django-stubs that is behaving unexpectedly by patching all default managers.

It still kind of makes sense though. Since django-money isn't typed and django-stubs requires types. But I agree it's unfortunate that it does so.

flaeppe · 2022-06-14T13:08:44Z

I think, in theory, that's quite simple to trigger? Just put a random name or whatever that can't be resolved as a model's manager?

I can try making a failing test where the manager is the return value of some function perhaps, so the symbol doesn't exist in scope?

That sounds like an approach that could work 👍 Might also work to monkey-patch the model class via that function (e.g. after model declaration), then we're very close to imitate what django-money does.

aleksanb · 2022-06-14T13:42:28Z

hyre/damage/models.py:805: error: Couldn't resolve related manager, fallbacking to a stock django related manager.  [django-manager-missing]

sample error.

aleksanb · 2022-06-14T13:58:31Z

All right! Updated with testcase (that catches the error!) and comments and descriptions, and separate error code so that i can surpress django-manager-missing on my side.

sobolevn

Thank you! Great work!

sobolevn · 2022-06-14T17:26:54Z

tests/typecheck/fields/test_related.yml

+            content: |
+                from django.db import models
+                class TimestampedModel(models.Model):
+                    id = models.UUIDField(primary_key=True, editable=False)


Let's simplify this case. Do we need id to be explicit? Or can we remove it?

sobolevn · 2022-06-14T17:27:09Z

tests/typecheck/fields/test_related.yml

+                class TimestampedModel(models.Model):
+                    id = models.UUIDField(primary_key=True, editable=False)
+                    created_at = models.DateTimeField(auto_now_add=True, db_index=True)
+                    updated_at = models.DateTimeField(auto_now=True, db_index=True)


Do we need this field?

Also, do we need boolean args here?

sobolevn · 2022-06-14T17:27:50Z

tests/typecheck/fields/test_related.yml

+                class User(TimestampedModel):
+                    name = models.TextField()
+
+                def DynamicManager():


Right now DynamicManager is not typed checked at all. Do we need -> None?

flaeppe

Nice work 👍

I added a few thoughts.

flaeppe · 2022-06-14T18:15:02Z

mypy_django_plugin/transformers/models.py

+                                attname, Instance(related_manager_info, [Instance(related_model_info, [])])
+                            )
+                            self.ctx.api.fail(
+                                f"Couldn't resolve related manager for relation {relation.name}, constructing a default one instead.",


Perhaps it's a bit over the top to mention that a default is used?

Suggested change

f"Couldn't resolve related manager for relation {relation.name}, constructing a default one instead.",

f"Couldn't resolve related manager for relation {relation.name}.",

flaeppe · 2022-06-14T18:21:31Z

mypy_django_plugin/transformers/models.py

+                            # If a django model has a Manager class that cannot
+                            # be resolved on runtime (like django-money
+                            # monkeypatches all default managers after models
+                            # already have been declared, and generates this
+                            # manager as the return value of a function, so it
+                            # cannot be imported when we try to set up the
+                            # default related managers in
+                            # AddDefaultManagerAttribute), we fallback to a
+                            # default related manager, so you at least get a
+                            # base level of working type checking.


IMO, involving django-money in this comment is a bit too much.

Suggested change

# If a django model has a Manager class that cannot

# be resolved on runtime (like django-money

# monkeypatches all default managers after models

# already have been declared, and generates this

# manager as the return value of a function, so it

# cannot be imported when we try to set up the

# default related managers in

# AddDefaultManagerAttribute), we fallback to a

# default related manager, so you at least get a

# base level of working type checking.

# If a django model has a Manager class that cannot

# be resolved, we fallback to the default related manager,

# so you at least get a base level of working type checking.

I mean, if django-money changes behaviour this comment becomes obsolete.

flaeppe · 2022-06-14T18:29:07Z

tests/typecheck/fields/test_related.yml

+        myapp/models:9: error: Couldn't resolve related manager for relation booking, constructing a default one instead.
+        myapp/models:9: error: Couldn't resolve related manager for relation bookingowner_set, constructing a default one instead.


Thought: I'm wondering if it could be valuable to try to include the path/fullname to the relation declaration as well?

It's good that the name of the reverse relation is there. But maybe it's also nice if e.g. myapp.models.Booking.x was in the message too?

e.g.

Suggested change

myapp/models:9: error: Couldn't resolve related manager for relation booking, constructing a default one instead.

myapp/models:9: error: Couldn't resolve related manager for relation bookingowner_set, constructing a default one instead.

myapp/models:9: error: Couldn't resolve related manager for relation booking, constructing a default one instead. (myapp.models.Booking.renter)

myapp/models:9: error: Couldn't resolve related manager for relation bookingowner_set, constructing a default one instead. (myapp.models.Booking.owner)

I think we have that context/data available when emitting, right? Otherwise just skip this.

flaeppe · 2022-06-14T18:30:17Z

mypy_django_plugin/transformers/models.py

+                                attname, Instance(related_manager_info, [Instance(related_model_info, [])])
+                            )
+                            self.ctx.api.fail(
+                                f"Couldn't resolve related manager for relation {relation.name}, constructing a default one instead.",


Suggested change

f"Couldn't resolve related manager for relation {relation.name}, constructing a default one instead.",

f"Couldn't resolve related manager for relation {relation.name!r}, constructing a default one instead.",

Should wrap the name inside of: ''. e.g. 'bookingowner_set'

Not ', but "

flaeppe · 2022-06-14T18:42:56Z

I also think merging this PR would close #981 as that similarly uses a package (django-modeltranslations) that isn't exporting types.

aleksanb · 2022-06-15T11:21:17Z

Updated with better comments, warnings, simpler test case etc. @sobolevn.

If a django model has a Manager class that cannot be resolved statically (if it is generated in a way where we cannot import it, like `objects = my_manager_factory()`), we fallback to the default related manager, so you at least get a base level of working type checking.

sobolevn

LGTM, great work! 👍

sobolevn · 2022-06-15T15:46:46Z

@flaeppe do you have any feedback? :)

flaeppe

LGTM too. Nice work 👍

aleksanb · 2022-06-15T17:22:30Z

Wohoo!

What's the release schedule nowadays?
is it easy to cut a new release for pypi?

aleksanb · 2022-06-16T10:14:09Z

Hm, it seems like you cannot surpress mypy errors from extensions through --disable-error-code django-manager-missing and friends:

➜  app git:(mypy-wip-again-june) ✗ make mypy
usage: mypy [-h] [-v] [-V] [more options; see below]
            [-m MODULE] [-p PACKAGE] [-c PROGRAM_TEXT] [files ...]
mypy: error: Invalid error code(s): django-manager-missing

aleksanb · 2022-06-16T10:39:04Z

python/mypy#12987

mannyanebi · 2022-08-31T07:46:41Z

Following this to see how this gets resolved. Having this mypy: error: Invalid error code(s): django-manager-missing can't seem to suppress it

aleksanb changed the title ~~Add fix for related managers where~~ Add fix for related manager Jun 13, 2022

sobolevn reviewed Jun 13, 2022

View reviewed changes

aleksanb commented Jun 14, 2022

View reviewed changes

flaeppe reviewed Jun 14, 2022

View reviewed changes

aleksanb force-pushed the related-manager-fix branch 2 times, most recently from 00443f6 to eb3fae5 Compare June 14, 2022 13:41

aleksanb force-pushed the related-manager-fix branch from eb3fae5 to ce397ef Compare June 14, 2022 13:57

aleksanb mentioned this pull request Jun 14, 2022

Create regression test for related managers 2022 #967

Closed

sobolevn reviewed Jun 14, 2022

View reviewed changes

flaeppe reviewed Jun 14, 2022

View reviewed changes

aleksanb force-pushed the related-manager-fix branch from ce397ef to d742c31 Compare June 15, 2022 11:15

aleksanb changed the title ~~Add fix for related manager~~ Add fallback related manager in final iteration of AddRelatedManagers Jun 15, 2022

aleksanb force-pushed the related-manager-fix branch from d742c31 to cb4ff0c Compare June 15, 2022 11:22

aleksanb force-pushed the related-manager-fix branch from cb4ff0c to dab26d0 Compare June 15, 2022 11:22

sobolevn approved these changes Jun 15, 2022

View reviewed changes

flaeppe approved these changes Jun 15, 2022

View reviewed changes

sobolevn merged commit 9044a35 into typeddjango:master Jun 15, 2022

aleksanb deleted the related-manager-fix branch June 15, 2022 17:21

aleksanb mentioned this pull request Jun 16, 2022

ErrorCodes defined in mypy plugins seemingly cannot be ignored by --disable-error-code python/mypy#12987

Open

flaeppe mentioned this pull request Jun 16, 2022

Emit error and set fallback type for managers that can't be resolved #999

Merged

flaeppe mentioned this pull request Jun 27, 2022

Could not resolve manager type for "core.models.FooModel.objects" [django-manager-missing] #1023

Open

flaeppe mentioned this pull request Sep 23, 2023

Some related managers not getting set up #969

Closed

	f"Couldn't resolve related manager for relation {relation.name}, constructing a default one instead.",
	f"Couldn't resolve related manager for relation {relation.name}.",

		myapp/models:9: error: Couldn't resolve related manager for relation booking, constructing a default one instead.
		myapp/models:9: error: Couldn't resolve related manager for relation bookingowner_set, constructing a default one instead.

Add fallback related manager in final iteration of AddRelatedManagers #993

Add fallback related manager in final iteration of AddRelatedManagers #993

Conversation

aleksanb commented Jun 13, 2022 • edited

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksanb commented Jun 14, 2022

flaeppe commented Jun 14, 2022

aleksanb commented Jun 14, 2022

aleksanb commented Jun 14, 2022

flaeppe commented Jun 14, 2022

aleksanb commented Jun 14, 2022

aleksanb commented Jun 14, 2022

aleksanb commented Jun 14, 2022 • edited

flaeppe commented Jun 14, 2022

flaeppe commented Jun 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksanb commented Jun 14, 2022

sobolevn commented Jun 14, 2022

flaeppe commented Jun 14, 2022

aleksanb commented Jun 14, 2022

flaeppe commented Jun 14, 2022

flaeppe commented Jun 14, 2022

aleksanb commented Jun 14, 2022 • edited

aleksanb commented Jun 14, 2022

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaeppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaeppe commented Jun 14, 2022

aleksanb commented Jun 15, 2022

sobolevn left a comment

Choose a reason for hiding this comment

sobolevn commented Jun 15, 2022

flaeppe left a comment

Choose a reason for hiding this comment

aleksanb commented Jun 15, 2022

aleksanb commented Jun 16, 2022

aleksanb commented Jun 16, 2022

mannyanebi commented Aug 31, 2022

aleksanb commented Jun 13, 2022 •

edited

aleksanb commented Jun 14, 2022 •

edited

aleksanb commented Jun 14, 2022 •

edited