Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Using depth option on serializers should not ignore foreign keys on post/create actions #7483

Closed
5 of 6 tasks
DrJfrost opened this issue Aug 17, 2020 · 7 comments
Closed
5 of 6 tasks

Comments

@DrJfrost
Copy link

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

depth option makes you specify the fields for creating or updating

Expected behavior

depth option should only work on list create options, however it should not ignore those fields when sending Create Update actions

Actual behavior

when depth option is defined the serializer immediately makes the user define primarykeyrelatedfields for saving/updating stuff otherwise it will ignore them, althought there are workarounds like creating two separate serializer classes (one for safe methods LIST RETRIEVE, and the other one for CREATE UPDATE DELETE), this forces the user to define extra code adding too much verbosity/complexity in it if the project is big enough.

Another workaround is to define those fields as primarykeyfields but on the RETRIEVE, LIST methods, they do appear as primarykey too

@xordoquy
Copy link
Collaborator

xordoquy commented Aug 17, 2020

My thoughts about that:

  1. depth documentation only says it is for representation hence reading. Does not say anything for writing. Documentation could be more explicitly on that point though. As such, I consider this issue invalid.
  2. I would object to any field that provides a different representation for read vs write. If you want that, have two fields, one <field> for read-only nested representations, another <field>_pk or <field>_id as read/write or write only with the primary key.
  3. Changing the default behaviour to include default writes would silently break a lot of existing API that already relies on the fact that depth provides read-only representation.

@DrJfrost
Copy link
Author

alright the workaround that you provided is a nice approach however there is no documentation about it, I agree there should be more explanation about this feature on the documentation, and maybe some example using that work around to be able to provide both functionalities (read/write operations)?

@kristingreen
Copy link

I would like to suggest that altering the depth option is not the correct solution.

However, it would seem to be the natural expectation for a Many-To-One relationship that we would be able to send the primary key of the foreign object when saving but receive a nested object when reading. This is a very popular request and a work-around solution has been suggested in the following thread...

https://stackoverflow.com/questions/29950956/drf-simple-foreign-key-assignment-with-nested-serializers

The two-field method was suggested but was decided to be less elegant. You can decide that for yourself.

@jerinpetergeorge
Copy link

The two-field method was suggested but was decided to be less elegant. You can decide that for yourself.

Personally, I wouldn't prefer the two field solution, but this solution

Btw, I have already created a PR #7415, if it gets merged, we will have a built-in field that takes care of these things

@aasutossh
Copy link

aasutossh commented Sep 19, 2020

I am facing the similar scenario, I added depth=2 in serializer, reading from it is great since, I get all the related information at once. But, I want to be able to update it too, which I can't do. My issue detailed: beda-software/drf-writable-nested#119. I thought this was drf-writable-nested issue.
BTW, if I comment out the depth line, then I can post data just fine.

Edit: Also, what is the proper way to handling such scenarios, I want to get related information too, but if I don't use the depth then I would have to fetch them one by one and use them.
Using depth is better, or fetching related data one by one is better?
If, depth is better, then I can't POST as of now.
If, fetching them all one by one is better than I would have to make multiple requests, which is probably not good imo.

@DrJfrost
Copy link
Author

The cleanest solution to me was using 2 serializer classes, one for reading info and other for "unsafe" actions. on the view I override the get_serializer_class() method and assign it according to the action given on the view just like this:

def get_serializer_class(self):
        if not self.request.action in SAFE_METHODS:
            return LoanSerializer
        return LoanInfoSerializer

@kristingreen
Copy link

kristingreen commented Sep 22, 2020

The cleanest solution to me was using 2 serializer classes...

I'm really enjoying this solution! Additionally, rather than using depth, I've been explicitly declaring nested representations on a field by field basis. This allows me to control how deep each one will go and avoid circular references.

For example, let's say I had declared customers and addresses in such a way that a customer can have many addresses but also have one selected as the default. The models might look something like this:

# models.py

from django.db import models


class Customer(models.Model):
    name = models.CharField(max_length=100, blank=True, default="New Customer")
    default_address = models.ForeignKey('Address', related_name='customer_default_address', null=True, blank=True, on_delete= models.SET_NULL)


class Address(models.Model):
    content = models.CharField(max_length=100, blank=True, null=True)
    customer = models.ForeignKey('Customer', on_delete=models.CASCADE)

While Address.customer sets up the many-to-one relationship that allows a customer to have many addresses, Customer.default_address ensures that the user can specify one address to be used as default.

The problem with using depth when serializing these objects is that a customer will reference an address that will, in turn, reference back to the customer which will reference the address again that will reference back to the customer... and on and on... While setting depth = 1 might seem like an appropriate answer in this scenario, imagine if your Address model had nested data as well (for Municipality, Region, Country, etc...). Limiting the depth would prevent these resources from being nested while extending depth would cause the unwanted circular reference.

The answer, I've found, is to use a combination of explicit field declarations using serializers to nest data when desired, and selecting the appropriate serializer depending upon the method, as presented above.

Here, I create an additional Info serializer that explicitly nests the address data within the customer representation instead of using depth.

#serializers.py

from rest_framework import serializers
from .models import Customer, Address


class CustomerSerializer(serializers.ModelSerializer):
    class Meta:
        model = Customer
        fields = '__all__'


class CustomerAddressSerializer(serializers.ModelSerializer):
    class Meta:
        model = Address
        fields = '__all__'


class CustomerInfoSerializer(serializers.ModelSerializer):
    default_address = AddressSerializer()

    class Meta:
        model = Customer
        fields = '__all__'

Finally, I use the get_serializer_method, described above, to do the routing...

#views.py

from rest_framework import viewsets
from rest_framework.permissions import SAFE_METHODS
from .models import Customer, Address
from .serializers import CustomerSerializer, CustomerInfoSerializer, AddressSerializer

class CustomerView(viewsets.ModelViewSet):
    queryset = Customer.objects.all()
    filterset_fields = '__all__'
    search_fields = ['name']

    def get_serializer_class(self):
        if not self.request.method in SAFE_METHODS:
            return CustomerSerializer
        return CustomerInfoSerializer


class AddressView(viewsets.ModelViewSet):
    queryset = Address.objects.all()
    serializer_class = AddressSerializer
    filterset_fields = '__all__'
    search_fields = ['content']

This solution allows me to create/update customers by providing the primary key as the value for default_address while receiving the full nested address when reading.

@encode encode locked and limited conversation to collaborators Mar 8, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants