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.

Sign up for GitHub

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

Jump to bottom

Set pk_field=UUIDField for UUID foreign keys #8791

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Set pk_field=UUIDField for UUID foreign keys #8791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
601ef12
Add test case for PrimaryKeyRelatedField with UUIDs
r-thomson Dec 5, 2022
1393573
Set pk_field from target_field in ModelSerializer
r-thomson Dec 11, 2022
5c7440d
Add test case for overriding default pk_field
r-thomson Dec 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions rest_framework/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,6 +1296,10 @@ def build_relational_field(self, field_name, relation_info):
field_kwargs['slug_field'] = to_field
field_class = self.serializer_related_to_field

# `pk_field` is only valid for PrimaryKeyRelatedField
if not issubclass(field_class, PrimaryKeyRelatedField):
field_kwargs.pop('pk_field', None)
Copy link
Member

Choose a reason for hiding this comment

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

The test suite lands here only for field_class==HyperlinkedRelatedField

It seems this change is neither covered by the new tests nor is it required as the new tests do pass without this addition.


# `view_name` is only valid for hyperlinked relationships.
if not issubclass(field_class, HyperlinkedRelatedField):
field_kwargs.pop('view_name', None)
Expand Down
2 changes: 2 additions & 0 deletions rest_framework/utils/field_mapping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ def get_relation_kwargs(field_name, relation_info):
kwargs.pop('queryset', None)
if model_field.null:
kwargs['allow_null'] = True
if isinstance(model_field.target_field, models.UUIDField):
Copy link
Member

Choose a reason for hiding this comment

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

I find it a bit awkward to introduce special handling for one field type while everything else is basically ignored. Sure, PKs are for the most part either AutoField or UUIDField, but that that is not a sure thing. For arguments sake this might very well be a DateTimeField.

Having more specific types seems reasonable, but then it should be done consistently and across the board imho.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, I was only thinking of integer, string, and UUID keys at the time but I can definitely see the value in supporting more fields.

Is there any reason we wouldn’t want to use ModelSerializer.serializer_field_mapping here?

Copy link
Member

Choose a reason for hiding this comment

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

This is just something that I noticed and thought it would be worth mentioning. Someone more knowledgeable on these parts should probably chime in on that question. @auvipy?

Copy link
Member

Choose a reason for hiding this comment

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

my thought here is, we should initially consider prospective primary key types / fields mapping here initiallly

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 most commonly used primary key types checking / special could be done initially

Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason we wouldn’t want to use ModelSerializer.serializer_field_mapping here?

We can use / try it here as well

Copy link
Author

Choose a reason for hiding this comment

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

Looked into this a bit— since some of the field types also need kwargs, we'd probably want to use build_field() instead of looking at the field mapping directly.

We could just make build_field() and friends classmethods so you could call ModelSerializer.build_field(...), but it'd also be nice if we passed the instance of ModelSerializer into get_relation_kwargs() so we could respect those methods being overriden on subclasses. At that point, though, perhaps it would make more sense for get_relation_kwargs to just be a method of ModelSerializer.

kwargs['pk_field'] = models.UUIDField()
if kwargs.get('read_only', False):
# If this field is read-only, then return early.
# No further keyword arguments are valid.
Expand Down
35 changes: 34 additions & 1 deletion tests/test_model_serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
from rest_framework import serializers
from rest_framework.compat import postgres_fields

from .models import NestedForeignKeySource
from .models import NestedForeignKeySource, UUIDForeignKeyTarget


def dedent(blocktext):
Expand Down Expand Up @@ -736,6 +736,39 @@ class Meta:
self.assertEqual(repr(TestSerializer()), expected)


class UUIDForeignKeyModel(models.Model):
foreign_key = models.ForeignKey(UUIDForeignKeyTarget, related_name='reverse_foreign_key', on_delete=models.CASCADE)


class TestUUIDForeignKeyMapping(TestCase):
def test_uuid_pk_relation(self):
class TestSerializer(serializers.ModelSerializer):
class Meta:
model = UUIDForeignKeyModel
fields = '__all__'

expected = dedent("""
TestSerializer():
id = IntegerField(label='ID', read_only=True)
foreign_key = PrimaryKeyRelatedField(pk_field=<django.db.models.fields.UUIDField>, queryset=UUIDForeignKeyTarget.objects.all())
""")
self.assertEqual(repr(TestSerializer()), expected)

def test_uuid_pk_relation_override(self):
class TestSerializer(serializers.ModelSerializer):
class Meta:
model = UUIDForeignKeyModel
fields = '__all__'
extra_kwargs = {'foreign_key': {'pk_field': serializers.UUIDField(format='int')}}

expected = dedent("""
TestSerializer():
id = IntegerField(label='ID', read_only=True)
foreign_key = PrimaryKeyRelatedField(pk_field=UUIDField(format='int'), queryset=UUIDForeignKeyTarget.objects.all())
""")
self.assertEqual(repr(TestSerializer()), expected)


class DisplayValueTargetModel(models.Model):
name = models.CharField(max_length=100)

Expand Down