ModelSerializer: OneToOne reverse relationships don't save in create()

[mindblight]

Checklist

• I have verified that that issue exists against the master branch of Django REST framework.
• I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• This is not a usage question. (Those should be directed to the discussion group instead.)
• This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• I have reduced the issue to the simplest possible case.
• I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Take two models:

class A(models.Model):
  name = models.CharField(...)

class B(models.Model):
  a = models.OneToOneField(A, related_name='b')

Their serializers:

class ASerializer(serializers.ModelSerializer):
  class Meta:
    model = A
    fields = ('name', 'b')

class BSerializer(serializers.ModelSerializer):
  a = models.OneToOneField(A, related_name='a')
  class Meta:
    model = B
    fields = ('a',)

And their viewsets:

class AViewSet(ModelViewSet):
  # endpoints at '/api/a'
  queryset = A.objects.all()
  serializer = ASerializer

class BViewSet(ModelViewSet):
  # endpoints at '/api/b'
  queryset = B.objects.all()
  serializer = BSerializer

FYI, I'm using DRF JSON API, so my JSON syntax might be a little off, but the idea is the same. Assume an A object exists with id 1, post a B object like this:

{
   "a": 1
}

Expected behavior

A[id=1] is updated to point to the new B. This works with a ManyToMany thanks to saving them after the fact: https://github.com/encode/django-rest-framework/blob/master/rest_framework/serializers.py#L960

Actual behavior

You get a response like:

{
  "id": 2,
  "a": 1,
}

Which says that A[id=1] points to B[id=2]. But, if you load B[id=2] afterwards, you'll get:

{
  "d": 2,
  "a": null,
}

Thoughts

It looks like the fix is to save any reverse relation instead of just ManyToManys. I'm also not sure if there's a similar bug for updates

[rpkilby]

Hi @mindblight. If you have the time, formalizing this as a failing test case would be greatly appreciated.

[xordoquy]

Django's model's fields are not supposed to work within serializers.
Please use Django REST framework relational fields instead.

[tomchristie]

Seconding @xordoquy - this line looks like a bug in your code...

a = models.OneToOneField(A, related_name='b')

[mindblight]

@tomchristie, good catch. I fixed the example code. This is a simplified version of what I'm doing, but I traced the problem through the debugger to that point.

@xordoquy I'm using a relational field in my actual code. It's DRF Json API's ResourceRelatedField, which is basically a PrimaryKeyRelatedField with extra validation. Are you saying that the PrimaryKeyRelatedField should know whether to save the reverse relation? I can't find where the code would do that.

It also seems like the behaviour is incorrect regardless of whether a serializer should save the reverse relation. When I POST to the endpoint, I get a 201 response with b.a == 1. If I query for b again, it will have b.a == null. It seems like it should be consistently null or consistently 1

[mindblight]

FYI, I'm using this code to solve the problem right now:

class BaseSerializer(serializers.ModelSerializer):
  def create(self, validated_data):
    instance = super().create(validated_data)

    ModelClass = self.Meta.model

    info = model_meta.get_field_info(ModelClass)
    for field_name, relation_info in info.relations.items():
      if relation_info.reverse and (field_name in validated_data):
        getattr(instance,field_name).save()
    
    return instance

It's basically the same thing ModelSerializer is already doing for ManyToMany relationships. Not sure if this is the right approach for DRF's architecture

[rpkilby]

Hi @mindblight. I'd recommend reducing the test case to it simplest form. In this case, by removing DRF JSON API from the mix. Double check to ensure that this isn't actual any issue with the extension.

[xordoquy]

I’m saying the code in the initial report shouldn’t work.
To consider reopening the issue, a test case in its simpliest form should be provided as @rpkilby just said

[feus4177]

I had to tweak @mindblight's solution slightly by adding not relation.to_many to the condition. Also, it affects both create and update.

[maerteijn]

Hi @xordoquy and/or @rpkilby

I encountered a similar issue with reverse relations that are not saved with a OneToOne field while the serializer data gives you the impression it did.

I added a testcase to demonstrate this: https://github.com/encode/django-rest-framework/compare/master...maerteijn:5996-reproduce-reverse-one-to-one-not-updated?expand=1

Run it with:

./runtests.py SlugNullableOneToOneTests

It will fail on the test_one_to_one_reverse_correctly_updated test.

Would you consider reopening this issue as I guess this is not expected or desired behaviour. Updating a reverse OneToOne relation like this would be a really poweful feature of DRF.

[maerteijn]

(And if it would help in the consideration: I would be happy to help with implementation of a PR to create a fix and/or update the documentation when it should not be supported)

[xordoquy]

@maerteijn your code looks like a valid issue. Mind opening it as a PR so we can see if that's depend on Django versions and how it fails ?

[xordoquy]

oh, forget my previous comment.
As it happens, reversed relations aren't automatically added. Therefore, they are subject to an explicit addition into the fields list.

[maerteijn]

I can access the field (with fields = '__all__') on the serializer instance, and the data is also serialized so I'm not sure here if explicitly defining the fields is a solution here.

But to be certain I just added all fields explicitly to the serializer fields but the issue remains.

If I understand you correctly, addding the reverse relations explicitly to the fields property should fix this and should update the reverse relation? As the test I added still fails, shall I open a PR with this test as you suggested in your first comment?