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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions manage.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#!/usr/bin/env python
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file doesn't seem to be required

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import os
import sys


def main():
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "test_settings")

try:
from django.core.management import execute_from_command_line
except ImportError as exc:
raise ImportError(
"Couldn't import Django. Are you sure it's installed and "
"available on your PYTHONPATH environment variable? Did you "
"forget to activate a virtual environment?"
) from exc

execute_from_command_line(sys.argv)


if __name__ == "__main__":
main()
Empty file added second_testapp/__init__.py
Empty file.
40 changes: 40 additions & 0 deletions second_testapp/migrations/0001_initial.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Generated by Django 4.2.2 on 2023-06-20 07:54

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

initial = True

dependencies = [
('testapp', '0003_product_order'),
]

operations = [
migrations.CreateModel(
name='BaseReview',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('type', models.CharField(choices=[('second_testapp.orderreview', 'order review')], db_index=True, max_length=255)),
('rating', models.IntegerField()),
('order', models.OneToOneField(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='order_review', to='testapp.order')),
('product', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='reviews', to='testapp.product')),
],
options={
'abstract': False,
},
),
migrations.CreateModel(
name='OrderReview',
fields=[
],
options={
'proxy': True,
'indexes': [],
'constraints': [],
},
bases=('second_testapp.basereview',),
),
]
Empty file.
12 changes: 12 additions & 0 deletions second_testapp/models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from django.db import models

from typedmodels.models import TypedModel

mcosti marked this conversation as resolved.
Show resolved Hide resolved

class BaseReview(TypedModel):
rating = models.IntegerField()


class OrderReview(BaseReview):
product = models.ForeignKey("testapp.Product", on_delete=models.CASCADE, related_name="reviews", null=True)
order = models.OneToOneField("testapp.Order", on_delete=models.CASCADE, related_name="order_review", null=True)
1 change: 1 addition & 0 deletions test_settings.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
INSTALLED_APPS = (
"typedmodels",
"django.contrib.contenttypes",
"second_testapp", # purposefully before testapp to test related models with lazy loading
"testapp",
)
MIDDLEWARE_CLASSES = ()
Expand Down
29 changes: 29 additions & 0 deletions testapp/migrations/0003_product_order.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Generated by Django 4.2.2 on 2023-06-20 07:53

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
('testapp', '0002_developer_employee_manager'),
]

operations = [
migrations.CreateModel(
name='Product',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('name', models.CharField(default='Product name', max_length=255)),
],
),
migrations.CreateModel(
name='Order',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('external_id', models.CharField(default='123', max_length=255)),
('product', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='orders', to='testapp.product')),
],
),
]
9 changes: 9 additions & 0 deletions testapp/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,12 @@ class Developer(Employee):
class Manager(Employee):
# Adds the _exact_ same field as Developer. Shouldn't error.
name = models.CharField(max_length=255, null=True)


class Product(models.Model):
name = models.CharField(max_length=255, default="Product name")


class Order(models.Model):
external_id = models.CharField(max_length=255, default="123")
product = models.ForeignKey(Product, on_delete=models.CASCADE, related_name="orders")
34 changes: 21 additions & 13 deletions typedmodels/models.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import inspect
from functools import partial
import types

from django.core.exceptions import FieldDoesNotExist, FieldError
from django.core.serializers.python import Serializer as _PythonSerializer
from django.core.serializers.xml_serializer import Serializer as _XmlSerializer
from django.db import models
from django.db.models.base import ModelBase, DEFERRED
from django.db.models.fields import Field
from django.db.models.fields.related import RelatedField
from django.db.models.options import make_immutable_fields_list
from django.utils.encoding import smart_str

Expand Down Expand Up @@ -95,21 +94,12 @@ class Meta:
)
)

if isinstance(field, models.fields.related.RelatedField):
if isinstance(field, 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):
field.do_related_class = _get_related_class_function(base_class, classname, field)
remote_field = field.remote_field
if (
isinstance(remote_field.model, TypedModel)
Expand Down Expand Up @@ -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):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
Returns a function that can be used to replace a RelatedField's
do_related_class method. This function is used to monkey patch
RelatedFields on subclasses of TypedModel to use the created class
instead of the base class.
"""
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()

return partial(do_related_class, field)


# Monkey patching Python and XML serializers in Django to use model name from base class.
# This should be preferably done by changing __unicode__ method for ._meta attribute in each model,
# but it doesn’t work.
Expand Down
25 changes: 25 additions & 0 deletions typedmodels/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

import pytest

from second_testapp.models import OrderReview

try:
import yaml

Expand All @@ -28,6 +30,8 @@
UniqueIdentifier,
Child2,
Employee,
Product,
Order,
)


Expand Down Expand Up @@ -444,3 +448,24 @@ class Tester2(Employee):

class Tester3(Employee):
name = models.IntegerField(null=True)


def test_related_name_is_preserved_for_foreign_keys(db):
"""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.
"""
Comment on lines +454 to +457
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

product = Product.objects.create(name="test")
order = Order.objects.create(product=product)

order_review = OrderReview.objects.create(
order=order,
product=product,
rating=5,
)
assert order_review.order == order
assert order_review.product == product

assert order.order_review == order_review
assert product.reviews.first() == order_review