Skip to content

fix(select): do not force set subQuery to false#13490

Merged
sdepold merged 6 commits into
sequelize:mainfrom
floydspace:fix/subQuery-select-with-multi-include
Oct 23, 2021
Merged

fix(select): do not force set subQuery to false#13490
sdepold merged 6 commits into
sequelize:mainfrom
floydspace:fix/subQuery-select-with-multi-include

Conversation

@floydspace
Copy link
Copy Markdown
Contributor

This occurs on validating included elements function in spite of a user sets it to true in include options

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Here it is the SSCCE:

const { createSequelizeInstance } = require('./dev/sscce-helpers');
const { Model, DataTypes } = require('.');

const sequelize = createSequelizeInstance({ benchmark: true });

class User extends Model {}
User.init({
  scopeId: DataTypes.INTEGER
}, { sequelize, modelName: 'User' });

class Company extends Model {}
Company.init({
  name: DataTypes.STRING,
  public: DataTypes.BOOLEAN,
  scopeId: DataTypes.INTEGER
}, { sequelize, modelName: 'Company' });

class Profession extends Model {}
Profession.init({
  name: DataTypes.STRING,
  scopeId: DataTypes.INTEGER
}, { sequelize, modelName: 'Profession' });

User.belongsTo(Company, { foreignKey: 'companyId' });
User.belongsTo(Profession, { foreignKey: 'professionId' });
Company.hasMany(User, { foreignKey: 'companyId' });
Profession.hasMany(User, { foreignKey: 'professionId' });

(async () => {
  await sequelize.sync({ force: true });

  const companies = await Company.findAll({
    where: { '$Users.Profession.name$': 'test' },
    include: [
      {
        model: User,
        attributes: [],
        include: [
          {
            model: Profession,
            attributes: [],
            required: true
          }
        ],
        required: true,
        subQuery: true
      }
    ],
    limit: 5,
    offset: 0,
    subQuery: true
  });

  await sequelize.close();
})();

which generate an invalid SQL query

What do you expect to happen?

I expect that query would not use not existing alias

What is actually happening?

But it generated query like this:

SELECT `Company`.* FROM (
    SELECT `Company`.`id`, `Company`.`name`, `Company`.`public`, `Company`.`scopeId`, `Company`.`createdAt`, `Company`.`updatedAt` FROM `Companies` AS `Company` 
    INNER JOIN `Professions` AS `Users->Profession` ON `Users`.`professionId` = `Users->Profession`.`id` 
    WHERE `Users->Profession`.`name` = 'test' AND ( 
        SELECT `Users`.`companyId` FROM `Users` AS `Users` 
        INNER JOIN `Professions` AS `Profession` ON `Users`.`professionId` = `Profession`.`id` 
        WHERE (`Users`.`companyId` = `Company`.`id`) LIMIT 1 
    ) IS NOT NULL LIMIT 0, 5
) AS `Company` 
INNER JOIN `Users` AS `Users` ON `Company`.`id` = `Users`.`companyId`;

as you can see in line 3 it joins to the Users table but the Users alias is not exist in current sub select scope, it has been concatenated at the end of query. So DB returns error: Unknown column 'Users.professionId' in 'on clause'.

The following PR fixes the issue, for me at least. And now generates the following SQL:

SELECT `Company`.* FROM (
    SELECT `Company`.`id`, `Company`.`name`, `Company`.`public`, `Company`.`scopeId`, `Company`.`createdAt`, `Company`.`updatedAt` FROM `Companies` AS `Company` 
    INNER JOIN `Users` AS `Users` ON `Company`.`id` = `Users`.`companyId`
    INNER JOIN `Professions` AS `Users->Profession` ON `Users`.`professionId` = `Users->Profession`.`id` 
    WHERE `Users->Profession`.`name` = 'test' AND ( 
        SELECT `Users`.`companyId` FROM `Users` AS `Users` 
        INNER JOIN `Professions` AS `Profession` ON `Users`.`professionId` = `Profession`.`id` 
        WHERE (`Users`.`companyId` = `Company`.`id`) LIMIT 1 
    ) IS NOT NULL LIMIT 0, 5
) AS `Company`;

All tests look fine.

Please have a look on it.

this occurred on validating included elements in spite of user set it to true in include options
@floydspace
Copy link
Copy Markdown
Contributor Author

Let me know if I'm using incorrect find options, I also tried using duplicating flag which says nothing to me from documentation, but it didn't fix the query.
I need nested filtering and pagination at the same query.
Thank you

@wbourne0 wbourne0 added the type: bug DEPRECATED: replace with the "bug" issue type label Oct 10, 2021
@sdepold sdepold self-assigned this Oct 17, 2021
@sdepold
Copy link
Copy Markdown
Member

sdepold commented Oct 17, 2021

Hey @floydspace, thanks for your work. Do you think we have to update any documentation or are we just fixing it as such as you would have expected to begin with.

I have to admit that I only understand half of what is going on here, but I can tell that we are generating a broken SQL query and it's better now :D

@sdepold sdepold added the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Oct 17, 2021
@floydspace
Copy link
Copy Markdown
Contributor Author

@sdepold I understand even less, so I hope there are no uncovered cases, at least it's not getting worse.
I hardly can find any documentation about the subQuery option, so nothing to update.

@github-actions github-actions Bot removed the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Oct 18, 2021
@sdepold sdepold merged commit 0943339 into sequelize:main Oct 23, 2021
@github-actions
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 6.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
* fix(select): do not force set subQuery to false

this occurred on validating included elements in spite of user set it to true in include options

* test: make include (subQuery alias) tests dry

Co-authored-by: Victor Korzunin <victor.korzunin@blanco.services>
Co-authored-by: Constantin Metz <58604248+Keimeno@users.noreply.github.com>
Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released type: bug DEPRECATED: replace with the "bug" issue type

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants