Mando75 / typeorm-graphql-loader

A query builder to easily resolve nested fields and relations for TypeORM-based GraphQL servers
https://gql-loader.bmuller.net
MIT License
57 stars 12 forks source link

Pagination query not selecting correct fields #6

Closed DomHynes closed 4 years ago

DomHynes commented 4 years ago

When using LoadManyPaginated, it passes in the queried fields from the root of the query. This works if you are only returning an array of entities, but this means you cannot return pagination metadata.

This appears to work, but only if you don't query for anything more than the primary column, which is fetched by default.

Not really sure what a good solution is here. The LoadManyPaginated function could accept a string as an option that specifies which field to drill into to find the selections for the entity?

I've added a test that reproduces the issue here.

Mando75 commented 4 years ago

Thanks for providing the test.

To be honest, this pagination method is kind of half-baked at the moment and targeted for how I did pagination in my personal projects. I'm not really happy with it. As a work around, the loader accepts a GraphQLResolveInfo parameter. but it also exposes a type FeedNodeInfo which you can use. This means that you can provide the loadManyPaginated method with a single node of the resolve info, and it can traverse the request from there. I created a PaginationHelper class that I've been using personally, but now that it seems more people are starting to find this package, I should probably come up with a better method :)

If you would like to see my pagination/search helper, I posted it in a gist with an example resolver : https://gist.github.com/Mando75/a94f295bca421aff4db34af5d234018b It is pretty specific to how I do pagination, which is just to load records for an infinite scroll, but you could change it to work with backwards pagination pretty easily. I included my GraphQL types and an example resolver that implements paginated search results as an example. Hopefully that helps.

Pretty much, I can pass the getFeedNodeInfo method of the helper the GraphQLResolveInfo parameter, and give it a specific field as a string (like you mentioned in your comment). It then extracts the sub node and sends it to the loader.

I'm probably not going to fix this right away, mostly because I'm currently reworking the loader API to be cleaner via function chaining, similar to the TypeORM query builder API. I will try to provide a better method of pagination in that release probably through something like the relay spec. Hopefully this pagination helper gist helps you out in the meantime.

I'll also leave this issue open for anyone else who may be having trouble with it. Thanks for bringing it to my attention.

DomHynes commented 4 years ago

Cool, thanks for the response! I think I'll implement that helper in my project, it looks straightforward.

Mando75 commented 4 years ago

I just noticed that apparently I forgot to actually post my comments with the GraphQL type defs and example resolver. Gist has been updated with that now. Sorry about that :)

Mando75 commented 4 years ago

Just an update on this. I finished re-implementing pagination in the new version I'm working on. The dataloader will now be used like this.

type PaginatedPost {
  hasMore: Boolean!
  offset: Int!
  posts: Post[]
}

type Post {
   id: ID!
   title: string
  # etc
}
  async paginatedPosts(
    rootValue: any,
    args: any,
    context: { loader: GraphQLDatabaseLoader },
    info: GraphQLResolveInfo
  ): PaginatedPost {
    const [posts, totalCount] = await context.loader
      .loadEntity(Post)
      .where(args.where)
     /* we want the loader to only try and resolve the "posts" fields
         passing that to the info method make sure the loader tries to 
        resolve the correct selection of fields */
      .info(info, "posts")
      .paginate(args.pagination)
      .loadPaginated();
    // Note: this is just an example of a simple pagination system that does
    // not check if the end of the list is reached when calculating the next offset
    // It is advised that a more robust/standardized implementation is used.
    const { offset, limit } = args.pagination;
    return {
      hasMore: offset + limit < totalCount,
      offset: offset + limit,
      posts
    };

The info method can take the name of a field in the info object and use that sub-selection to resolve the query. This is essentially what my pagination helper does that I linked to above, but instead it is just baked into the library. I also checked and this passes the failing test example you provided. The benefit for doing it this way is that you can now select a specific field to resolve on any query, not just paginated queries.

This fix should be available in the next release which should be wrapping up shortly.

Mando75 commented 4 years ago

The fix I outlined in the comment above is now available in the latest package version.