Skip to content

Allow overwriting of get_queryset() of related field #415

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 6, 2018
Merged

Allow overwriting of get_queryset() of related field #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5936c0e
Allow overwriting of get_queryset() of related field
Mar 14, 2018
4895e3d
Always use get_queryset to retrieve queryset
May 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/usage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ class LineItemViewSet(viewsets.ModelViewSet):
serializer_class = LineItemSerializer

def get_queryset(self):
queryset = self.queryset
queryset = super(LineItemViewSet, self).get_queryset()

# if this viewset is accessed via the 'order-lineitems-list' route,
# it wll have been passed the `order_pk` kwarg and the queryset
Expand Down
7 changes: 6 additions & 1 deletion example/tests/test_relations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,13 @@ def test_invalid_resource_id_object(self):
}


class BlogResourceRelatedField(ResourceRelatedField):
def get_queryset(self):
return Blog.objects


class BlogFKSerializer(serializers.Serializer):
blog = ResourceRelatedField(queryset=Blog.objects)
blog = BlogResourceRelatedField()


class EntryFKSerializer(serializers.Serializer):
Expand Down
5 changes: 3 additions & 2 deletions rest_framework_json_api/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ def get_queryset(self):
"""
Override :meth:``get_queryset``
"""
queryset = super(MultipleIDMixin, self).get_queryset()
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to just use self.queryset = ... rather than creating a new local queryset variable? That would lead to a more consistent code style (see #416).

Copy link
Member Author

Choose a reason for hiding this comment

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

If I used self.queryset the output of get_queryset might be different when calling get_queryset several times and we certainly want to avoid this.

Hope this makes it clear otherwise let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so: self.queryset as an lvalue is updated as the return value of self.get_queryset(). Worst case, it gets the exact same value that get_queryset() sets it to. You always want the result of the last get_queryset to be in self.queryset.

Am I misunderstanding something here? My tests with the new Mixins bear this out. Stacking several Mixins does the "right" thing, building up a list of object manager calls to further narrow the query set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me try to outline what happens when we overwrite self.queryset and get_queryset gets called multiple times.

This is just a simplified pseudo version of what actually happens in DRF:

class BaseView():
  def get_queryset():
    return self.queryset

class MyMixin():
  def get_queryset():
    queryset = super().get_queryset()
    self.queryset = queryset.filter(username='test')
    return self.queryset

class MyView(MyMixin, BaseView):
  queryset = User.objects.all()


view = MyView()
# first get queryset call
queryset = view.get_queryset()
print(queryset.query)
# Output is SELECT * FROM auth_user WHERE username = test

# second get queryset call
queryset = view.get_queryset()
print(queryset.query)
# Output is SELECT * FROM auth_user WHERE username = test AND username = test

When self.queryset is not overwritten the result of the 2nd call is the same as the first call. In my above example the query is not the same but the result will still be the same. However there might be even example where results might be different when traversing through ManyToMany relations and several joins happens.

Hope this explains.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sliverc Sorry, I still don't see how setting self.queryset is incorrect. Following is a log where I've added prints to the mixins on entry and exit and the expected behavior is happening for this request:

GET /v1/courses/?sort=course_name,course_identifier&filter[subject_area_code]=ACCT&filter[course_number]=75810,75819,73272

At class definition:

CourseViewSet      queryset: SELECT "myapp_course"."id", "myapp_course"."effective_start_date", "myapp_course"."effective_end_date", "myapp_course"."last_mod_user_name", "myapp_course"."last_mod_date", "myapp_course"."school_bulletin_prefix_code", "myapp_course"."suffix_two", "myapp_course"."subject_area_code", "myapp_course"."course_number", "myapp_course"."course_identifier", "myapp_course"."course_name", "myapp_course"."course_description" FROM "myapp_course" ORDER BY "myapp_course"."course_number" ASC

and then execution of the above GET:

SortMixin    entry queryset: SELECT "myapp_course"."id", "myapp_course"."effective_start_date", "myapp_course"."effective_end_date", "myapp_course"."last_mod_user_name", "myapp_course"."last_mod_date", "myapp_course"."school_bulletin_prefix_code", "myapp_course"."suffix_two", "myapp_course"."subject_area_code", "myapp_course"."course_number", "myapp_course"."course_identifier", "myapp_course"."course_name", "myapp_course"."course_description" FROM "myapp_course" ORDER BY "myapp_course"."course_number" ASC
(0.001) SELECT COUNT(*) AS "__count" FROM "myapp_course" WHERE ("myapp_course"."course_number" IN ('75819', '73272', '75810') AND "myapp_course"."subject_area_code" IN ('ACCT')); args=('75819', '73272', '75810', 'ACCT')
FilterMixin  entry queryset: SELECT "myapp_course"."id", "myapp_course"."effective_start_date", "myapp_course"."effective_end_date", "myapp_course"."last_mod_user_name", "myapp_course"."last_mod_date", "myapp_course"."school_bulletin_prefix_code", "myapp_course"."suffix_two", "myapp_course"."subject_area_code", "myapp_course"."course_number", "myapp_course"."course_identifier", "myapp_course"."course_name", "myapp_course"."course_description" FROM "myapp_course" ORDER BY "myapp_course"."course_number" ASC
FilterMixin return queryset: SELECT "myapp_course"."id", "myapp_course"."effective_start_date", "myapp_course"."effective_end_date", "myapp_course"."last_mod_user_name", "myapp_course"."last_mod_date", "myapp_course"."school_bulletin_prefix_code", "myapp_course"."suffix_two", "myapp_course"."subject_area_code", "myapp_course"."course_number", "myapp_course"."course_identifier", "myapp_course"."course_name", "myapp_course"."course_description" FROM "myapp_course" WHERE ("myapp_course"."course_number" IN (75819, 73272, 75810) AND "myapp_course"."subject_area_code" IN (ACCT)) ORDER BY "myapp_course"."course_number" ASC
SortMixin   return queryset: SELECT "myapp_course"."id", "myapp_course"."effective_start_date", "myapp_course"."effective_end_date", "myapp_course"."last_mod_user_name", "myapp_course"."last_mod_date", "myapp_course"."school_bulletin_prefix_code", "myapp_course"."suffix_two", "myapp_course"."subject_area_code", "myapp_course"."course_number", "myapp_course"."course_identifier", "myapp_course"."course_name", "myapp_course"."course_description" FROM "myapp_course" WHERE ("myapp_course"."course_number" IN (75819, 73272, 75810) AND "myapp_course"."subject_area_code" IN (ACCT)) ORDER BY "myapp_course"."course_name" ASC, "myapp_course"."course_identifier" ASC
(0.001) SELECT "myapp_course"."id", "myapp_course"."effective_start_date", "myapp_course"."effective_end_date", "myapp_course"."last_mod_user_name", "myapp_course"."last_mod_date", "myapp_course"."school_bulletin_prefix_code", "myapp_course"."suffix_two", "myapp_course"."subject_area_code", "myapp_course"."course_number", "myapp_course"."course_identifier", "myapp_course"."course_name", "myapp_course"."course_description" FROM "myapp_course" WHERE ("myapp_course"."course_number" IN ('75819', '73272', '75810') AND "myapp_course"."subject_area_code" IN ('ACCT')) ORDER BY "myapp_course"."course_name" ASC, "myapp_course"."course_identifier" ASC LIMIT 4; args=('75819', '73272', '75810', 'ACCT')
(0.000) SELECT "myapp_courseterm"."id", "myapp_courseterm"."effective_start_date", "myapp_courseterm"."effective_end_date", "myapp_courseterm"."last_mod_user_name", "myapp_courseterm"."last_mod_date", "myapp_courseterm"."term_identifier", "myapp_courseterm"."audit_permitted_code", "myapp_courseterm"."exam_credit_flag", "myapp_courseterm"."course_id" FROM "myapp_courseterm" WHERE "myapp_courseterm"."course_id" = '860cbf005f81402b995020f5b1aa15a8' ORDER BY "myapp_courseterm"."term_identifier" ASC; args=('860cbf005f81402b995020f5b1aa15a8',)
(0.000) SELECT "myapp_courseterm"."id", "myapp_courseterm"."effective_start_date", "myapp_courseterm"."effective_end_date", "myapp_courseterm"."last_mod_user_name", "myapp_courseterm"."last_mod_date", "myapp_courseterm"."term_identifier", "myapp_courseterm"."audit_permitted_code", "myapp_courseterm"."exam_credit_flag", "myapp_courseterm"."course_id" FROM "myapp_courseterm" WHERE "myapp_courseterm"."course_id" = '06d136801b7247e584b389a8a3c5bbfd' ORDER BY "myapp_courseterm"."term_identifier" ASC; args=('06d136801b7247e584b389a8a3c5bbfd',)
(0.000) SELECT "myapp_courseterm"."id", "myapp_courseterm"."effective_start_date", "myapp_courseterm"."effective_end_date", "myapp_courseterm"."last_mod_user_name", "myapp_courseterm"."last_mod_date", "myapp_courseterm"."term_identifier", "myapp_courseterm"."audit_permitted_code", "myapp_courseterm"."exam_credit_flag", "myapp_courseterm"."course_id" FROM "myapp_courseterm" WHERE "myapp_courseterm"."course_id" = '3b8ce22ba4274159bba7f43a7049009a' ORDER BY "myapp_courseterm"."term_identifier" ASC; args=('3b8ce22ba4274159bba7f43a7049009a',)
(0.000) SELECT "myapp_courseterm"."id", "myapp_courseterm"."effective_start_date", "myapp_courseterm"."effective_end_date", "myapp_courseterm"."last_mod_user_name", "myapp_courseterm"."last_mod_date", "myapp_courseterm"."term_identifier", "myapp_courseterm"."audit_permitted_code", "myapp_courseterm"."exam_credit_flag", "myapp_courseterm"."course_id" FROM "myapp_courseterm" WHERE "myapp_courseterm"."course_id" = 'd420982ca54c43a790bd7750ec87191c' ORDER BY "myapp_courseterm"."term_identifier" ASC; args=('d420982ca54c43a790bd7750ec87191c',)

The lines with (0.000) are the SQL from django.db.backends logging.

Here's the code that includes the prints:

# myapp/views.py:
class CourseBaseViewSet(AuthnAuthzMixIn, SortMixin, FilterMixin, viewsets.ModelViewSet):
    pass


class CourseViewSet(CourseBaseViewSet):
    # API endpoint that allows course to be viewed or edited.
    queryset = Course.objects.all()
    print("CourseViewSet      queryset: {}".format(queryset.query))
    serializer_class = CourseSerializer
# rest_framework_json_api/mixins.py:
class FilterMixin(object):
    """
    A stackable Mixin that overrides get_queryset for JSON API filter query parameter support
    per http://jsonapi.org/recommendations/#filtering.

    The `filter` syntax is
    `filter[name1]=list,of,alternative,values&filter[name2]=more,alternatives...`
    which can be interpreted as `(name1 in [list,of,alternative,values])
    and (name2 in [more,alternatives])`
    `name` can be `id` or attributes field.

    @example
    GET /widgets/?filter[name1]=can+opener,tap&filter[name2]=foo

    TODO: decide if we want to allow multiple instances of the *same* filter[field]
      e.g. Is "&filter[id]==123&filter[id]=345" the same as "&filter[id]=123,345"?
      For now additional instances of the same filter[field] are ignored.
    """

    def get_queryset(self):
        """
        Override :meth:``get_queryset``
        """
        print("FilterMixin  entry queryset: {}".format(self.queryset.query))
        self.queryset = super(FilterMixin, self).get_queryset()
        qp = dict(self.request.query_params if hasattr(self.request, 'query_params')
                  else self.request.QUERY_PARAMS)
        if not qp:
            print("FilterMixin return queryset (no qp): {}".format(self.queryset.query))
            return self.queryset
        FILTER = 'filter['
        flen = len(FILTER)
        filters = {}
        for k in qp:
            if k[:flen] == FILTER and k[-1] == ']':
                attr = k[flen:-1]
                filters[attr + "__in"] = qp.get(k)[0].split(',')

        self.queryset = self.queryset.filter(**filters)
        print("FilterMixin return queryset: {}".format(self.queryset.query))
        return self.queryset


class SortMixin(object):
    """
    A stackable Mixin that overrides get_queryset for JSON API sort query parameter support
    per http://jsonapi.org/format/#fetching-sorting.

    The `sort` syntax is `sort=-field1,field2,...`
    The `sort` parameter is allowed to be given more than once and the lists are combined.
    e.g. "&sort=field1&sort=field2" is the same as "&sort=field1,field2".
    TODO: Decide if allowing repeats of the sort parameter is correct.
    TODO: Add dot-separated type.field to enable sorting by relationship attributes.

    @example
    GET /widgets/?sort=-name1,name2&sort=name3,name4
    """

    def get_queryset(self):
        """
        Override :meth:``get_queryset``
        """
        print("SortMixin    entry queryset: {}".format(self.queryset.query))
        self.queryset = super(SortMixin, self).get_queryset()
        qp = dict(self.request.query_params if hasattr(self.request, 'query_params')
                  else self.request.QUERY_PARAMS)
        if not qp:
            print("SortMixin   return queryset (no qp): {}".format(self.queryset.query))
            return self.queryset
        sorts = []
        if 'sort' in qp:
            for s in qp.get('sort'):
                sorts += s.split(',')

        self.queryset = self.queryset.order_by(*sorts)
        print("SortMixin   return queryset: {}".format(self.queryset.query))
        return self.queryset

Since all the super methods do the same thing: setting self.queryset and then returning it, it doesn't seem to matter if I use a local variable instead of self.queryset, except if there's some other code elsewhere in DJA or DRF that just references self.queryset instead of calling get_queryset, which is entirely possible. So it's a belt-and-suspenders, but not broken.

If get_queryset() were instead an @property queryset() getter for self.queryset, then referencing self.queryset would be guaranteed to call the getter, but it is not, so not only does it not hurt to set self.queryset in get_queryset() but it guarantees that it's got the latest value in case it's used directly.

I hope this makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sliverc OK, so after writing all the above I've taken a look at DRF's use of queryset and now I think I understand it a little better.

Never mind! I'll dismiss my suggestion and merge.

if hasattr(self.request, 'query_params'):
ids = dict(self.request.query_params).get('ids[]')
else:
ids = dict(self.request.QUERY_PARAMS).get('ids[]')
if ids:
self.queryset = self.queryset.filter(id__in=ids)
return self.queryset
queryset = queryset.filter(id__in=ids)
return queryset
2 changes: 1 addition & 1 deletion rest_framework_json_api/relations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def to_internal_value(self, data):
if not isinstance(data, dict):
self.fail('incorrect_type', data_type=type(data).__name__)

expected_relation_type = get_resource_type_from_queryset(self.queryset)
expected_relation_type = get_resource_type_from_queryset(self.get_queryset())
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this use of get_queryset() instead of queryset as some of the mixins I've proposed in #416 take advantage of queryset stacking with, for example, self.queryset = super(FilterMixin, self).get_queryset()

Are there other places in the code where self.queryset should be replaced by self.get_queryset() to make this a consistent approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have searched through the code and found one more in the documentation and the MultipleIDMixin mixin.

It is adjusted now.

serializer_resource_type = self.get_resource_type_from_included_serializer()

if serializer_resource_type is not None:
Expand Down