{"action":"created","issue":{"url":"https://api.github.com/repos/sequelize/sequelize/issues/9188","repository_url":"https://api.github.com/repos/sequelize/sequelize","labels_url":"https://api.github.com/repos/sequelize/sequelize/issues/9188/labels{/name}","comments_url":"https://api.github.com/repos/sequelize/sequelize/issues/9188/comments","events_url":"https://api.github.com/repos/sequelize/sequelize/issues/9188/events","html_url":"https://github.com/sequelize/sequelize/pull/9188","id":305602795,"number":9188,"title":"fix(query-generator): generate subQueryFilter even for nested joins","user":{"login":"jharting","id":167352,"avatar_url":"https://avatars0.githubusercontent.com/u/167352?v=4","gravatar_id":"","url":"https://api.github.com/users/jharting","html_url":"https://github.com/jharting","followers_url":"https://api.github.com/users/jharting/followers","following_url":"https://api.github.com/users/jharting/following{/other_user}","gists_url":"https://api.github.com/users/jharting/gists{/gist_id}","starred_url":"https://api.github.com/users/jharting/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jharting/subscriptions","organizations_url":"https://api.github.com/users/jharting/orgs","repos_url":"https://api.github.com/users/jharting/repos","events_url":"https://api.github.com/users/jharting/events{/privacy}","received_events_url":"https://api.github.com/users/jharting/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":13,"created_at":"2018-03-15T15:32:02Z","updated_at":"2018-04-09T13:44:44Z","closed_at":"2018-03-22T11:04:51Z","author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/sequelize/sequelize/pulls/9188","html_url":"https://github.com/sequelize/sequelize/pull/9188","diff_url":"https://github.com/sequelize/sequelize/pull/9188.diff","patch_url":"https://github.com/sequelize/sequelize/pull/9188.patch"},"body":"Closes https://github.com/sequelize/sequelize/issues/9157\r\n\r\n_Please make sure to review and check all of these items:_\r\n\r\n- [x] Does `npm run test` or `npm run test-DIALECT` pass with this change (including linting)?\r\n- [x] Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?\r\n- [x] Have you added new tests to prevent regressions?\r\n- [ ] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?\r\n- [x] Did you follow the commit message conventions explained in [CONTRIBUTING.md](../CONTRIBUTING.md)?\r\n\r\n### Description of change\r\n\r\nThis code introduces `generateSubQueryFilter()` which builds upon code previously developed in `generateThroughJoin()` and `generateInclude()` to guarantee that proper subQueryFilter even in scenarios where multiple levels of duplicating joins are nested in a query that uses LIMIT / OFFSET.\r\n\r\nHere's an example:\r\n\r\nConsider the following query:\r\n\r\n```js\r\nProject.findAll({\r\n  include: [{\r\n    model: this.User,\r\n    required: true,\r\n    include: [{\r\n      model: this.Hobby,\r\n      where: {\r\n        name: 'archery'\r\n      }\r\n    }]\r\n  }],\r\n  order: ['name'],\r\n  limit: 1,\r\n  offset: 1\r\n})\r\n```\r\n\r\nProject --> User and User --> Hobby are many-to-many queries.\r\n\r\nFor limit and offset to work properly with duplicating joins it is necessary to push the required joins and where clauses down to subQueryFilter. Currently, sequelize produces the following output:\r\n\r\n```sql\r\nSELECT `Project`.*, `Users`.`name` AS `Users.name`, `Users->user_project`.`createdAt` AS `Users.user_project.createdAt`, `Users->user_project`.`updatedAt` AS `Users.user_project.updatedAt`, `Users->user_project`.`UserName` AS `Users.user_project.UserName`, `Users->user_project`.`ProjectName` AS `Users.user_project.ProjectName`, `Users->Hobbies`.`name` AS `Users.Hobbies.name`, `Users->Hobbies->user_hobby`.`createdAt` AS `Users.Hobbies.user_hobby.createdAt`, `Users->Hobbies->user_hobby`.`updatedAt` AS `Users.Hobbies.user_hobby.updatedAt`, `Users->Hobbies->user_hobby`.`UserName` AS `Users.Hobbies.user_hobby.UserName`, `Users->Hobbies->user_hobby`.`HobbyName` AS `Users.Hobbies.user_hobby.HobbyName` FROM \r\n(\r\n    SELECT `Project`.`name` FROM `Projects` AS `Project` WHERE (\r\n        SELECT `user_project`.`UserName` FROM `user_project` AS `user_project` INNER JOIN `Users` AS `User` ON `user_project`.`UserName` = `User`.`name` WHERE (`Project`.`name` = `user_project`.`ProjectName`) LIMIT 1 \r\n    ) IS NOT NULL ORDER BY `Project`.`name` LIMIT 1, 1\r\n) AS `Project` INNER JOIN ( `user_project` AS `Users->user_project` INNER JOIN `Users` AS `Users` ON `Users`.`name` = `Users->user_project`.`UserName`) ON `Project`.`name` = `Users->user_project`.`ProjectName` INNER JOIN ( `user_hobby` AS `Users->Hobbies->user_hobby` INNER JOIN `Hobbies` AS `Users->Hobbies` ON `Users->Hobbies`.`name` = `Users->Hobbies->user_hobby`.`HobbyName`) ON `Users`.`name` = `Users->Hobbies->user_hobby`.`UserName` AND `Users->Hobbies`.`name` = 'archery' ORDER BY `Project`.`name`;\r\n\r\n```\r\n\r\nUpon closer inspection this is wrong as the subQueryFilter (most nested query) does not join into the required Hobbies table nor does it check for `name: 'archery'`. As a result a User that is associated with the Project but does not have association with `archery` Hobby entity may come out of the subQuery. Such user would then be filtered out in the top-most query, resulting in an empty result set.\r\n\r\nSQL generated after this patch:\r\n\r\n```sql\r\nSELECT `Project`.*, `Users`.`name` AS `Users.name`, `Users->user_project`.`createdAt` AS `Users.user_project.createdAt`, `Users->user_project`.`updatedAt` AS `Users.user_project.updatedAt`, `Users->user_project`.`UserName` AS `Users.user_project.UserName`, `Users->user_project`.`ProjectName` AS `Users.user_project.ProjectName`, `Users->Hobbies`.`name` AS `Users.Hobbies.name`, `Users->Hobbies->user_hobby`.`createdAt` AS `Users.Hobbies.user_hobby.createdAt`, `Users->Hobbies->user_hobby`.`updatedAt` AS `Users.Hobbies.user_hobby.updatedAt`, `Users->Hobbies->user_hobby`.`UserName` AS `Users.Hobbies.user_hobby.UserName`, `Users->Hobbies->user_hobby`.`HobbyName` AS `Users.Hobbies.user_hobby.HobbyName` FROM \r\n(\r\n    SELECT `Project`.`name` FROM `Projects` AS `Project` WHERE ((\r\n        SELECT `user_project`.`UserName` FROM `user_project` AS `user_project` INNER JOIN `Users` AS `User` ON `user_project`.`UserName` = `User`.`name` INNER JOIN ( `user_hobby` AS `User->Hobbies->user_hobby` INNER JOIN `Hobbies` AS `User->Hobbies` ON `User->Hobbies`.`name` = `User->Hobbies->user_hobby`.`HobbyName`) ON `User`.`name` = `User->Hobbies->user_hobby`.`UserName` AND `User->Hobbies`.`name` = 'archery' WHERE (`Project`.`name` = `user_project`.`ProjectName`) LIMIT 1\r\n    ) IS NOT NULL) ORDER BY `Project`.`name` LIMIT 1, 1\r\n) AS `Project` INNER JOIN ( `user_project` AS `Users->user_project` INNER JOIN `Users` AS `Users` ON `Users`.`name` = `Users->user_project`.`UserName`) ON `Project`.`name` = `Users->user_project`.`ProjectName` INNER JOIN ( `user_hobby` AS `Users->Hobbies->user_hobby` INNER JOIN `Hobbies` AS `Users->Hobbies` ON `Users->Hobbies`.`name` = `Users->Hobbies->user_hobby`.`HobbyName`) ON `Users`.`name` = `Users->Hobbies->user_hobby`.`UserName` AND `Users->Hobbies`.`name` = 'archery' ORDER BY `Project`.`name`;\r\n```\r\n\r\nNotice how here the subQueryFilter joins into Hobies table and checks for `name=archery`.\r\n\r\nI tested this patch extensively both in our project and with Sequelize testsuite. However, please review `generateSubQueryFilter` carefully as may have missed some special cases."},"comment":{"url":"https://api.github.com/repos/sequelize/sequelize/issues/comments/379757049","html_url":"https://github.com/sequelize/sequelize/pull/9188#issuecomment-379757049","issue_url":"https://api.github.com/repos/sequelize/sequelize/issues/9188","id":379757049,"user":{"login":"Alexsey","id":6392013,"avatar_url":"https://avatars0.githubusercontent.com/u/6392013?v=4","gravatar_id":"","url":"https://api.github.com/users/Alexsey","html_url":"https://github.com/Alexsey","followers_url":"https://api.github.com/users/Alexsey/followers","following_url":"https://api.github.com/users/Alexsey/following{/other_user}","gists_url":"https://api.github.com/users/Alexsey/gists{/gist_id}","starred_url":"https://api.github.com/users/Alexsey/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Alexsey/subscriptions","organizations_url":"https://api.github.com/users/Alexsey/orgs","repos_url":"https://api.github.com/users/Alexsey/repos","events_url":"https://api.github.com/users/Alexsey/events{/privacy}","received_events_url":"https://api.github.com/users/Alexsey/received_events","type":"User","site_admin":false},"created_at":"2018-04-09T13:44:44Z","updated_at":"2018-04-09T13:44:44Z","author_association":"CONTRIBUTOR","body":"@sunshinewyin It was an upstream to regression fix so I thought that they are related. Now I see that they aren't, at least not directly. So I agree with @jamiesoncj that we should allow the reward rightaway. @jharting I'm sorry I didn't rise this question in a week after https://github.com/sequelize/sequelize/pull/9247 was merged. I have never claimed a bounty but I think you should go throught the link that @jamiesoncj has provided and press the \"Claim Bounty\" button. Thank you for this fix - you have killed a giant here :)"}}
{"actor":{"display_login":"Alexsey"}}