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

Avoid executing querysets when getting the string representation of Serializers with related fields #7783

Closed

Conversation

michelts
Copy link

@michelts michelts commented Mar 6, 2021

This way, if the serializer is inadvertently converted to a string, the queryset won't be executed. Take the snippet below as an example:

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)

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. It could be potentially harmful if the queryset you are using is not optimized to be listed sequentially and has millions of records.

The discussion #7782 has more details. I didn't convert the discussion to a ticket yet though.

This way, if the serializer is inadvertently converted to a string, the
queryset won't be executed.
Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Similar to the conclusion on the Django mailing list, I think this is artificially favouring an advanced use-case over the common case. The recommendation is to define __repr__ on your QuerySet if you want this kind of thing. (Let’s discuss on the discussion, but noting my general worry here.)

@michelts
Copy link
Author

michelts commented Mar 8, 2021

Closing after the conclusion this is not something to be solved at the django-rest-framework side. More details on discussion #7782.

@michelts michelts closed this Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants