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

Generate Page Links without Pagination #59

Closed
jyoung opened this issue Dec 18, 2015 · 13 comments
Closed

Generate Page Links without Pagination #59

jyoung opened this issue Dec 18, 2015 · 13 comments

Comments

@jyoung
Copy link

jyoung commented Dec 18, 2015

Is there a way to generate the page links without the library handling the paging?

Basically the data has already been paged and I just need to get the page links generated.

@joukevandermaas
Copy link
Owner

Not currently, although I have been thinking about this and I'm trying out some approaches to making this work. As soon as I'm happy with it, I'll publish a prerelease package so you can try it out.

Just out of curiosity, is there a specific reason you can't have the library handle pagination for you? I'm asking because I know pagination in Saule is somewhat limited currently, and I'd like to know what people are actually doing to make it better.

@jyoung
Copy link
Author

jyoung commented Dec 21, 2015

We use a library that completely encapsulates the data access and the
result the query object is already paged.

The really cool benefit of your library is that you define a resource and
it gets out of your way, though in some cases such as this we need some
more extension points.

If we could set the page size, page number (current page) and a total
record count or total number pages, then we could get all the paging links
in document as well. The "last" link does not get generated.

Being able to specify meta data would be really useful as well.

Thank you for this library, it has been incredibly helpful.

On Fri, Dec 18, 2015 at 3:31 PM, Jouke van der Maas <
notifications@github.com> wrote:

Not currently, although I have been thinking about this and I'm trying out
some approaches to making this work. As soon as I'm happy with it, I'll
publish a prerelease package so you can try it out.

Just out of curiosity, is there a specific reason you can't have the
library handle pagination for you? I'm asking because I know pagination in
Saule is somewhat limited currently, and I'd like to know what people are
actually doing to make it better.


Reply to this email directly or view it on GitHub
#59 (comment)
.

@joukevandermaas joukevandermaas modified the milestone: 1.5 Jan 4, 2016
@joukevandermaas
Copy link
Owner

I've been thinking about the problem, and I'm thinking of providing a hook that Saule calls to find the current, next, previous, etc. page numbers. Ideally I'd let users provide the pagination scheme as well. The only thing I'm not sure how to do, is to provide the requested page to the action method.

Is this library you are using publicly available? If so I'd like to take a look at it. If not, how do you provide paging information to it? I assume you would need to do that somewhere in your action method.

@joukevandermaas joukevandermaas removed this from the 1.5 milestone Oct 23, 2016
@bjornharrtell
Copy link
Contributor

@joukevandermaas would you be interested in sponsorship towards this feature?

@joukevandermaas
Copy link
Owner

@bjornharrtell not really. I feel like involving money brings in a lot of other complications. Thanks for the offer.

I am interested in working on this, though. This, #128 and #78 are some of the things I think Saule can really use.

@bjornharrtell
Copy link
Contributor

Ok that's understandable. Let me know what I can do to help. Will likely try to implement in near future but any input on how is of course appreciated.

@joukevandermaas
Copy link
Owner

I think this involves a refactor in the way pagination gets applied. I'd like to move that code to ApiResource and clean up the API a bit. In the end I think it would be nice if this worked similar to the new meta support, where the virtual method returns an object that contains the new collection (after applying pagination) and the links that should be generated (where null means no link).

By doing it that way you should get:

  1. same default behavior as now
  2. when you just need to add e.g. a last link, you can override that method, call the base and just add the one property
  3. when you have already applied pagination, you can just return the collection and populate the links object
  4. when you have a custom pagination scheme, you can write that code here

The method should take as many parameters as needed to make this convenient. I'm expecting this will be the collection you returned from the action method, all query parameters related to pagination and maybe the type of the items in the collection.

If it turns out to be really hard to write code that applies pagination using generics, we might consider a different, typed API that is easier to use. We could also expose some helpers that would make this easier.

@joukevandermaas
Copy link
Owner

(a long-term plan would be to move filters and sorting into this class too, that way everything works the same way and consumers get complete control if they need it)

@bjornharrtell
Copy link
Contributor

Revisiting this issue I agree that using an approach similar to the meta support would be nice but I have one concern.

The concern is when I need some additional context (probably owned by the controller) to calculate meta and/or links data. To examplify, in the code at http://awesan.com/saule/content/8-adding-metadata.html the Total is calculated with people.Count(). What if that operation is too slow and we have the total count at some other source accessable using a service that the controller owns? I.e I'd like to be able to somehow inject a context into the GetMetadata method and the equivalent future method for links.

@FLOWii-CRM
Copy link

I'm currently facing the same issue.

There is a pretty simple solution to this problem in another project - Telerik MVC.

  1. PaginatedAttribute (GridActionAttribute in Telerik) has "EnableCustomBinding" property - if set to true, it won't filter through a result.

  2. Instead of returing a plain collection, one needs to return a "GridModel" object containing "Data" (collection) and "Total" (total count). Links could be then simply generated from Page, PerPage, and GridModel.Total without necessity of having a whole collection returned.

  3. PaginatedAttribute is not a sealed class so one can override its behaviour for on demand "EnableCustomBinding" switching.

  4. Maybe a bit offtopic, but I also think static class Constants should not be internal in case one needs to access request properties from outside of a Saule. There is a lot of sealed/internal classes in Saule that might be better off being public to let people impelement custom behaviour as in Telerik.

@joukevandermaas
Copy link
Owner

The use-case in this issue is supported as of #181, but I haven't had time to document it for a stable release yet. In general I don't think an issue should be closed if the new API does not have documentation. If you're happy looking at the PR to see how it works, you can use the latest pre-release to get this functionality today.

There is a lot of sealed/internal classes in Saule that might be better off being public to let people impelement custom behaviour

I agree with this in theory, but I want to think properly about what the API for those should be. Simply making existing stuff public without any thought will probably lead to bad usability. That said, I have accepted some recent PRs that made existing internal class public (possibly with some modifications). I'm generally happy making Saule more flexible as long as it doesn't break the simple 'getting started without configuration' cases.

@FLOWii-CRM
Copy link

@joukevandermaas Thanks for a tip. Should have read changelog more carefully.

A small insight maybe. While "HandlesQueryAttribute" solution seems to work for what it does, it does not allow Saule to create a "last" page link as described in here: http://jsonapi.org/format/#fetching-pagination, because it does not know what a total count is. It will also generate an incorrect "next" page link when being on a "last" page for a same reason.

@joukevandermaas
Copy link
Owner

Since the documentation now exists, i will close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants