Reimplementing pagination

[lucacorti]

This implements page/offset/cursor pagination strategies in the serializer by passing a JSONAPI.Page object to the serialize function and implementing basic page, offset and cursor strategies. It also gets rid of the now useless with_pagination knob.

[lucacorti]

After some thought, I think pagination strategies do not belong into the serializer. The JSONAPI spec does not mandate any particular strategy, it just reserves the use of the page query param for pagination.

So, my latest commit provides an extensible behavior for dealing with pagination which basically boils down to delegating pagination links to the view pagination_links overridable function.

The deafult implementation of the function provides no links, unless a paginator option is provided to use JSONAPI.View or a global paginator configuration is provided. These point to a module conforming to JSONAPI.Paginator which defines one callback function to delegate links generation to. This allows users to implement custom pagination strategies. The page struct from the conn assigns is used to retrieve total items/pages information which can be patched in the conn at the controller level.

An implementation of a page-based strategy is implemented. Tests are updated and demonstrate the usage.

I think this improves on the previous iteration, thoughts?

[doomspork]

Sorry for missing this PR @lucacorti! We'll get eyes on it this week and let you know our thoughts, thanks for getting involved 😁

[doomspork]

Looking good @lucacorti! Some comments and thoughts on the code so far

[lucacorti]

The failing test esposes a bug (#191) in the QueryParser, which ends up populating the JSONAPI.Page struct with string values from the query parametes, which in turn break the paginator which expects integers as per JSONAPI.Page typespec.

[jherdman]

Hey friends. Sorry for taking so long to get to this. Life, etc.

I haven't used the current pagination support, so I have some background questions:

• What are the problems with the current implementation?
• Does pagination even belong in the "core" of this project? Should it be extracted into a supporting addon?

[lucacorti]

JSON API mandates the use of the page query params and defines standard pagination links format in the response, beyond that everything is implementation specific. So I think generic pagination support belongs in the library. My PR goes in this direction. The provided page-based implementation is merely an example implementation. I guess not shipping it or any other specific implementation would be fine, but could be nice to have out of the box.

The main problem with the current implementation from my point of view is everything is done into the links callback and doesn't offer much flexibility. In my opinion the links callback is a good place to address custom links, while being able to delegate pagination to a different module allows to define a clear interface, configure pagination at project/view level and possibly override the pagination_links for even more flexibility.

So IMHO basic pagination support belongs in the project, specific pagination implementations maybe not.

Another thing I'd like to add to my PR to go down the spec compliance route even more is allowing the user to specify which query params map to which JSONAPI.Page struct members, because the usage of specific parameters is outside the scope of the spec and the library is enforcing the use of specific parameter names. Again, the current ones may stay as defaults, but the user should be able to verride them.

[lucacorti]

Latest commit fixes #191 and adds support for custom query parameters for pagination via

config :jsonapi, :page_query_param, "mypage"

For all pagination query params inside page[].

[jherdman]

Generally speaking I feel like this PR needs to be approached slightly differently. Currently:

• There's a fix for JSONAPI.Page struct attrs parsed as strings #191 in it
• It removes existing behaviour
• Adds new behaviour

I know this will be more work and it's a lot to ask from someone freely contributing their time, but I'd like to see an alternate approach.

1. Extract the fix for JSONAPI.Page struct attrs parsed as strings #191 into a distinct PR. This gives us the option of releasing it independently to see feedback from users of this package.
2. Construct this PR in a way that offers existing users of the pagination approach a cleaner, and documented, way to upgrade. My understanding is that this code would break existing consumption of this package, thus necessitating a major release without a release informing of a deprecated pathway, and a pathway to upgrade.

[lucacorti]

@jherdman Sure, I'll make a separate PR for #191.

As far as this PR goes, documentation is the next step. If a major release is acceptable for the project, it should suffice, otherwise I'd like to discuss ways in which the upgrade path should be smoothed.

[lucacorti]

@jherdman

So, #192 addresses #191.

Also I gave some more thought about the upgrade path and realized my PR already addresses this in a backwards compatible way. Pagination links currently generated in the view links callback should be preserved if the user doesn't opt-in to the new pagination style by either implementing the pagination_links callback or by passing a paginator module to the view/config.

I guess properly documenting this behavior and how to migrate to the new one should be enough.

[lucacorti]

So, I'd like to recap where I'm trying to go with this PR.

The current implementation has a few issues:

1. It does not expose a simple and clear interface to handle pagination.
2. It enforces a specific way to pass pagination parameters and the type of values in these which is not in line with the JSON API spec and limits pagination schemes that can be implemented.

What this PR does:

1. Moves pagination link generation to a new pagination_links callback in the view, which can be used to provide pagination links.
2. Provides a default implementation of the pagination links callback which delegates pagination to a module which handles pagination link generation through a well defined behavior. The pagination behavior can be implemented, configured globally, overriden per view.
3. Gets rid of the JSONAPI.Page struct and value parsing and just relies on the page query parameter contents to provide context for pagination, which is all JSON:API mandates. It also adds a way to pass needed context back to the pagination module (ie. total_items/total_pages) from the controller.
4. Provides backwards compatibility. If the user does not opt in to the new system, pagination links generated in the links callback should still work as expected.

What is missing to complete the work

1. Documentation about the new behaviour and how to opt in to the new pagination api.

[jherdman]

A few quick notes above. The one thing I'm really uncertain about is what a Paginator is or does. Is it the thing that actually does the paginating, or is that done by something else? If the latter, perhaps we should document it, or add a smoke test for it?

[jherdman]

How hard would it be to resolve the issues in #131 using your implementation?

[lucacorti]

@jherdman I think this PR and #131 are solving the same problems in two fundamentally different ways. Which issues in #131 are you referring to?

[lucacorti]

@jherdman About the Paginator behavior, its responsibility is to just generate links, actual pagination is done externally (in my case Scrivener paginating Ecto models), because knowledge about the actual data is needed to do that. No assumption is made about how pagination is actually performed.

This is why options are passed back from the controller to the paginator module in form of options, to pass back information from the pagination process needed to generate links.

[jherdman]

@jherdman I think this PR and #131 are solving the same problems in two fundamentally different ways. Which issues in #131 are you referring to?

Ah, my bad. I think we can ignore this comment.

[lucacorti]

@jherdman I've updated the tests and added documentation. I think this is quite complete. Anything else we need to address?

[jherdman]

@lucacorti LGTM! It needs a rebase, but I'd love to get @doomspork or @snewcomer 's eyes on it too.

[doomspork]

@jherdman / @lucacorti this looks good to me. Thank you both for putting in so much time to get this across the finish line.

[lucacorti]

@jherdman @doomspork Thank you for bearing with me while I was iterating on the implementation.

[doomspork]

Thank you @lucacorti for doing that. It's truly awesome to have open source contributors like you, I cannot stress enough how much we value people like yourself. I hope you'll continue your contributions 😁