Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sequelize findAndCountAll returns wrong count when association models included #9481

Open
tumdav opened this issue May 24, 2018 · 19 comments
Open
Labels
topic: count For issues and PRs. Things that involve counting such as the `count` and the `findAndCountAll` meth. type: bug type: docs For issues and PRs. Things related to documentation, such as changes in the manuals / API reference.

Comments

@tumdav
Copy link

tumdav commented May 24, 2018

What are you doing?

I have Post model.
Post model has attachment(oneToMany),users(oneToMany),
I'm trying to fetch posts with attachments using sequelize findAndCountAll

// POST MODEL
module.exports = (sequelize, DataTypes) => {
     const Post = sequelize.define(
        'Post',
        {
            id: {
                primaryKey: true,
                type: DataTypes.UUID
            },
            title: DataTypes.STRING(50)
        },
        {
            tableName: 'post',
            timestamps: true
        }
    );

    Post.associate = models => {
        Post.hasMany(models.PostAttachment, {
            as: 'attachment',
            foreignKey: 'postId'
        });
    };

    return Post;
};
// POST ATTACHMENT
module.exports = (sequelize, DataTypes) => {
    const PostAttachment = sequelize.define(
        'PostAttachment',
        {
            id: {
                primaryKey: true,
                type: DataTypes.UUID
            },
            key: DataTypes.STRING
        },
        {
            tableName: 'postAttachment',
            timestamps: true
        }
    );

    PostAttachment.associate = models => {
        PostAttachment.belongsTo(models.Feed, {
            as: 'post',
            foreignKey: 'postId'
        });
    };

    return PostAttachment;
};

POST TABLE

id title createdAt updatedAt
f8d68fc0-48b1-4e90-af73-c9a4dc577461 Title1 2018-05-23 16:47:09.228000 +00:00 2018-05-23 16:47:09.228000 +00:00
01a528d2-8696-465f-8739-2b32bc52ceca Title2 2018-05-23 16:47:09.228000 +00:00 2018-05-23 16:47:09.228000 +00:00

POST ATTACHMENT TABLE

id key postId createdAt updatedAt
410819f6-d1c4-44f4-9a52-edbac765e714 Key1 f8d68fc0-48b1-4e90-af73-c9a4dc577461 2018-05-23 16:47:09.228000 +00:00 2018-05-23 16:47:09.228000 +00:00
754ca095-4967-4472-a0f4-1a8e2d761a24 Key2 f8d68fc0-48b1-4e90-af73-c9a4dc577461 2018-05-23 16:47:09.228000 +00:00 2018-05-23 16:47:09.228000 +00:00

SEQUELIZE QUERY

const posts = Post.findAndCountAll({
    include: ['attachment', 'users', 'x count of models']
});

 return {
    "posts": posts.rows,
    "total": posts.count
};

What do you expect to happen?

I'm expecting get 2 posts and total sum 2

{
"posts": [{...},{...}],
"total": 2
}

What is actually happening?

I'm getting 2 posts and total sum 4

{
"posts": [{...},{...}],
"total": 4
}

Dialect: postgres
Dialect version: XXX
Database version: PostgreSQL 10.3 on x86_64-apple-darwin17.3.0, compiled by Apple LLVM version 9.0.0 (clang-900.0.39.2), 64-bit
Sequelize version: ^4.37.8

@ErnestoSalazar
Copy link

ErnestoSalazar commented May 31, 2018

@tumdav try adding distinct:true to the findAndCountAll passed object that should do the trick, i had the same problem few days ago and that solve it

const posts = Post.findAndCountAll({
    include: ['attachment', 'users', 'x count of models'],
    distinct:true
});

@kshateesh
Copy link

I've the same issue adding distinct:true did not solve the problem for me. I'm using MSSQL dialect

@paulmowat
Copy link
Contributor

@tumdav - I'm happy to take a look at this but can you provide a complete set of your models your using for this example please. Seems that your missing users above.

@JamesMGreene
Copy link

JamesMGreene commented Mar 15, 2019

Give this workaround a try:

