Allow overwriting of get_queryset() of related field

[sliverc]

As documented in http://www.django-rest-framework.org/api-guide/relations/#custom-relational-fields this allows to implement custom ResourceRelatedField where queryset is depended on context.

[codecov-io]

Codecov Report

Merging #415 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #415      +/-   ##
==========================================
+ Coverage   91.75%   91.76%   +0.01%     
==========================================
  Files          55       55              
  Lines        2923     2927       +4     
==========================================
+ Hits         2682     2686       +4     
  Misses        241      241

Impacted Files	Coverage Δ
rest_framework_json_api/relations.py	82.95% <100%> (ø)	⬆️
rest_framework_json_api/mixins.py	88.88% <100%> (+1.38%)	⬆️
example/tests/test_relations.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e501b04...4895e3d. Read the comment docs.

[n2ygk]

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?

[sliverc]

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

It is adjusted now.

[n2ygk]

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).

[sliverc]

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.

[n2ygk]

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.

[sliverc]

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.

[n2ygk]

@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.

[n2ygk]

@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.

[n2ygk]

Request use self.queryset vs queryset.

(Sorry, still learning how to be a proper reviewer)

[sliverc]

@n2ygk No worries. Thanks for review and see my comment above to your question.

I was wrong

[n2ygk]

looks good. sorry for the confusion around self.queryset.