Avoid printing/executing querysets present in a related field

[michelts]

Hello django-rest-framework developers!

First of all, thanks for this great software!

I had a simple view that was using a serializer with a PrimaryKeyRelatedField, something like:

class SampleSerializer(serializers.Serializer):
    user = serializers.PrimaryKeyRelatedField(
        queryset=User.objects.filter(is_active=True)
    )

def sample_view(request):
    serializer = SampleSerializer(data=request.data)
    data = serializer.is_valid(raise_exception=True)
    do_something_with(data)
    return response.Response(data, status=status.HTTP_200_OK)

This is a simplified example, but the point is: I was having an error after initializing the serializer (e.g. the do_something_with function was raising an unexpected exception).

The default Django logs setup was causing the serializer to be printed, I mean, to be converted to a string, and due to the way the printable serializer representation works, the queryset from the primary key related field was also printed and that caused some troubles.

Django has a default string representation for a queryset: if you try to print it, it will actually query 21 items from the database and print them. There is a discussion about the potential risk involved in that, as well as rejected bug reports here and here.

In my case, this specific view was querying a table with millions of rows, without a good sorting pattern. Luckily we could manage to find out what was causing performance problems and realized it was a hit to that whole table (thanks for Sentry and Datadog 😄), but this is a potential pain point that is hard to debug. I would feel more comfortable if django-rest-framework would prevent this to happen 😉

Django-rest-framework already deals with Manager instances. I'm not sure if it is because of the same reason, but I would suggest printing the queryset.query instead if the instance is a Queryset.

I created a sample project to simulate the problem and recorded a video with details on how to reproduce it.

[michelts]

I'm not sure about the workflow to escalate this discussion to be an issue. Anyway, I created the pull request #7783 with a possible solution.

[carltongibson]

Your immediate solution is to implement __repr__ on your query set.

As per the mailing list thread you posted on, this behaviour comes from Django and is very unlikely to change.

[michelts]

Hi @carltongibson

Yeah, that's what I did in my case! Thanks for the suggestion!

I was about to argue that if I would be using a queryset, I would be careful to avoid creating a situation like this. That the queryset is being evaluated passively by django-rest-framework and I have no control over it.

But even if I just set the queryset to a variable, I could cause the same situation:

def sample_view(request):
    queryset = User.objects.filter(is_active=True)
    raise Exception
    ...

The queryset variable would be logged and thus evaluated. With this in mind, I agree the solution is not in the django-rest-framework domain. May I close the PR?

[carltongibson]

Please do. Thanks.

IIRC the conclusion on the Django Developers thread was that "Yes, that can happen" and to implement the repr as you need, rather than change it in core, because the existing behaviour has other benefits, no least of which it being quite established.

If on rereading that thread you don't agree that would be the place to progress this. (But I doubt it will change.)

I hope that makes sense.

Thanks!

[michelts]

Hi @carltongibson

Yes, I agree!

In the beginning, I was thinking that since DRF was printing the queryset through the serializer, it could potentially avoid that, but I agree it is not a situation created exclusively by DRF, it is possible to reach the same situation with a regular Django view.

What do you think about documentation? Do you think it worth mentioning this situation?

I could see a warning when mentioning that one can inspect the model serializer. I also believe the details for the queryset parameter for the related fields could include a small note mentioning it, with a link to the former instructions.

I can write these adjustments and create a new PR if you agree. What do you think?

Thanks!

[carltongibson]

Hi @michelts, I don’t know... I’m not sure it’s feasible to document every possible caveat. (I’m not sure the value it adds is worth the noise...) Maybe others think different.