// Add a permanent global hook to prevent unknowingly hitting this Sequelize bug:
//   https://github.com/sequelize/sequelize/issues/10557
sequelize.addHook('beforeCount', function (options) {
  if (this._scope.include && this._scope.include.length > 0) {
    options.distinct = true
    options.col = this._scope.col || options.col || `"${this.options.name.singular}".id`
  }

  if (options.include && options.include.length > 0) {
    options.include = null
  }
})

@sirjaag
Copy link

sirjaag commented Oct 6, 2019

@JamesMGreene I've had this problem for a long time and this worked beautifully (even though I don't quite understand how...). So thanks a million !! 😄

@raviqqe
Copy link

raviqqe commented Oct 29, 2019

The bug is introduced in this PR to fix another issue. Currently the findAndCountAll method does not work as documented because the documentation is of the old implementation.

The previous version of this issue was fixed and closed a while ago.

@papb papb added status: awaiting investigation topic: count For issues and PRs. Things that involve counting such as the `count` and the `findAndCountAll` meth. type: docs For issues and PRs. Things related to documentation, such as changes in the manuals / API reference. labels Nov 2, 2019
thurt added a commit to KenzieAcademy/kwitter-api that referenced this issue Dec 9, 2019
this bug is apparently due to sequelize findAndCountAll method which
adds extra count in cases where there are other models included (in this
case Likes model is included in the getMessages result). Adding
distinct: true appears to fix the bug. see sequelize/sequelize#9481
@shahzadthathal
Copy link

shahzadthathal commented Dec 18, 2020

Give this workaround a try:

// Add a permanent global hook to prevent unknowingly hitting this Sequelize bug:
//   https://github.com/sequelize/sequelize/issues/10557
sequelize.addHook('beforeCount', function (options) {
  if (this._scope.include && this._scope.include.length > 0) {
    options.distinct = true
    options.col = this._scope.col || options.col || `"${this.options.name.singular}".id`
  }

  if (options.include && options.include.length > 0) {
    options.include = null
  }
})

I added this hook inside models/index.js and it works 👍

//Fix the wrong count issue in findAndCountAll()
sequelize.addHook('beforeCount', function (options) {
  if (this._scope.include && this._scope.include.length > 0) {
    options.distinct = true
    options.col = this._scope.col || options.col || `"${this.options.name.singular}".id`
  }

  if (options.include && options.include.length > 0) {
    options.include = null
  }
})
db.sequelize = sequelize;
db.Sequelize = Sequelize;

db.users = require("./user.js")(sequelize, Sequelize);

@mikemerritt
Copy link

I've run into this issue numerous times, and today encountered it again. 3+ years and it hasn't been fixed? There is obviously a workaround available posted by @JamesMGreene , thanks for that, but it shouldn't be required for such a core piece of functionality of Sequelize to work correctly. This example is even in the official docs and it doesn't even work!

@talhabalaj
Copy link

The complex your queries get, the workaround stops working. For example, If I want to pass where argument in the include. For example

const data = User.findAndCountAll({ include: [{as: 'order', model: Order, where: { orderName: 'laptop' } }] })

Now the count will be wrong with the workaround by @JamesMGreene (which was genius, tho). This needs to be properly fixed

@ibadnabihashmi
Copy link

the issue seems to be fixed, adding distinct: true works

@mprevdelta
Copy link

At first I thought this was a bug but it appears it's just critical to read the docs. When using findAndCountAll(), make sure to pass the same params as you would to a call to count(). Namely: 'col' and 'distinct'.

I think most users here will want to use the root object id as the 'col' value, and 'distinct' set to true.

@faraz9x
Copy link

faraz9x commented Jan 24, 2022

distinct: true works but remember, your query must have an @Index column

@ilanl
Copy link

ilanl commented Jul 12, 2022

Thank you! distinct: true was the answer I was looking for.

@bhojakRahul13
Copy link

dyaskur

Try to used separate: true in association if you count is wrong when you used findAndCountAll with associate

supercrytoking pushed a commit to supercrytoking/Devcash-Bounty-Platform that referenced this issue Nov 27, 2022
@tfmertz
Copy link

tfmertz commented Dec 22, 2022

Adding separate: true to the include seemed to work for my use case. (The docs are hard to link to, but go here and search for separate). The api docs add this note about what this option does:

