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

Regression: distinct option in scopes generates invalid SQL for findAndCountAll() queries (MySQL 5.5) #6404

Closed
adamjmurray opened this issue Aug 9, 2016 · 1 comment

Comments

@adamjmurray
Copy link

What you are doing?

We are using scopes with the distinct option in combination with findAndCount()

var Sequelize = require('sequelize');
var sequelize = new Sequelize('mysql://root@localhost/sequelize_test');

var User = sequelize.define('user', {
  name: {
    type: Sequelize.STRING
  }
});

var Role = sequelize.define('role', {
  name: {
    type: Sequelize.STRING
  }
});

Role.belongsTo(User);
User.hasMany(Role);

User.addScope('withRoles', {
  distinct: true,
  include: [{model: Role}]
});

sequelize.sync({force:true})
  .then(function() {
    return User.scope('withRoles').findAndCountAll();
  });

What do you expect to happen?

In Sequelize 3.23.4 and earlier, this generated valid SQL (for MySQL):

SELECT count(DISTINCT(`user`.`id`)) AS `count` FROM `users` AS `user` LEFT OUTER JOIN `roles` AS `roles` ON `user`.`id` = `roles`.`userId`;

What is actually happening?

Starting in Sequelize 3.23.5, this generates invalid SQL (for MySQL):

SELECT count(DISTINCT(*)) AS `count` FROM `users` AS `user` LEFT OUTER JOIN `roles` AS `roles` ON `user`.`id` = `roles`.`userId`;

Unhandled rejection SequelizeDatabaseError: ER_PARSE_ERROR: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '*)) AS `count` FROM `users` AS `user`' at line 1

The only difference is count(DISTINCT(*)) instead of count(DISTINCT(user.id))

System info

Dialect: mysql
Database version: 5.5.46
Sequelize version: 3.23.5+

Other notes:

Interestingly, if you change the above code sample to do the same thing but without a scope, like this:

sequelize.sync({force:true})
  .then(function() {
    return User.findAndCountAll({
      distinct: true,
      include: [{model: Role}]
    });
  });

That did not work in Sequelize 3.23.4 and earlier, which is why we setup our scopes with distinct options. This approach started working in 3.23.5 at the same time scopes+distinct broke.

We use this scopes+distinct pattern throughout our codebase. Given the recent changes in behavior, I am hesitant to switch all our scopes to inline options, in case the behavior changes again in a future update.

Is it possible to make distinct + findAndCount work with both scopes and inline options, so we can write our code either way? If not, can a Sequelize contributor comment on whether one of these approaches is officially supported so I can update our codebase appropriately?

@sushantdhiman
Copy link
Contributor

Thanks for reporting with short and easy to test example :) Not everyone does that.

In master #6114 will solve this issue. It allow you to pass col option in count queries.

So this should work

sequelize.sync({force:true})
  .then(function() {
    return User.scope('withRoles').findAndCountAll({ col: 'user.id' });
  })
  .then(console.log)
  .catch(console.error);

I am not sure if #6114 has been backported to v3, if not you can copy code and make a PR, we will release new version with fix.

TimMensch added a commit to TimMensch/sequelize that referenced this issue Feb 1, 2020
* Calling count() for a query with distinct will generate invalid SQL. Sequelize should endeavor to _not_ generate invalid SQL.
* findAndCountAll() similarly calls count(), and will generate invalid SQL unless you pass `col` to
* Sequelize + FeathersJS, to take one example, will generate a call to findAndCountAll() with all rows. findAndCountAll() should just work as expected.

See:

sequelize#7344
sequelize#6418 (comment at the end)
sequelize#6404
sequelize#2713

And I seem to run into this issue every time I use Sequelize. Please merge ASAP and fix.
TimMensch added a commit to TimMensch/sequelize that referenced this issue Feb 1, 2020
Add code to count() to cause it to create valid SQL when distinct=true and col is unspecified or "*". Otherwise the SQL generated is COUNT(DISTINCT(*)), which is invalid. Issues referenced below were already closed, but the issue hadn't actually been fixed, and it _keeps coming up_.

Closes: sequelize#2713, sequelize#6404, sequelize#6418, sequelize#7344
TimMensch added a commit to TimMensch/sequelize that referenced this issue Feb 1, 2020
Add code to count() to cause it to create valid SQL when distinct=true and col is unspecified or "*". Otherwise the SQL generated is COUNT(DISTINCT(*)), which is invalid. Issues referenced below were already closed, but the issue hadn't actually been fixed, and it _keeps coming up_.

Closes: sequelize#2713, sequelize#6404, sequelize#6418, sequelize#7344
TimMensch added a commit to TimMensch/sequelize that referenced this issue Feb 20, 2020
Add code to count() to cause it to create valid SQL when distinct=true and col is unspecified or "*". Otherwise the SQL generated is COUNT(DISTINCT(*)), which is invalid. Issues referenced below were already closed, but the issue hadn't actually been fixed, and it _keeps coming up_.

Closes: sequelize#2713, sequelize#6404, sequelize#6418, sequelize#7344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants