Limit parameter breaks eager loading where clause

[Goblinlordx]

What are you doing?

npm i sequelize sqlite3

// index.js
const Sequelize = require("sequelize");

const sequelize = new Sequelize({ dialect: "sqlite" });
class Model1 extends Sequelize.Model {}
Model1.init({}, { sequelize });

class Model2 extends Sequelize.Model {}
Model2.init({}, { sequelize });

Model1.hasMany(Model2);
Model2.belongsTo(Model1);

(async () => {
  await Promise.all([Model1.sync(), Model2.sync()]);

  await Promise.all([
    Model1.create({}),
    Model1.create({}),
    Model1.create({}),
    Model2.create({
      Model1Id: 1
    })
  ]);

  const logRes = label => res =>
    console.log(`${label}\n`, res.map(o => o.toJSON()));
  await Model1.findAll().then(logRes("Model1:"));
  await Model2.findAll().then(logRes("Model2:"));
  await Model1.findAll({
    where: {
      "$Model2s.id$": null
    },
    include: [
      {
        model: Model2,
        required: false
      }
    ]
  }).then(logRes("Model1 with no Model2 record:"));
  await Model1.findAll({
    where: {
      "$Model2s.id$": null
    },
    limit: 1,
    include: [
      {
        model: Model2,
        required: false
      }
    ]
  }).then(logRes("Erroneous Query:"));
})();

To Reproduce
Steps to reproduce the behavior:

1. Define 2 models where there are associations defined between models (I tested with hasMany and belongsTo).
2. Run the following Model1.findAll({ where: "$Model2s.id$": null, limit: 1, include: { model: Model2, required: false})
3. Error received: UnhandledPromiseRejectionWarning: SequelizeDatabaseError: SQLITE_ERROR: no such column: Model2s.id

In the example code above you can see the results. A query without limit works properly. A query with limit causes the error. It appears that it is putting the limit clause in the incorrect place in the query (should be after the where clause at the end) and instead moves the part of the where which contains the condition selecting the nested property into the inner where clause erroneously.
Run:

With these files and node version 11+ I receive the described error only when it hits the last findAll query when limit is added.

What do you expect to happen?

I expect the query to be executed successfully without an error.

What is actually happening?

LIMIT clause and the part of the where clause selecting the property from the eagerly loaded model are put on the inner where clause selection causing an error. UnhandledPromiseRejectionWarning: SequelizeDatabaseError: SQLITE_ERROR: no such column: Model2s.id

Environment

Dialect:

• mysql
• postgres
• sqlite
• mssql
• any
  Dialect library version: N/A
  Database version: N/A (occurs on both sqlite3 and mysql)
  Sequelize version: 5.8.9
  Node Version: 11.10.1
  OS: N/A
  If TypeScript related: TypeScript version: N/A
  Tested with latest release:
• No
• Yes, specify that version: 5.8.10

[Goblinlordx]

Related issue: #10962

[Goblinlordx]

Found a 2 solutions. One seems completely undocumented and the other I can only find very vague documentation.

The first solution I only found here: #7778 (comment)
This solution involves using subQuery: false on the top level options object. In above example like:

  await Model1.findAll({
    where: {
      "$Model2s.id$": null
    },
    subQuery: false,
    limit: 1,
    include: [
      {
        model: Model2,
        required: false
      }
    ]
  })

The second solution is here in: #1756 (comment) -> Model Reference Docs
This solution would be used like:

  await Model1.findAll({
    where: {
      "$Model2s.id$": null
    },
    limit: 1,
    include: [
      {
        model: Model2,
        required: false,
        duplicating: false
      }
    ]
  })

Both solutions require the user to know they are even available (a real problem with the first). They also require the user to know that under the hood, Sequelize will be using a sub-query. This seems like a real issue considering I feel that is very in depth knowledge of the inner workings of Sequelize.

Can this be adjusted to not require either of these 2 things in this case (since the generated query doesn't even work) or can appropriate documentation under Associations and Querying be added so that users can, at the least, be informed about these 2 options and when to use them?

[felamaslen]

@Goblinlordx the above solutions are non-solutions. Here is why:

Both of the proposed solutions turn the following (erroneous) query:

SELECT "main".*, "joined"."prop" AS "joined.prop"
FROM (
  SELECT "main"....
  FROM "main" AS "main"
  WHERE "joined"."prop" = "foo"
  LIMIT 10
  OFFSET 10
) AS "main"
LEFT OUTER JOIN "joined" AS "joined" ON "joined"."foreign_key" = "main"."other_key"

into the following:

SELECT "main".*, "joined"."prop" AS "joined.prop"
FROM "main" as "main"
  SELECT "main"....
  FROM "main" AS "main"
) AS "main"
LEFT OUTER JOIN "joined" AS "joined" ON "joined"."foreign_key" = "main"."other_key"
WHERE "joined"."prop" = "foo"
LIMIT 10
OFFSET 10

This executes, but it isn't guaranteed to get the first 10 unique "main" results. Why? Because if one of the "main" results has more than one matching "joined" result, it will appear more than once in the result set. E.g. you could have the following full result set:

"main"."id" | "joined"."id"
m1          | j1
m1          | j2
m1          | j3
m2          | j4
m2          | j5
m3          | j6
m4          | j7
m5          | j8
m5          | j9
m5          | j10
m5          | j11

It would be from this result set that the LIMIT and OFFSET is applied. So in this case, you'd get all of m1 up to m4 with their joins, and m5 with its first three joins.

You'd miss j11, joined to m5 (!).

You'd also only get five results in your set, not 10 as you requested (which you should, assuming the full result set includes more than 10 unique main items).

[felamaslen]

I believe this is a duplicate of #10962.

[Goblinlordx]

Understood~ I hope your PR gets merged and released soon. It is a little disheartening that your issue has no comments other than your own and others with the issue and it is a month old.

[goohooh]

@felamaslen @Goblinlordx Thx guys and I do hope so that @felamaslen PR gets merged~

[papb]

Closing in favor of #10962