Improvements for pagination, sorting, filtering, and exception h…

[n2ygk]

…andling

• Pagination updates:
  • Improve existing documentation of pagination, mixins
    • Document LimitOffsetPagination
    • Describe how to override pagination class query parameter names.
    • Remove reference to PAGINATE_BY_PARAM which was deprecated in DRF 3.2.
    • Document SparseFieldsetsMixin
  • Add new default settings for pagination query parameters and maximum size.
• Add combinable mixins for filter and sort query parameters and make MultiplIDMixin combinable.
• Exceptions updates:
  • Document JSON_API_UNIFORM_EXCEPTIONS setting.
  • handle missing fields exception thrown by new filter and sort Mixins as a 400 error.
  • Catch all exceptions not caught by DRF and format as JSON API error objects instead of returning HTML error pages.

[n2ygk]

Still TODO: write test cases for the new code, but I wanted to get a chance for others to take a look and provide feedback.

[codecov-io]

Codecov Report

Merging #416 into master will increase coverage by 0.21%.
The diff coverage is 97.39%.

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
+ Coverage   91.75%   91.96%   +0.21%     
==========================================
  Files          55       55              
  Lines        2923     3026     +103     
==========================================
+ Hits         2682     2783     +101     
- Misses        241      243       +2

Impacted Files	Coverage Δ
example/tests/test_views.py	100% <100%> (ø)	⬆️
rest_framework_json_api/pagination.py	92.85% <100%> (+0.54%)	⬆️
example/views.py	91.93% <100%> (+0.41%)	⬆️
rest_framework_json_api/mixins.py	97.22% <100%> (+9.72%)	⬆️
rest_framework_json_api/exceptions.py	85.36% <81.25%> (+0.75%)	⬆️

---

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...0c2675b. Read the comment docs.

[n2ygk]

@mblayman Should I be adding stuff to the tox.ini so it matches whats in .travis.yml. flake8 and coverage are missing from it.

[n2ygk]

I think I've got enough test cases. @mblayman: ready to merge if you see fit.

[n2ygk]

@mblayman nagging about this PR.

[mblayman]

Hey, @n2ygk. Sorry for my super delayed response. I haven't really had time for DJA recently as I'm no longer using it on any regular basis. I've put out a request for a new maintainer (#424) so I hope that someone can step up and act as a proper reviewer for this PR.

[n2ygk]

@sliverc perhaps you can review this PR?

[sliverc]

@n2ygk Thanks a lot for this contribution. I really like that this improves conformance with jsonapi specification! Certainly the way forward.
It is though when the description doesn't fit into the commit message it is an indication that too many changes are in one PR... 😉
So could you split this PR up into severals? Maybe one for pagination, sorting, filtering and exception?
This would make reviewing easier and less error prone.
I try to review those quickly so you can also do one by one if you want to try to avoid merge conflicts.

[n2ygk]

@sliverc I could perhaps separate pagination from the others, but the Mixins and exception handler are linked, as the filter and sort mixins throw an exception if a query parameter contains an incorrect field name. I could also make the commit description shorter;-) If I were to split it into 4, it would I guess have to be in this order: 1) exception handler, 2) sort, 3) filter, 4) pagination. And then I'd have to split up the test case and documentation changes into 4 pieces as well. That's a lot of work. Maybe you can let me slide this time?

[sliverc]

Let me add some general comments before we decide how or whether we want to split this PR up.

• I do not think SortMixin is actually needed, rest framework already has such a feature with OrderingFilter. The only thing which needs to be configured is ORDERING_PARAM: 'sort'. I guess it would be good to add this to the documentation

• Also the FilterMixin should not be done as view mixin but rather DjangoFilterBackend be used. There is one difference between the filter backend and the json api specification which is that filters are added as 'filter[' and not simple query params. I think it is best to derive from DjangoFilterBackend to add this feature though.

• On pagination. Not sure how ideal it is that all different pagination classes need to be adjusted for this to work. Maybe we could think of a Middleware which passes the query parameters and renames them as default pagination classes need it (without the page[]). Also filter[ could be done this way potentially.

• On exception handling. I haven't had a look at the DRF implementation but maybe a change in DRF could make this easier for us instead of doing regex matching.

[n2ygk]

@sliverc I'm not familiar with how the DjangoFilterBackends are configured. I'll have to read up on that. I based my stuff on the MultipleIDMixin example.

[sliverc]

I haven't noticed that there is a MultipleIDMixin till #415

I think though we should rather deprecate it, as firstly I do not think the json api specification mentions that this is possible at all and I also don't think it is the right way to implement this as DRF has better options to do so (e.g. filter backends).

Question is how we want to move forward with this PR. As it implements quite a few things which we might need to consider how they are best implemented I would suggest we close this PR and we can open new PRs for each area of problem? (Not sure when I will get around but a few problems you are trying to address I certainly also encountered in the past and would like to see fixed)

What do you think? Or do you have any other suggestions?

[n2ygk]

@sliverc wrote:

I haven't noticed that there is a MultipleIDMixin till #415

I think though we should rather deprecate it, as firstly I do not think the json api specification mentions that this is possible at all and I also don't think it is the right way to implement this as DRF has better options to do so (e.g. filter backends).

MultipleIDMixin was added by @gaker in 2014. The jsonapi spec wasn't finalized until 2015 so things may have changed. Certainly the way to do this per jsonapi 1.0 would be to use &filter[id]=id1,id2,... so I agree about deprecation. I'm also OK with just leaving it as it's not used unless a developer adds it to their code. So we could deprecate it in the documentation.

Question is how we want to move forward with this PR. As it implements quite a few things which we might need to consider how they are best implemented I would suggest we close this PR and we can open new PRs for each area of problem? (Not sure when I will get around but a few problems you are trying to address I certainly also encountered in the past and would like to see fixed)

I can close this PR and split it up into exceptions, pagination improvements and mixins (or filter backends if that make more sense -- I will have to research that.)

[n2ygk]

@sliverc I finally think I might have a decent way to customize DjangoFilterBackend to allow `GET /resources/?filter[field]=value. Maybe something to add to this project or maybe just documentation, since filter syntax is undefined in the spec. Here's a snippet:

from django_filters.rest_framework import DjangoFilterBackend


class JsonApiFilterBackend(DjangoFilterBackend):
    """
    Override's django_filters.rest_framework.DjangoFilterBackend to use `filter[field]` query parameter.
    """

    def get_filterset_kwargs(self, request, queryset, view):
        """
        Turns filter[<field>]=<value> into <field>=<value> which is what DjangoFilterBackend expects
        N.B. `filter[all]` is special-cased for multi-field keyword searches done by rest_framework.filters.SearchFilter

        TODO: use SearchFilter's `search_param` instead of hardcoding
        """
        data = request.query_params.copy()
        for qp, val in data.items():
            if qp[:7] == 'filter[' and qp[-1] == ']' and qp != 'filter[all]':
                data[qp[7:-1]] = val
                del data[qp]
        return {
            'data': data,
            'queryset': queryset,
            'request': request,
        }

I don't know if the upstream project (django-filters) would accept a request to make the query parameters configurable in this way...

My settings snippet looks like this:

    'DEFAULT_FILTER_BACKENDS': (
        'rest_framework.filters.OrderingFilter',  # for sort
        'myapp.backends.JsonApiFilterBackend',  # for `filter[field]` filtering
        'rest_framework.filters.SearchFilter',  # for keyword filtering across multiple fields
    ),
    'ORDERING_PARAM': 'sort',
    'SEARCH_PARAM': 'filter[all]',

And viewset looks like this:

class CourseViewSet(CourseBaseViewSet):
    queryset = Course.objects.all()
    serializer_class = CourseSerializer
    filterset_fields = ('subject_area_code', 'course_name', 'course_description',)
    search_fields = ('course_name', 'course_description', 'course_identifier', 'course_number',)

[sliverc]

@n2ygk
I think to provide a specific json api filter backend is the right way to go. For now I would leave out search filter (potentially something to address in DRF) and concentrate on the filter backend.

As filter[...] param is part of the json api spec I think it would be good when we provide such a backend in json api.

I think what such a filter backend should also do is formatting taking JSON_API_FORMAT_FIELD_NAMES into account. Otherwise would a filter name be underscored e.g. field[test_name] which is not really consistent.

Feel free to open up with a PR with this.

[n2ygk]

@sliverc I'll start a PR shortly. Just a few items to get your feedback:

• The filter implementation should be Djangoistic (e.g. use the various django-filter backends, queries via the ORM, etc.
• It should be a relatively simple to use a filter which the jsonapi spec intentionally does not define. So it's a "use this at your option for lack of a better one"; we can't claim it complies with the spec because there is none other than this statement: "The filter query parameter is reserved for filtering data. Servers and clients SHOULD use this key for filtering operations." However, there are also some recommendations that lead to the use of filter[stuff].
• It should properly map between model representation and DJA representations of field names (use JSON_API_FORMAT_FIELD_NAMES).
• Should it map django orm use of __ (double U+005F LOW LINE, “_”) some other way since double __ is a legal component of a member name? So that in djangoistic style one would write filter[courses__course_terms__term_identifier]=20181 perhaps it should use "U+002E PERIOD, “.” (used as a separator within relationship paths)": filter[courses.courses_terms.term_identifier]=20181.
• How do we have all the things we like about the ORM such as __gt or __in in a filter? filter[courses.course_terms.term_identfier.gt]=2018? Is that still a relationship path component. Since filters are not defined by the spec, I would argue that this still follows the style of the spec.
• It must return a 400 status for unknown query parameters ("If a server encounters a query parameter that does not follow the naming conventions above, and the server does not know how to process it as a query parameter from this specification, it MUST return 400 Bad Request."). With the current approach. unknown query parameters are currently just silently ignored.
• Should it be a single backend filter that one can include in REST_FRAMEWORK['DEFAULT_FILTER_BACKENDS'] or just documented usage of multiple filters as it is (mostly) now? For example, include all these filters in a single JsonApiFilterBackend:
  - rest_framework.filters.OrderingFilter and ordering_param = 'sort'
  - django_filters.rest_framework.DjangoFilterBackend to use filter[field] query parameter
  - rest_framework.filters.SearchFilter to use search_param=filter[all]
  Probably keep them separate to allow the most configurability for the developer, I think. Maybe with a better way to override the REST_FRAMEWORK defaults?

[sliverc]

Thanks for summarizing. The points I ticked above look good to me.

Concerning the others points some thoughts:
DJA takes care of json api specification details. Everything which is not covered by the specification should be left to other libraries with a potential glue in DJA if necessary.

This means we should not worry about how filter are named, whether __ or gt are allowed etc. What we should worry about is that the right query param name is used (filter[...] as defined in the spec) and error codes as you have outlined (Thanks for collecting all the json api specification links. Really helpful!).

django-filter has its own way of defining filters which is fairly static (no dynamic filters). Maybe we also add a glue to https://github.com/philipn/django-rest-framework-filters which is more dynamic for users which prefer Django ORM specific filters.

What do you think?

To keep it simple and get started I would create a DJA version of Django Filter backend and once this is integrated can be expanded.

[n2ygk]

@sliverc You don't like rewriting . as __? That gets us proper relationship path component separation.

[sliverc]

I think in a first implementation I would certainly leave it out and keep it simple. Once it is established we can still think about improving it (although the specification is silent on this - it only talks about member names but filter is not a member).

[n2ygk]

Actually a filter is a member; that’s what goes inside the brackets per the examples. At least that’s my reading of it.

…

On Tue, Aug 7, 2018 at 3:03 AM Oliver Sauder ***@***.***> wrote: I think in a first implementation I would certainly leave it out and keep it simple. Once it is established we can still think about improving it (although the specification is silent on this - it only talks about member names but filter is not a member). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#416 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJ5d5OFCwlf3Z6V9ac2bwRU3OvjV9Y-ks5uOTujgaJpZM4S2L4M> .

[sliverc]

Interesting I understand the specification completely different 😄

Note: JSON API is agnostic about the strategies supported by a server. The filter query parameter can be used as the basis for any number of filtering strategies.

According to this is anything is allowed as filter as long as it uses the filter query param e.g. also filter[search] or anything else. The rest is up to the designer of the api.
The example does show member names but this doesn't have to be this way as JSON API is filter strategy agnostic. It's just an example in the end. Therefore I think DJA should also be agnostic to it and not decide for the api designer how to do filtering. This does mean though that several options can be provided of course which I think is best done with integrating different filtering libraries.

[n2ygk]

@sliverc is there an inverse function for JSON_API_FORMAT_FIELD_NAMES? I see utils.format_field_names for one direction... Can it even be reversed? I need to accept sort=keyName and convert it to internal model's field name key_name or something... in order to pass the correct keys to OrderingFilter and so on.

-- https://django-rest-framework-json-api.readthedocs.io/en/stable/usage.html#inflecting-object-and-relation-keys

[sliverc]

Copied from JSONParser where it basicially needs to do the same thing:

# convert back to python/rest_framework's preferred underscore format
return utils._format_object(attributes, 'underscore')