-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Set pk_field=UUIDField for UUID foreign keys #8791
base: master
Are you sure you want to change the base?
Conversation
aae9080
to
1393573
Compare
have to think about any breaking changes btw. |
I would expect the obvious breaking change to be that anyone who was expecting a If this was recently introduced, my gut would call this is a bugfix that isn't subject to any deprecations. Given that (I assume) it has been in the wild for years, that makes it hard to claim as a bugfix instead of expected behaviour. |
This is definitely a possibility; I discovered the behavior myself when I was writing a test case like |
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.
On a larger scale, one could pose the question whether this is worth the trouble. I'm pretty sure people will come out of the woodwork, saying that the change broke them because they relied on this seasoned implementation detail. Given that UUID("str") == UUID(UUID("str"))
, I guess it will likely go by unnoticed for most people, but you never know.
Clarification: My statement relates to modified serializer methods. As previously stated, the change is transparent to the actual API due to the renderer doing the string conversion anyway.
@@ -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) |
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.
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.
@@ -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): |
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.
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.
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.
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?
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 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?
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.
my thought here is, we should initially consider prospective primary key types / fields mapping here initiallly
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.
I meant most commonly used primary key types checking / special could be done initially
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.
Is there any reason we wouldn’t want to use
ModelSerializer.serializer_field_mapping
here?
We can use / try it here as well
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.
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
.
>>> UUID(UUID('051c9030-0fb2-4f1d-a36d-5d69e3c9794d'))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File ".../python3.10/uuid.py", line 174, in __init__
hex = hex.replace('urn:', '').replace('uuid:', '')
AttributeError: 'UUID' object has no attribute 'replace' |
my bad @r-thomson, I could have sworn I used that before. Anyways, that would be one more con point then. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@@ -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): |
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.
my thought here is, we should initially consider prospective primary key types / fields mapping here initiallly
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@@ -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): |
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.
I meant most commonly used primary key types checking / special could be done initially
@@ -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): |
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.
Is there any reason we wouldn’t want to use
ModelSerializer.serializer_field_mapping
here?
We can use / try it here as well
Can you please explain this with some example codes? |
As this PR is right now, the class ExampleSerializer(serializers.ModelSerializer):
class Meta:
model = PrimaryModel
fields = ('related',)
"""
ExampleSerializer():
related = PrimaryKeyRelatedField(pk_field=<django.db.models.fields.UUIDField>, queryset=RelatedModel.objects.all())
""" The alternative would be to have class ExampleSerializer(Serializer):
related = PrimaryKeyRelatedField(queryset=RelatedModel.objects.all()) |
should we accept this and explicitly document it? |
i think we should experiment more with this for now to reach a concrete consensus |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Updates
ModelSerializer
’s field generation to setpk_field=UUIDField()
if the related field isPrimaryKeyRelatedField
and the foreign key field on the model is aUUIDField
.See this discussion for an explanation of why I think this change should be made
Compatibility
This could potentially break existing code if that code was relying on getting a
UUID
instance inserializer.data
. This doesn’t apply once the data has been passed to the renderer, since AFAIK all built-in renderers will stringifyUUID
s.Alternatives
A potential alternative to this implementation would be for the behavior to be moved into
PrimaryKeyRelatedField
itself. In that case, thepk_field
setting would apply even when used outside ofModelSerializer
(unless overridden).