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

Fixes instance check in ListSerializer.to_representation (#8726) #8727

Merged
merged 13 commits into from
Nov 22, 2022
2 changes: 1 addition & 1 deletion rest_framework/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ def to_representation(self, data):
"""
# Dealing with nested relationships, data can be a Manager,
# so, first get a queryset from the Manager if needed
iterable = data.all() if isinstance(data, models.Manager) else data
iterable = data.all() if isinstance(data, models.manager.BaseManager) else data
Copy link
Member

Choose a reason for hiding this comment

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

I triggered the CI, can you please pull from main branch again? the bug was not verify able in the test suite, would you mind investigate further to figure out if we can add some relevant test for this?

Copy link
Contributor Author

@954-Ivory 954-Ivory Nov 21, 2022

Choose a reason for hiding this comment

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

I triggered the CI, can you please pull from main branch again? the bug was not verify able in the test suite, would you mind investigate further to figure out if we can add some relevant test for this?

Yes, but it may take a while.
I'm a little hungry. I'm going to cook dinner first. 😋


Is that what you mean? ( My English is not good )

  1. Append the test case.
  2. Merge this branch to master in my fork ( I'm not sure about this ).
  3. Then pull request again (encode:master from 954-ivory:master).

Copy link
Member

Choose a reason for hiding this comment

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

I meant pull from encode:master to your working branch for this PR 954-ivory:master so that the recently updated versions in CI could reflect here. And some unit tests :)

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the rebase, now just appending/adding additional test cases would be really great


return [
self.child.to_representation(item) for item in iterable
Expand Down
26 changes: 26 additions & 0 deletions tests/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

from django.contrib.auth.models import User
from django.db import models
from django.db.models import QuerySet
from django.db.models.manager import BaseManager
from django.utils.translation import gettext_lazy as _


Expand Down Expand Up @@ -124,3 +126,27 @@ class OneToOnePKSource(RESTFrameworkModel):
target = models.OneToOneField(
OneToOneTarget, primary_key=True,
related_name='required_source', on_delete=models.CASCADE)


class CustomManagerModel(RESTFrameworkModel):
class CustomManager:
def __new__(cls, *args, **kwargs):
cls = BaseManager.from_queryset(
QuerySet
)
return cls

objects = CustomManager()()
# `CustomManager()` will return a `BaseManager` class.
# We need to instantiation it, so we write `CustomManager()()` here.

text = models.CharField(
max_length=100,
verbose_name=_("Text comes here"),
help_text=_("Text description.")
)

o2o_target = models.ForeignKey(OneToOneTarget,
help_text='OneToOneTarget',
verbose_name='OneToOneTarget',
on_delete=models.CASCADE)
43 changes: 43 additions & 0 deletions tests/test_serializer_lists.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

from rest_framework import serializers
from rest_framework.exceptions import ErrorDetail
from tests.models import (
auvipy marked this conversation as resolved.
Show resolved Hide resolved
CustomManagerModel, NullableOneToOneSource, OneToOneTarget
)
auvipy marked this conversation as resolved.
Show resolved Hide resolved


class BasicObject:
Expand Down Expand Up @@ -683,3 +686,43 @@ def test_min_max_length_six_items(self):
assert min_serializer.validated_data == input_data

assert not max_min_serializer.is_valid()


@pytest.mark.django_db()
class TestToRepresentationManagerCheck:
"""
https://github.com/encode/django-rest-framework/issues/8726
"""

def setup_method(self):
class CustomManagerModelSerializer(serializers.ModelSerializer):
class Meta:
model = CustomManagerModel
fields = '__all__'

class OneToOneTargetSerializer(serializers.ModelSerializer):
my_model = CustomManagerModelSerializer(many=True, source="custommanagermodel_set")

class Meta:
model = OneToOneTarget
fields = '__all__'
depth = 3

class NullableOneToOneSourceSerializer(serializers.ModelSerializer):
target = OneToOneTargetSerializer()

class Meta:
model = NullableOneToOneSource
fields = '__all__'

self.serializer = NullableOneToOneSourceSerializer

def test(self):
o2o_target = OneToOneTarget.objects.create(name='OneToOneTarget')
NullableOneToOneSource.objects.create(
name='NullableOneToOneSource',
target=o2o_target
)
queryset = NullableOneToOneSource.objects.all()
serializer = self.serializer(queryset, many=True)
assert serializer.data