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

Detect underlying field for OneToOne primary key #8371

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schferbe
Copy link

Description

When using a OneToOneField as a primary key it does not get serialized in the same way as the "parent" model. E.g.

        class MyModelA(models.Model):
            id = models.DecimalField(max_digits=4, decimal_places=2, primary_key=True)

        class MyModelB(models.Model):
            id = models.OneToOneField(MyModelA, models.CASCADE, primary_key=True)

        class MyModelASerializer(serializers.ModelSerializer):
            class Meta:
                model = MyModelA
                fields = "__all__"

        class MyModelBSerializer(serializers.ModelSerializer):
            class Meta:
                model = MyModelB
                fields = "__all__"

The following serializations would result

MyModelASerializer().to_representation(MyModelA(id=12.34))
# {"id": "12.24"}
MyModelBSerializer().to_representation(MyModelB(id=MyModelA(id=12.34)))
# {"id": 12.24}

This PR sets the pk_field attribute of the PrimaryKeyRelatedField to ensure serialization is consistent with the "parent" model. This is also in line with the OpenApi schema that would be generated.

@@ -1234,6 +1234,10 @@ def build_standard_field(self, field_name, model_field):
if model_field.one_to_one and model_field.primary_key:
field_class = self.serializer_related_field
field_kwargs['queryset'] = model_field.related_model.objects
pk_field = field_mapping[model_field.foreign_related_fields[0]]
pk_field_kwargs = get_field_kwargs(field_name, model_field.foreign_related_fields[0])
Copy link
Author

Choose a reason for hiding this comment

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

Should the field_name be extracted from the foreign_related_fields[0] here?

@schferbe schferbe force-pushed the detect-underlying-field-for-one-to-one-primary-key branch from b1d737e to 1129a8d Compare February 24, 2022 14:15
@stale
Copy link

stale bot commented Apr 27, 2022

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.

@stale stale bot added the stale label Apr 27, 2022
@schferbe
Copy link
Author

bump

@stale stale bot removed the stale label Apr 28, 2022
@stale
Copy link

stale bot commented Jul 13, 2022

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.

@stale stale bot added the stale label Jul 13, 2022
@schferbe
Copy link
Author

schferbe commented Aug 1, 2022

bump

@stale stale bot removed the stale label Aug 1, 2022
@stale
Copy link

stale bot commented Oct 1, 2022

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.

@stale stale bot added the stale label Oct 1, 2022
@tomchristie
Copy link
Member

Okay - I think this makes sense.(?)

Is there an example that's more obvious than a DecimalField pk?
Feels pretty confusing using a Decimal as a primary key.

@stale stale bot removed the stale label Oct 4, 2022

def test_one_to_one_field(self):
class MyModelA(models.Model):
id = models.DecimalField(max_digits=4, decimal_places=2, primary_key=True)
Copy link
Member

Choose a reason for hiding this comment

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

decimal field as primary key? what are some actual use case of that?

@auvipy
Copy link
Member

auvipy commented Dec 8, 2022

instead o using decimal field as primary key, can you try something else like UUID field as primary key here in test case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants