eclipse / microprofile-graphql

microprofile-graphql
Apache License 2.0
98 stars 40 forks source link

Pagination #29

Open phillip-kruger opened 4 years ago

phillip-kruger commented 4 years ago

Add Spec, test setup, tests for pagination

andymc12 commented 4 years ago

I've been trying to think how this might look in the user's application code. The simplest forms of pagination could probably already work with what we have so far - something like:

@Query
public List<Hero> allHeroes(@DefaultValue("-1") @Argument("first") int first,
                            @DefaultValue("0") @Argument("offset") int offset) {
    List<Hero> allHeroes = HeroDB.getAllHeroes();
    if (first < 0) {
        return allHeroes;
    }
    return allHeroes.subList(offset, offset + first);
}

This might not be the best example, since it is getting all heroes from the database even if it will only return a subset, but it should work. Whatever the API ends up looking like, I think that the pagination data (first, offset, cursor, etc.) should be passed to the application method so that the application code can optimize for it (i.e. only lookup two rows in the database, etc.).

phillip-kruger commented 4 years ago

Yip, this is kind of how I have done this way back. See https://github.com/phillip-kruger/membership/blob/master/src/main/java/com/github/phillipkruger/membership/graphql/MembershipGraphQLApi.java

I called it first and skip (not offset) based on this from Bojan: https://www.howtographql.com/graphql-java/10-pagination/

Also, this means that certain argument names is reserved ? Right ?

andymc12 commented 4 years ago

It sounds like pagination is not covered explicitly by the GraphQL spec, but rather it is up to application developers to determine how to do it. I read this article ( https://graphql.org/learn/pagination/ ) which used the terms "first" and "offset" - but they actually preferred a more complex "cursor" style approach.

So, in the 1.0 release of MP GraphQL, users could try any of these approaches by explicitly coding it themselves. I'm wondering if there might be a way we could simplify the code to some degree - maybe something like:

@GraphQLApi
public class MyApi {

    @Inject
    PaginationSupport pagination; // new MP GraphQL interface - injects request-scoped dynamic proxy

    @Query(paginationStrategy=FIRST_AND_OFFSET) // tells schema generator to add optional "first" and "offset" fields
    public List<Widget> allWidgets() {
        return getWidgetsFromDB(pagination.getIndex(), pagination.getRange());
    }

    @Query // no paginationStrategy, so pagination fields are generated
    public List<Widget> fixedLengthWidgets() {
        return getWidgetsFromDB(0, -1 /* or Integer.MAX_INT, etc. */);
    }
}

Not fully baked, but hopefully it makes sense... Something like that might be able to cut down on the number of parameters for each query. If we do it right, then maybe the user could use their own semantics (i.e. be able to choose between "skip" or "offset", etc.).

WDYT?

phillip-kruger commented 4 years ago

I like that. It keeps the parameters clean. Nice.

kaqqao commented 4 years ago

Sorry if I'm being dumb, but how do you know which PaginationSupport to inject where? 🤔 E.g. imagine a query like:

{
    heroes(offset: 10, limit: 10) {
        powers(offset: 5, limit: 5)
    }
}

What would the injected PaginationSupport provide? Inside heroes resolver, it should be 10/10 and 5/5 inside the powers resolver... So it either requires some serious magic or I'm not getting it right 😶

Is it maybe enough/simpler to provide one PaginationSupport per resolver, e.g.

@Query
public List<Hero> heroes(PaginationSupport pagination) {
    return getHeroesFromDB(pagination.getIndex(), pagination.getRange());
}

This has a potential nice side benefit of directly communicating the pagination strategy by the type of the injected object. E.g.

heroes(OffsetPagination pagination) //Use limit/offset
heroes(CursorPagination pagination) //Use the last seen cursor + limit
andymc12 commented 4 years ago

@kaqqao Good idea. I like it. One thing to add to your example (sorry if it is obvious) would be that the child query must be a separate query method as opposed to a property of Hero.

For example:

@Query
public List<Power> powers(PaginationSupport pagination, @Source Hero hero) {
    return getPowersForHeroFromDB(hero)
        .subList(pagination.getCursor(), pagination.getCursor() + pagination.getLimit());
}
phillip-kruger commented 4 years ago

How about this:

To indicate that something should be pageable (so add the page fields to the schema)

@Inject Page page;

@Query
public List<@Pageable SuperHero> allHeroes() {
    // page.getCursor();
    // page.getLimit();
}

We add that to the 'Collection' as that is the thing that is pageable.

in a more 'complex' case, like where the Superhero has a collection of something via a @Source

@Query
public List<@Pageable Power> powers(Page page, @Source SuperHero hero) {
    // page.getCursor();
    // page.getLimit();
}

If the list is a property on the pojo, the page operation would anyway not be on the get operation, but rather on the underlying data query right ? So maybe then we can still mark it as pageable as above (List<@Pageable String> listOfSomething) then we can inject the Page somewhere and get the page details from there. We would need to be able to tell the Page what object we are asking for, maybe:

page.getCursor(Class c, fieldName) so using the example above: page.getCursor(SuperHero.class, "listOfSomething")

(Just thinking out loud)

In any of the cases, if the parameters is not set on input , we get default values from config.

phillip-kruger commented 4 years ago

Or we can allow a user the give the @Pageable some name or reference:

List<@Pageable("somethingList") String> listOfSomething

Then I can always ask the page for the Page context for a certain case:

page.getCursor("somethingList")

This can then apply to all cases, on a Query, on a Source and on a field in a POJO.

I can then also inject the page anywhere to actually do better paging, example write better SQL.

marceloverdijk commented 4 years ago

I think for pagination there are multiple use cases: cursor based pagination, offset based pagination or page based pagination.

For Relay cursor based pagination there is a spec, but for the others it might be even different per use. E.g. when using offset based pagination, some might what to use skip or offset depending how they want to name it their api.

I was yesterday thinking of something else. In Spring Data related REST projects I always used Spring Data JPA Web Support which offers in REST controllers:

  @RequestMapping
  public List<User> getUsers(Pageable pageable) {

    var users = userRepository.findAll(pageable);
    return "users";
  }

Spring Data uses HandlerMethodArgumentResolvers for this, but maybe JAX-RS has something similar?

If SmallRye could also support something like GraphQLMethodArgumentResolvers you could do something like:

    @Query("continents")
    @Description("Get a page of continents.")
    public @NonNull ContinentPageType getContinents(
            Pageable pageable) {

This Pageable is not an input type (with nested fields) but its fields would be flattened as top level arguments, just as one would do:

    @Query("continents")
    @Description("Get a page of continents.")
    public @NonNull ContinentPageType getContinents(
            @Name("page")
            Integer page,
            @Name("size")
            Integer size) {

Just like Spring's HandlerMethodArgumentResolver interface the implementing class could also support defaults like:

    @Query("continents")
    @Description("Get a page of continents.")
    public @NonNull ContinentPageType getContinents(
            @PageableDefault(page = 0, size = 10)
            Pageable pageable) {

With such a HandlerMethodArgumentResolver abstraction you could support many use case: the various pagination use case, but e.g. also sorting or even filtering.

And the good thing is users would be able plugin their own HandlerMethodArgumentResolver implementation.

I'm not saying it's easy and off course the mechanism should be able to define the GraphQL annotation like @Name, @Description etc.

Such a Pageable page based pagination class could look like:

public interface Pageable {

    @Name("page")
    @DefaultValue("0")
    int getPageNumber();

    @Name("size")
    @DefaultValue("10")
    int getPageSize();

And the PageableHandlerMethodArgumentResolver implementation would be responsible for getting the arguments from the GraphQL request.

Instead of just a Pageable I could imagine more concrete CursorPageable, OffsetPageable,PagePageable` ones. PS: I'm not saying MP GraphQL should provide all these implementations.

phillip-kruger commented 3 years ago

Also see https://relay.dev/graphql/connections.htm and https://graphql.org/learn/pagination/

kaqqao commented 3 years ago

I'm basically on the same page with @marceloverdijk on this one. I think that approach requires very little magic (no proxy objects, shared context, or anything) and is fully explicit in its intention and usage. The only downsides I see is that it requires @Source in order to turn nested queries pageable, but that's also what makes it explicit and local - which is IMHO always worth it.

arimeyer commented 1 year ago

What is the status of pagination support currently? I didn't see anything in the SmallRye guides about it, so I'm assuming it's not there yet. I'm currently experimenting with GitHub's API (cursor-based), and performing pagination manually is very onerous.

t1 commented 1 year ago

We didn't come to any conclusion. Maybe you have a suggestion on how your use-case could be made easier?

arimeyer commented 1 year ago

Thank you, Rudiger. We are using Quarkus to execute queries via the DynamicGraphQLClient. The GitHub API doesn't support offsets, so we have to do the following:

  1. Execute an initial query without an "after" clause
  2. Extract pageInfo.hasNextPage from the response
  3. If that's true, take the pageInfo.endCursor and insert it into the query following an "after: " token
  4. Repeat the process to the end, appending/modifying the relevant JSON fragment appropriately to construct the full response

It's ugly, repetitive code that one would hope could be hidden behind a useful abstraction. Thanks

t1 commented 1 year ago

Yes, sounds like it would be better to hide that somehow. But it's quite GitHub specific; there is just no standardised way to do pagination in GraphQL. For my own part, I'm totally into the typesafe client, which works for all my current requirements, so I haven't worked with the dynamic client, yet. I don't have a good suggestion ready for you. Maybe you can find a appropriate abstraction in your client adapter/gateway code, so you can reuse that part? If that works well over many of your client use-cases, we might merge it in here, given that the pagination style used by GitHub does not turn out to be too specific over time.

arimeyer commented 1 year ago

Thanks, Rudiger. I will look at generalizing this over the next couple months and see what I can contribute. I like the dynamic client better at this point as I can shift between a tool like GraphiQL or Postman and keep the queries in sync, rather than have to flip between different representations. That said, it has it's drawbacks. ;-)