Demonstrate bug where related_name is not working properly when using… #73

mcosti · 2023-06-19T15:34:46Z

 File "/Users/mcosti/dev/django-typed-models/env/bin/activate/lib/python3.9/site-packages/django/apps/registry.py", line 433, in do_pending_operations
    function(model)
  File "/Users/mcosti/dev/django-typed-models/env/bin/activate/lib/python3.9/site-packages/django/apps/registry.py", line 411, in apply_next_model
    self.lazy_model_operation(next_function, *more_models)
  File "/Users/mcosti/dev/django-typed-models/env/bin/activate/lib/python3.9/site-packages/django/apps/registry.py", line 397, in lazy_model_operation
    function()
  File "/Users/mcosti/dev/django-typed-models/env/bin/activate/lib/python3.9/site-packages/django/db/models/fields/related.py", line 373, in resolve_related_class
    field.do_related_class(related, model)
  File "/Users/mcosti/dev/django-typed-models/typedmodels/models.py", line 108, in do_related_class
    old_do_related_class(other, cls)
  File "/Users/mcosti/dev/django-typed-models/env/bin/activate/lib/python3.9/site-packages/django/db/models/fields/related.py", line 448, in do_related_class
    self.set_attributes_from_rel()
  File "/Users/mcosti/dev/django-typed-models/env/bin/activate/lib/python3.9/site-packages/django/db/models/fields/related.py", line 445, in set_attributes_from_rel
    self.remote_field.set_field_name()
  File "/Users/mcosti/dev/django-typed-models/env/bin/activate/lib/python3.9/site-packages/django/db/models/fields/reverse_related.py", line 306, in set_field_name
    self.field_name = self.field_name or self.model._meta.pk.name
AttributeError: 'str' object has no attribute '_meta'

… lazy model references

mcosti · 2023-06-19T15:41:16Z

The culprit lives somewhere here

                if isinstance(field, models.fields.related.RelatedField):
                    # Monkey patching field instance to make do_related_class use created class instead of base_class.
                    # Actually that class doesn't exist yet, so we just monkey patch base_class for a while,
                    # changing _meta.model_name, so accessor names are generated properly.
                    # We'll do more stuff when the class is created.
                    old_do_related_class = field.do_related_class

                    def do_related_class(self, other, cls):
                        base_class_name = base_class.__name__
                        cls._meta.model_name = classname.lower()
                        old_do_related_class(other, cls)
                        cls._meta.model_name = base_class_name.lower()

                    field.do_related_class = types.MethodType(do_related_class, field)
                if isinstance(field, models.fields.related.RelatedField):
                    remote_field = field.remote_field
                    if (
                        isinstance(remote_field.model, TypedModel)
                        and remote_field.model.base_class
                    ):
                        remote_field.limit_choices_to[
                            "type__in"
                        ] = remote_field.model._typedmodels_subtypes

second_testapp/models.py

mcosti · 2023-06-20T18:35:04Z

This is fixed now.
I added a print in RelatedFIeld.resolve_related_class

Without my changes:

<class 'testapp.models.UniqueIdentifier'> <class 'django.contrib.contenttypes.models.ContentType'> testapp.UniqueIdentifier.content_type
<class 'testapp.models.Animal'> <class 'testapp.models.UniqueIdentifier'> testapp.Animal.unique_identifiers
<class 'testapp.models.Canine'> <class 'testapp.models.UniqueIdentifier'> testapp.Canine.unique_identifiers
<class 'testapp.models.Feline'> <class 'testapp.models.UniqueIdentifier'> testapp.Feline.unique_identifiers
<class 'testapp.models.BigCat'> <class 'testapp.models.UniqueIdentifier'> testapp.BigCat.unique_identifiers
<class 'testapp.models.Animal'> <class 'testapp.models.Canine'> testapp.Animal.canines_eaten
<class 'testapp.models.Animal_canines_eaten'> <class 'testapp.models.Animal'> testapp.Animal_canines_eaten.animal
<class 'testapp.models.Animal_canines_eaten'> <class 'testapp.models.Canine'> testapp.Animal_canines_eaten.canine
<class 'testapp.models.AngryBigCat'> <class 'testapp.models.UniqueIdentifier'> testapp.AngryBigCat.unique_identifiers
<class 'testapp.models.Parrot'> <class 'testapp.models.UniqueIdentifier'> testapp.Parrot.unique_identifiers
<class 'testapp.models.Parent'> <class 'testapp.models.Parent'> testapp.Parent.b
<class 'second_testapp.models.BaseReview'> <class 'testapp.models.Product'> second_testapp.BaseReview.product

With the changes (respecting the same way Django deals with these things):

<class '__fake__.Animal_canines_eaten'> <class '__fake__.Animal'> testapp.Animal_canines_eaten.animal
<class '__fake__.Animal_canines_eaten'> <class '__fake__.Canine'> testapp.Animal_canines_eaten.canine
<class '__fake__.Animal'> <class '__fake__.Canine'> testapp.Animal.canines_eaten
<class '__fake__.Parent'> <class '__fake__.Parent'> testapp.Parent.b
<class '__fake__.BaseReview'> <class '__fake__.Product'> second_testapp.BaseReview.product
<class '__fake__.BaseReview'> <class '__fake__.Order'> second_testapp.BaseReview.order
<class '__fake__.Order'> <class '__fake__.Product'> testapp.Order.product

@craigds if you could please have a look that would be amazing. Thank you

craigds · 2023-06-20T23:31:57Z

manage.py

@@ -0,0 +1,22 @@
+#!/usr/bin/env python


this file doesn't seem to be required

Thought it would be easier for future people to create migrations :D But I can remove it

craigds · 2023-06-20T23:45:32Z

typedmodels/tests.py

+    """Regression test for the following scenario:
+    A subclass of a typed model has foreign key to two models in a different app, while they are also related
+    to each other.
+    """


Suggested change

"""Regression test for the following scenario:

A subclass of a typed model has foreign key to two models in a different app, while they are also related

to each other.

"""

"""

Regression test for the following scenario:

* A typed model has a ForeignKey or OneToOneField to a model in another app (lazily referenced via a string model name)

* The ForeignKey or OneToOneField has a `related_name` specified.

Previously, the related_name argument was not initialized correctly during startup, and threw an AttributeError.

In this test, the fixed behaviour is tested via the `Order.order_review` and `Product.reviews` attributes.

"""

I'm still trying to get my head around what the issue is - is this an accurate description of what's going on?

Is it important that the ForeignKey/OneToOneField is in another app, or would this happen with related models even in the same app if they used lazy/string references to each other?

The description is accurate and indeed, the models being in a different app is what triggers the error. I encountered it in my project and this was the minimum demonstration possible.

craigds · 2023-06-20T23:53:48Z

typedmodels/models.py

@@ -453,6 +443,24 @@ def _get_unique_checks(self, exclude=None, **kwargs):
        return unique_checks, date_checks


+def _get_related_class_function(base_class, classname, field):


I can't seem to figure out what's changed – other than a bit of rearrangement this looks like the same code as before. Can you explain how this fixes the bug?

I'll be honest with you:
I am not entirely sure why this works. Before the new monkey patched method was bound to the metaclass itself. Somehow this was affecting the model discovery, but to be fair, I came to this solution via a bit of trial and error.

It definitely makes a difference. If you want, you can undo my change and have a look at the error

mcosti · 2023-08-31T18:05:06Z

Hey @craigds . Any updates? This no longer impacts me as I've removed the depdency, but I think it's still a good QoL improvement

Demonstrate bug where related_name is not working properly when using…

d014d3d

… lazy model references

djbios reviewed Jun 19, 2023

View reviewed changes

second_testapp/models.py Show resolved Hide resolved

mcosti added 3 commits June 20, 2023 09:51

Simplify regression test

3b5d7d8

Add migrations

15b9860

Fix issue

498d882

Clean-up

5e25625

craigds reviewed Jun 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demonstrate bug where related_name is not working properly when using… #73

Demonstrate bug where related_name is not working properly when using… #73

mcosti commented Jun 19, 2023

mcosti commented Jun 19, 2023

mcosti commented Jun 20, 2023

craigds Jun 20, 2023

mcosti Jun 21, 2023

craigds Jun 20, 2023

craigds Jun 20, 2023

mcosti Jun 21, 2023

craigds Jun 20, 2023

mcosti Jun 21, 2023

mcosti commented Aug 31, 2023

		@@ -453,6 +443,24 @@ def _get_unique_checks(self, exclude=None, **kwargs):
		return unique_checks, date_checks


		def _get_related_class_function(base_class, classname, field):

Demonstrate bug where related_name is not working properly when using… #73

Are you sure you want to change the base?

Demonstrate bug where related_name is not working properly when using… #73

Conversation

mcosti commented Jun 19, 2023

mcosti commented Jun 19, 2023

mcosti commented Jun 20, 2023

craigds Jun 20, 2023

Choose a reason for hiding this comment

mcosti Jun 21, 2023

Choose a reason for hiding this comment

craigds Jun 20, 2023

Choose a reason for hiding this comment

craigds Jun 20, 2023

Choose a reason for hiding this comment

mcosti Jun 21, 2023

Choose a reason for hiding this comment

craigds Jun 20, 2023

Choose a reason for hiding this comment

mcosti Jun 21, 2023

Choose a reason for hiding this comment

mcosti commented Aug 31, 2023