mickhansen / graphql-sequelize

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

orderBy aggregate relation SUM #573

Closed intellix closed 6 years ago

intellix commented 6 years ago

User hasMany Wallets. I'd like to orderBy SUM of user.wallets within an admin section.

First I created an orderBy:

orderBy: new GraphQLEnumType({
  name: 'UserOrderBy',
  values: {
    ID: { value: ['id', 'ASC'] },
    BALANCE: { value: ['wallets.balance', 'ASC'] },
  },
}),

Then I alter the query in a before hook to add the aggregation:

// Set to raw to remove the automatic wallets.id breaking aggregation
findOptions.raw = true;
findOptions.group = 'User.id';
findOptions.include = [
  {
    association: User.associations.wallets,
    attributes: [
      [sequelize.fn('SUM', sequelize.col('wallets.amount')), 'balance'],
    ],
  },
];

First issue I've identified is that the GROUP BY appears within the INNER query rather than OUTER:

SELECT 
    `User`.*, SUM(`wallets`.`amount`) AS `wallets.balance`

FROM
    (SELECT 
        `User`.`id`,
        `User`.`name`,
        `User`.`createdAt`,
        `User`.`updatedAt`,
        `User`.`deletedAt`
    FROM
        `User` AS `User`
    WHERE
        (`User`.`deletedAt` > '2018-03-11 19:53:57' OR `User`.`deletedAt` IS NULL)
    GROUP BY `User`.`id`    <<<<<<<<<<<<<<<<<<<<<<
    ORDER BY `User`.`id` ASC
    LIMIT 250) AS `User`

LEFT OUTER JOIN `Wallet` AS `wallets` ON `User`.`id` = `wallets`.`userId`

ORDER BY `User`.`id` ASC;

Second thing is if I use the orderBy: users(orderBy: BALANCE) { the wallets.balance gives an error of: Cannot read property 'type' of undefined. Ideally I would just name the column balance but it gets prefixed with wallets automatically.

intellix commented 6 years ago

The below works in standard sequelize so I'm guessing the above issues are caused by the SELECT getting wrapped along with all of the findOptions and not playing well with includes:

User.findAll({
  raw: true,
  group: 'User.id',
  include: [
    {
      association: User.associations.wallets,
      attributes: [
        [sequelize.fn('SUM', sequelize.col('wallets.amount')), 'balance'],
      ],
    },
  ],
});
mickhansen commented 6 years ago

Your standard sequelize call doesn't have a limit. A limit changes things substantially when using includes.

intellix commented 6 years ago

Ah, missed that out. I'm forcing a hard limit on all queries of 250 using findOptions elsewhere. That goes inside the inner select

mickhansen commented 6 years ago

Limits don't work like you expect them to when you have a cartesian product, that's why the limit triggers a subquery which can mess up logic like that

intellix commented 6 years ago

ok, removed the limit, the query looks fine. Am now getting item.get is not a function which I'm guessing is caused by the raw and I'd need to re-hydrate afterwards.

LIMIT seemingly works if I add it to the generated query afterwards. (4 wallets, 2 users and LIMIT 1 returns 1 row) but am not sure about the other issues it can cause.

I'm using this before hook throughout to stop queries of 1 billion results if a first/after isn't provided:

export function defaultLimit(findOptions, args, context) {
  if (!findOptions.limit) {
    findOptions.limit = 250;
  }

  return findOptions;
}

Guess I'll have to use an after hook to apply a limit post-query as well to get this particular case working

mickhansen commented 6 years ago

Generally you don't want to mix aggregation and pagination queries, it's just no fun.

intellix commented 6 years ago

It's just that there's no issues with the query I'm trying to do. I know it should work but I just can't mould sequelize to write a valid query :(

Two options I see:

SELECT `User`.*, SUM(`Wallet`.`amount`) as `balance`
FROM `User`
INNER JOIN `Wallet` ON `User`.`id` = `Wallet`.`userId`
GROUP BY `User`.`id`
ORDER BY `balance` DESC
LIMIT ?
intellix commented 6 years ago

For other people looking for this. I'm lurking inside the Slack chat and noticed someone else wanting to do this for a common case: User hasMany Posts and orderBy COUNT(User.Post). Recommendation that I saw was to use a subquery as it's the cleanest.

This code was posted and reported to work, so I'll give it a go tonight:

options.order = [
[sequelize.literal('(SELECT COUNT("comments"."id") FROM "comments" WHERE "comments"."postId" = "post"."id")'), 'DESC']
];

Hopefully someone else finds this useful for the future. Would have been nice for it to be posted back here :)

mickhansen commented 6 years ago

I might have posted the recommendation on Slack, however it's entirely my personal opinion - I find them cleaner and i try to leave includes out of my GraphQL work.

intellix commented 6 years ago

Great, this works :) Didn't think to order by a subquery:

orderBy: new GraphQLEnumType({
  name: 'UserOrderBy',
  values: {
    ID: { value: ['id', 'ASC'] },
    BALANCE: {
      value: [sequelize.literal('(SELECT SUM(`Wallet`.`amount`) FROM `Wallet` WHERE `Wallet`.`userId` = `User`.`id`)'), 'ASC'],
    },
  },
}),