-
Notifications
You must be signed in to change notification settings - Fork 44
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
#!/usr/bin/env python | ||
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() |
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',), | ||
), | ||
] |
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) |
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')), | ||
], | ||
), | ||
] |
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 | ||
|
||
|
@@ -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) | ||
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll be honest with you: 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. | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,6 +3,8 @@ | |||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
import pytest | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
from second_testapp.models import OrderReview | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||
import yaml | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
@@ -28,6 +30,8 @@ | |||||||||||||||||||||||||||
UniqueIdentifier, | ||||||||||||||||||||||||||||
Child2, | ||||||||||||||||||||||||||||
Employee, | ||||||||||||||||||||||||||||
Product, | ||||||||||||||||||||||||||||
Order, | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'm still trying to get my head around what the issue is - is this an accurate description of what's going on? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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