If true, runs a separate query to fetch the associated instances, only supported for hasMany associations

await findAndCountAll({
  where: { ... }
  include: [{ model: Asset, separate: true }]
});

Worked for me with this issue in Sequelize v6.19.2. Granted it runs another query for the associations, but the counts seem to be right (at least for my query use-case).

Good luck.

EDIT: Ran into a weird bug where adding separate: true wasn't working with one of our double nested associations, but it turned out the middle association wasn't asking for id as one of its attributes, so the separate query was looking for all NULLs 😅

@FahadAshraf26
Copy link

FahadAshraf26 commented Jan 15, 2023

added beforeCount hook inside the DB connection file this snippet will fix the wrong count issue in findAndCountAll()

sequelize.addHook("beforeCount", function (options) {
  const includeAvailable = options.find((item) => item["where"]);
  if (this._scope.include && this._scope.include.length > 0) {
    options.distinct = true;
    options.col =
      this._scope.col || options.col || `"${this.options.name.singular}".id`;
  }
  const whereSymbols = includeAvailable
    ? Object.getOwnPropertySymbols(includeAvailable["where"]).length
    : 0;
  const whereString = includeAvailable
    ? JSON.stringify(includeAvailable["where"]).length
    : 0;
  const whereObject = whereSymbols > 0 ? whereSymbols : whereString > 2 ? 1 : 0;
  if (whereObject === 0 && options.include && options.include.length > 0) {
    options.include = null;
  }
});

@LarMaySee
Copy link

@tumdav try adding distinct:true to the findAndCountAll passed object that should do the trick, i had the same problem few days ago and that solve it

const posts = Post.findAndCountAll({
    include: ['attachment', 'users', 'x count of models'],
    distinct:true
});

working for mysql

@jegangits
Copy link

@tumdav try adding distinct:true to the findAndCountAll passed object that should do the trick, i had the same problem few days ago and that solve it

const posts = Post.findAndCountAll({
    include: ['attachment', 'users', 'x count of models'],
    distinct:true
});

thank you bro

@alinowrouzii
Copy link

alinowrouzii commented Jan 20, 2024

Sequelize is performing two queries for findAndCountAll—one for performing the original query and another for counting the records.
There are two problems with this method:

  • First is that sequelize performing unnecessary joins for counting the records. You don't need to do those joins.
  • Also in some cases, these extra joins cause sequelize to return a wrong number of records as a count. In my case count was 26 instead of 4.

Using distinct=true solves the problem in some cases. But the problem remains the same in another case and causes the wrong number to be returned as a count.

I came up with a solution by creating my method.
One for performing the origin query and another for counting records.

Using nest js and sequelize-typescript:

import { Attributes, FindAndCountOptions, ModelStatic } from 'sequelize';
import { Model } from 'sequelize-typescript';

/**
 * This is a workaround for the issue
 * sequlize do an extra join for counting the number of records.
 * This is a problem when you have a lot of records and you want to count them
 * also in some cases sequelize returns a WRONG number of records which is another problem
 * for the method findAndCountAll sequelize do two queries. one for counting and one for getting the records
 * Here I added a new method findAndCountAllCorrectly which does the same thing but without extra joins and also in the correct way
 * *IMPORTANT: instead of using findAndCountAll use findAndCountAllCorrectly. For this, you need to extend
 * *your model from CustomModel instead of Model
 */

export class CustomModel<T> extends Model<T> {
  public static async findAndCountAllCorrectly<M extends Model>(
    this: ModelStatic<M>,
    options?: Omit<FindAndCountOptions<Attributes<M>>, 'group'>,
  ): Promise<{ rows: M[]; count: number }> {
    const itemsPromise = super.findAll<M>(options);
    const countPromise = super.count({ where: options?.where });
    console.log(
      'hi im going the count all records without extra joins and most importantly in a correct way',
    );
    const [items, count] = await Promise.all([itemsPromise, countPromise]);
    // const count = countGroup[0].count;
    return { rows: items, count };
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: count For issues and PRs. Things that involve counting such as the `count` and the `findAndCountAll` meth. type: bug type: docs For issues and PRs. Things related to documentation, such as changes in the manuals / API reference.
Projects
Development

No branches or pull requests