From 7e4fc5566519e841d393f198dd5f74cb7c823451 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zo=C3=A9?= Date: Thu, 27 Jan 2022 16:43:33 +0100 Subject: [PATCH] docs: clarify how the `limit` option works (#13985) --- lib/dialects/mssql/query-generator.js | 4 +++- lib/operators.ts | 7 +++++- test/support.js | 15 ++++++++++++ test/unit/sql/select.test.js | 32 +++++++++++++++++++++++-- types/lib/model.d.ts | 34 +++++++++++++++++++++++++-- 5 files changed, 86 insertions(+), 6 deletions(-) diff --git a/lib/dialects/mssql/query-generator.js b/lib/dialects/mssql/query-generator.js index 0920e015a3f3..49d73adec724 100644 --- a/lib/dialects/mssql/query-generator.js +++ b/lib/dialects/mssql/query-generator.js @@ -966,7 +966,9 @@ class MSSQLQueryGenerator extends AbstractQueryGenerator { } if (options.limit || options.offset) { - if (!options.order || !options.order.length || options.include && !orders.subQueryOrder.length) { + // TODO: document why this is adding the primary key of the model in ORDER BY + // if options.include is set + if (!options.order || options.order.length === 0 || options.include && orders.subQueryOrder.length === 0) { const tablePkFragment = `${this.quoteTable(options.tableAs || model.name)}.${this.quoteIdentifier(model.primaryKeyField)}`; if (!options.order || !options.order.length) { fragment += ` ORDER BY ${tablePkFragment}`; diff --git a/lib/operators.ts b/lib/operators.ts index a6e581d96e9c..a3a30b0b971a 100644 --- a/lib/operators.ts +++ b/lib/operators.ts @@ -478,7 +478,12 @@ interface OpTypes { readonly values: unique symbol; } -const Op: OpTypes = { +// Note: These symbols are registered in the Global Symbol Registry +// to counter bugs when two different versions of this library are loaded +// Source issue: https://github.com/sequelize/sequelize/issues/8663 +// This is not an endorsement of having two different versions of the library loaded at the same time, +// a lot more is going to silently break if you do this. +export const Op: OpTypes = { eq: Symbol.for('eq'), ne: Symbol.for('ne'), gte: Symbol.for('gte'), diff --git a/test/support.js b/test/support.js index 220100284d4c..983c841677f4 100644 --- a/test/support.js +++ b/test/support.js @@ -248,6 +248,21 @@ const Support = { isDeepEqualToOneOf(actual, expectedOptions) { return expectedOptions.some(expected => isDeepStrictEqual(actual, expected)); + }, + + /** + * Reduces insignificant whitespace from SQL string. + * + * @param {string} sql the SQL string + * @returns {string} the SQL string with insignificant whitespace removed. + */ + minifySql(sql) { + // replace all consecutive whitespaces with a single plain space character + return sql.replace(/\s+/g, ' ') + // remove space before coma + .replace(/ ,/g, ',') + // remove whitespace at start & end + .trim(); } }; diff --git a/test/unit/sql/select.test.js b/test/unit/sql/select.test.js index 646128710186..544d8d5cf6be 100644 --- a/test/unit/sql/select.test.js +++ b/test/unit/sql/select.test.js @@ -15,10 +15,10 @@ const Support = require('../support'), describe(Support.getTestDialectTeaser('SQL'), () => { describe('select', () => { - const testsql = function(options, expectation) { + const testsql = function(options, expectation, testFunction = it) { const model = options.model; - it(util.inspect(options, { depth: 2 }), () => { + testFunction(util.inspect(options, { depth: 2 }), () => { return expectsql( sql.selectQuery( options.table || model && model.getTableName(), @@ -30,6 +30,8 @@ describe(Support.getTestDialectTeaser('SQL'), () => { }); }; + testsql.only = (options, expectation) => testsql(options, expectation, it.only); + testsql({ table: 'User', attributes: [ @@ -299,6 +301,32 @@ describe(Support.getTestDialectTeaser('SQL'), () => { }) AS [user] LEFT OUTER JOIN [post] AS [POSTS] ON [user].[id_user] = [POSTS].[user_id] ORDER BY [user].[last_name] ASC;` }); + // By default, SELECT with include of a multi association & limit will be ran as a subQuery + // This checks the result when the query is forced to be ran without a subquery + testsql({ + table: User.getTableName(), + model: User, + include, + attributes: [ + ['id_user', 'id'], + 'email', + ['first_name', 'firstName'], + ['last_name', 'lastName'] + ], + order: [['[last_name]'.replace(/\[/g, Support.sequelize.dialect.TICK_CHAR_LEFT).replace(/\]/g, Support.sequelize.dialect.TICK_CHAR_RIGHT), 'ASC']], + limit: 30, + offset: 10, + hasMultiAssociation: true, // must be set only for mssql dialect here + subQuery: false + }, { + default: Support.minifySql(`SELECT [user].[id_user] AS [id], [user].[email], [user].[first_name] AS [firstName], [user].[last_name] AS [lastName], [POSTS].[id] AS [POSTS.id], [POSTS].[title] AS [POSTS.title] + FROM [users] AS [user] LEFT OUTER JOIN [post] AS [POSTS] + ON [user].[id_user] = [POSTS].[user_id] + ORDER BY [user].[last_name] ASC + ${sql.addLimitAndOffset({ limit: 30, offset: 10, order: [['last_name', 'ASC']], include }, User)}; + `) + }); + const nestedInclude = Model._validateIncludedElements({ include: [{ attributes: ['title'], diff --git a/types/lib/model.d.ts b/types/lib/model.d.ts index b5f5982dbf92..d74661797bef 100644 --- a/types/lib/model.d.ts +++ b/types/lib/model.d.ts @@ -555,10 +555,37 @@ export interface FindOptions group?: GroupOption; /** - * Limit the results + * Limits how many items will be retrieved by the operation. + * + * If `limit` and `include` are used together, Sequelize will turn the `subQuery` option on by default. + * This is done to ensure that `limit` only impacts the Model on the same level as the `limit` option. + * + * You can disable this behavior by explicitly setting `subQuery: false`, however `limit` will then + * affect the total count of returned values, including eager-loaded associations, instead of just one table. + * + * @example + * // in the following query, `limit` only affects the "User" model. + * // This will return 2 users, each including all of their projects. + * User.findAll({ + * limit: 2, + * include: [User.associations.projects], + * }); + * + * @example + * // in the following query, `limit` affects the total number of returned values, eager-loaded associations included. + * // This may return 2 users, each with one project, + * // or 1 user with 2 projects. + * User.findAll({ + * limit: 2, + * include: [User.associations.projects], + * subQuery: false, + * }); */ limit?: number; + // TODO: document this - this is an undocumented property but it exists and there are tests for it. + groupedLimit?: unknown; + /** * Skip the results; */ @@ -589,7 +616,10 @@ export interface FindOptions having?: WhereOptions; /** - * Use sub queries (internal) + * Use sub queries (internal). + * + * If unspecified, this will `true` by default if `limit` is specified, and `false` otherwise. + * See {@link FindOptions#limit} for more information. */ subQuery?: boolean; }