mickhansen / graphql-sequelize

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

Possible bug transitive dependencies dataloader #692

Closed mschipperheyn closed 4 years ago

mschipperheyn commented 4 years ago

I've followed the suggestions in https://github.com/mickhansen/graphql-sequelize/issues/670. But I'm still having an issues where the n+1 appears in some scenarios and those scenarios sometimes work normally.

It's really hard to debug. What would help hugely is a debug implementation where you could active DEBUG=dataloader and see dataloader hits and misses.

Most of my issues occur in many to many through and hasOne scenarios, although I never use through and prefer instead to use an explicit intermediary class with belongsTo connections to the tables it connects. Normal hasMany scenarios seem ok, but when I have a hasMany to a table that only contains an id to the table we're actually interested in, I often see n+1 style

Social__Post: {
         photos: resolver(() => models.SocialPost.associations.photos),
},
Social__PostPhoto: {
    photo: resolver(() => models.SocialPostPhoto.associations.photo),
},
SELECT `id`, `createdAt`, `updatedAt`, `postId`, `photoId` FROM `SocialPostPhotos` AS `SocialPostPhoto` WHERE `SocialPostPhoto`.`postId` = 'cc87181e-ec5d-4d78-b210-8b1500ec2407' ORDER BY `SocialPostPhoto`.`id` ASC;

for each post in an array. Subsequently, I see

SELECT `id`, `label`, `name`, `alt`, `width`, `height`, `isDefault`, `folder`, `createdAt`, `updatedAt` FROM `Photos` AS `Photo` WHERE `Photo`.`id` = 'd9ff65b3-66e8-4b26-98dc-9934439746d4';

However, when I customize the resolver like so:

Social__Post: {
    photos: (parent, args, context, info) => resolver(() => models.SocialPost.associations.photos, {
        before: findOptions => {
            findOptions[context.dataloader] = context[context.dataloader];
            return findOptions;
        },
    })(parent, args, context, info),
}
Social__PostPhoto: {
    photo: (parent, args, context, info) => resolver(() => models.SocialPostPhoto.associations.photo, {
        before: findOptions => {
            findOptions[context.dataloader] = context[context.dataloader];
            return findOptions;
        },
    })(parent, args, context, info),
},

it works as expected.

I created a little resolver wrapper for myself to help with this. But it should not be necessary to pass the dataloader to achieve transitive dataloading. It may be something that's wrong with my setup, but it looks like a bug.

mschipperheyn commented 4 years ago

The workaround above actually doesn't "work". What is really happening is that transitive dataloading sometimes works and most of the time doesn't. The confusing part is that it seems to start working when I start console logging stuff to figure out why it doesn't work. It must be some kind of order of loading issue.

mickhansen commented 4 years ago

@mschipperheyn Hmm, i don't have any immediate ideas to what might be happening here.

mschipperheyn commented 4 years ago

@mickhansen Yeah, I realize not very actionable. Happy New Year!

stale[bot] commented 4 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.