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

PanacheQuery.range() #3870

Closed
loicmathieu opened this issue Sep 5, 2019 · 22 comments · Fixed by #7510
Closed

PanacheQuery.range() #3870

loicmathieu opened this issue Sep 5, 2019 · 22 comments · Fixed by #7510
Labels
kind/enhancement New feature or request
Milestone

Comments

@loicmathieu
Copy link
Contributor

loicmathieu commented Sep 5, 2019

Description
PanacheQuery support a paging mechanism that allow to fetch the result of a query page by page. This is handy to list all the result of a query but not to give an external application the capability to fetch all the results.

Moreover, paging is statefull as it implies to store the size of the page and the current page index.

In API developement, range query is often implemented by providing an extra query parameter ?range=0-10, this is suitable to be used with HTTP range headers defined in the follwing RFC; https://tools.ietf.org/html/rfc7233

Building such range capability where a user can ask for any range within the list of result is not suitable with the current paging implementation.

So I propose to add a PanacheQuery.range() method for this.

Implementation ideas
Add a PanacheQuery.range(startIdx, endIdx) method that will allow to set offset and limit on the generated SQL query as the current paging mechanism do.

If it's OK, I can works on a PR for this.

@stale
Copy link

stale bot commented Nov 13, 2019

This issue/pullrequest has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 13, 2019
@loicmathieu
Copy link
Contributor Author

Still needed this functionality.

@stale stale bot removed the stale label Nov 13, 2019
@emmanuelbernard
Copy link
Member

@loicmathieu it's similar to JPA's query.setFirstResult(startIdx).setMaxResult(endIdx), correct?
I'd change your implementation to not use Page.of.

@FroMage happy with the API range(startIndex, endIndex)?

@loicmathieu
Copy link
Contributor Author

@emmanuelbernard yes, and I'm not happy with my implementation, but currently PanacheQuery implementation always use pagination.

I can think of two options:

  • Using directly the indexes when making a range query and throwing an error if a range is use then a page (to avoid inconsistent usage)
  • Using directly the indexes when making a range query and returning a different interface after using a range to avoid mismatch between range an pagination

Because, if we directly use the index, we will skip the pagination stuff and if the user use it after a range query it will be inconsistent.

@FroMage
Copy link
Member

FroMage commented Nov 18, 2019

Well, PanacheQuery has plenty of page methods that can't make any sense if we set a range like range(7, 19) which does not have a corresponding page index/size. What do we do about this?

@FroMage
Copy link
Member

FroMage commented Nov 18, 2019

Also, can I have some example use-cases of ranges? I've never seen APIs that use non-pageable ranges before, so I hope this is not one of those "hey it would be cool" features with no actual use-case ;)

@loicmathieu
Copy link
Contributor Author

In my previous Job, we implement pagination for all our API (more than 30 APIs) using HTTP range: https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests

So the user will include a ?range=1-10 query parameter and we will return the range header to him (including HTTP link header for HATEOAS navigation between pages).

I would say, range are as common as pagination but I'm not sure.
You can also see an example of range query inside OCTO API refcard that is a reference API design : https://blog.octo.com/wp-content/uploads/2014/12/OCTO-Refcard_API_Design_EN_3.0.pdf

@loicmathieu
Copy link
Contributor Author

@FroMage WDYT of this ?

@FroMage
Copy link
Member

FroMage commented Nov 21, 2019

Well, how can we do ranges if the paging part of Query does not support it? It's not clear to me.

Also, fine: on the surface it looks fine, and you've used it before. I still think we should wait until other users request it, because so far you're the only user so perhaps you're an outlier?

@emmanuelbernard
Copy link
Member

@FroMage try and implement range on the paging API of Panache, you'll see you can't naturally build one on the other. You can off the JPA API which does support range.

@emmanuelbernard
Copy link
Member

@FroMage try and implement range on the paging API of Panache, you'll see you can't naturally build one on the other. You can off the JPA API which does support range.

That was a reply to " how can we do ranges if the paging part of Query does not support it? It's not clear to me."

@FroMage
Copy link
Member

FroMage commented Nov 21, 2019

Yeah, I know they don't map, which is why I asked what the paging methods should do if we're ranging.

You can off the JPA API which does support range

Sorry I can't parse that :(

@loicmathieu
Copy link
Contributor Author

There is three popular ways to limit the list to a certain number of items and allow to fetch them:

  • Pagination with pageNumber and pageCount
  • Cursors: stateful but surpisingly still used accross various big player's APIs
  • Offset based: with limit and offset (or startIndex lastIndex)

The first is possible with the current API.
The second should also be possible but as it's stateful I don't realy like it
The third is what I asked for.

If we don't want to provides a way to do it for a user, I may ask for a way to access the underlying Query before triggering, so adding a method PanacheQuery.getQuery():Query.

So the user can do what he want with the Query, with this we don't need to implements various ne features on PanacheQuery that are wanted to have more customization on the query before triggering it (like the PanacheQuery.withHint() new method that is asked to be able to cache queries).
I don't like it, but it's better than not having a way to specify offset/limit on a PanacheQuery.

Having a way to use Panache direclty with a Query object instead if creating it will also works. This is also something requested in an existing issue. And it's something we do for PanacheMongo: Person.list(Query).

@FroMage
Copy link
Member

FroMage commented Nov 22, 2019

That's not the issue. The issue is that if we allow for this (and again, I'm recommending to wait for a few more users asking for it), what will the behaviour of the current paging methods be? Methods like page() and pageCount()? Should they throw once a non-pageable range method has been called? Should we have two sorts of queries PageableQuery and RangeableQuery to avoid throwing in some methods?

@loicmathieu
Copy link
Contributor Author

@FroMage yes, when we use range we should return a different interface: RangeableQuery or Findable the latest will be re-usable if we found more usecase (or re-usable accross Panache implementation ... of I know you're against it :) )

@loicmathieu
Copy link
Contributor Author

@FroMage I updated my proposal to return a Findable when using the range method. I also review how range and page handles the offsert/limit.

You can see the changes in my improvements branch: master...loicmathieu:feat/panache-improvements

@marceloverdijk
Copy link
Contributor

I would also be interested in a standard offset / limit query.

@pilhuhn
Copy link
Contributor

pilhuhn commented Feb 19, 2020

I also have a use case for this, where the current paging approach makes it hard to implement.

@josejulio
Copy link
Contributor

Also interested in this usage @loicmathieu

@loicmathieu
Copy link
Contributor Author

@emmanuelbernard @FroMage I count 5 people asking for it now, should I propose an implementation ?

@emmanuelbernard
Copy link
Member

yes I guess so

@loicmathieu
Copy link
Contributor Author

Cool, I add it to my TODO list ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants