mickhansen / graphql-sequelize

GraphQL & Relay for MySQL & Postgres via Sequelize
MIT License
1.9k stars 172 forks source link

Relay Connections with Dataloader #702

Closed intellix closed 3 years ago

intellix commented 3 years ago

So today I found that we're deeply nesting connections and causing havok:

users {
  edge {
    node {
      id
      posts {
        edges {
          node {
            ...Post
          }
        }
      }
    }
  }
}

In our API we create relay connections using the helper library provided here like:

export const UserConnection = relay.createConnectionResolver({
  orderBy: 'UserOrderBy',
  target: () => User,
});

export const PostConnection = relay.createConnectionResolver({
  orderBy: 'PostOrderBy',
  target: () => Post,
  before: (findOptions, args, context, info) => {
    if (info.source) {
      switch (info.source.constructor) {
        case User:
          findOptions.where.userId = info.source.id;
          break;
      }
    }
  },
});

And we use them like so:

export const UserResolver = {
  User: {
    id: globalIdField(User.name),
    posts: PostConnection.resolveConnection,
  }
  Query: {
    users: UserConnection.resolverConnection,
  }
},

We create our dataloader on a request basis like so:

const { resolver } = require('graphql-sequelize');

return new ApolloServer({
  context({ req, connection }) {
    // Magic swap for request-based dataloader
    resolver.contextToOptions = {
      dataloaderContext: [EXPECTED_OPTIONS_KEY],
    };
  }
})

The dataloader works fine, but we can't seem to get it to work for Relay connections and we end up with an insane amount of queries.

Should we be doing something different or providing the dataloader context somehow? I can see inside the relay.js lib that it just uses require('./resolver') but when logging that, the contextToOptions is an empty object. I've even tried manually attaching the context before using the required resolver but still had no luck.

I can do a PR afterwards when I figure it out to add it to the Relay README.md but right now I'm completely bamboozled.

mickhansen commented 3 years ago

The dataloader might only work for actual relation calls, i don't believe it's smart enough to merge similar where queries at this time.

It's been a while since i've used that specific code, but IIRC you want to modify the target callback to return the proper relationship based on the input:

https://github.com/mickhansen/graphql-sequelize/blob/master/src/relay.js#L203 https://github.com/mickhansen/graphql-sequelize/blob/master/src/resolver.js#L46

targetMaybeThunk should be receiving (source, args, context, info) which should let you apply almost the same logic you currently do in before and return the stored relationship reference (something like return User.Posts if you have User.Posts = User.hasMany(Post)

mickhansen commented 3 years ago

As a general debug technique for batching i generally write some unit/integration tests and add context.logging = console.log and roughly count the number of queries.

intellix commented 3 years ago

Awesome, it actually works:

target: (findOptions, args, context, info) => {
  switch (info.parentType.name) {
    case User.name:
      return User.associations.posts;
    default:
      return Post;
  }
},

Thanks for that, I'll send a PR with an example of it in after I verify it working 100% and my above code is robust.

When I add a findOptions.limit it create a massive union which I guess is expected as I'm asking for a limit of 250 for each of the individual groups, which gets quite unperformant but don't see any way around that

stale[bot] commented 3 years ago

This issue 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.