Merge pull request #725 from janmeier/limitoffset

Closes #723 (consistent handling of limit and offset across dialects)

Merge pull request #725 from janmeier/limitoffset
Closes #723 (consistent handling of limit and offset across dialects)
Daniel Durante
Commit 366685e5 authored Jun 28, 2013 by Daniel Durante
Showing with 21 additions and 12 deletions
changelog.md
lib/dialects/mysql/query-generator.js
lib/dialects/postgres/query-generator.js
lib/dialects/sqlite/query-generator.js
spec-jasmine/dao-factory.spec.js
spec-jasmine/mysql/query-generator.spec.js
--- a/changelog.md
+++ b/changelog.md
@@ -30,13 +30,14 @@
 - [FEATURE] `findOrCreate` now returns an additional flag (`created`), that is true if a model was created, and false if it was found [#648](https://github.com/sequelize/sequelize/pull/648). janmeier
 - [FEATURE] Field and table comments for MySQL and PG. [#523](https://github.com/sequelize/sequelize/pull/523). MySQL by iamjochen. PG by janmeier
 - [FEATURE] BigInts can now be used for autoincrement/serial columns. [#673](https://github.com/sequelize/sequelize/pull/673). thanks to sevastos
- [REFACTORING] hasMany now uses a single SQL statement when creating and destroying associations, instead of removing each association seperately [690](https://github.com/sequelize/sequelize/pull/690). Inspired by [#104](https://github.com/sequelize/sequelize/issues/104). janmeier
 - [FEATURE] Use moment for better postgres timestamp strings. [#710](https://github.com/sequelize/sequelize/pull/710). Thanks to seth-admittedly
 - [FEATURE] Keep milliseconds in timestamps for postgres. [#712](https://github.com/sequelize/sequelize/pull/712). Thanks to seth-admittedly
 - [FEATURE] You can now set lingo's language through Sequelize. [#713](https://github.com/sequelize/sequelize/pull/713). Thanks to durango
 - [FEATURE] Added a `findAndCountAll`, useful for pagination. [#533](https://github.com/sequelize/sequelize/pull/533). Thanks to iamjochen
 - [FEATURE] Made explicit migrations possible. [#728](https://github.com/sequelize/sequelize/pull/728). Thanks to freezy
 - [FEATURE] Added support for where clauses containing !=, < etc. and support for date ranges  [#727](https://github.com/sequelize/sequelize/pull/727). Thanks to durango
+- [REFACTORING] hasMany now uses a single SQL statement when creating and destroying associations, instead of removing each association seperately [690](https://github.com/sequelize/sequelize/pull/690). Inspired by [#104](https://github.com/sequelize/sequelize/issues/104). janmeier
+- [REFACTORING] Consistent handling of offset across dialects. Offset is now always applied, and limit is set to max table size of not limit is given [#725](https://github.com/sequelize/sequelize/pull/725). janmeier
 # v1.6.0 #
 - [DEPENDENCIES] upgrade mysql to alpha7. You *MUST* use this version or newer for DATETIMEs to work

--- a/lib/dialects/mysql/query-generator.js
+++ b/lib/dialects/mysql/query-generator.js
@@ -217,8 +217,14 @@ module.exports = (function() {
        query += " ORDER BY " + options.order
      }
+      if (options.offset && !options.limit) {
-      if (options.limit && !(options.include && (options.limit === 1))) {
+        /*
+         * If no limit is defined, our best bet is to use the max number of rows in a table. From the MySQL docs:
+         * There is a limit of 2^32 (~4.295E+09) rows in a MyISAM table. If you build MySQL with the --with-big-tables option, 
+         * the row limitation is increased to (2^32)^2 (1.844E+19) rows.
+         */
+        query += " LIMIT " + options.offset + ", " + 18440000000000000000;
+      } else if (options.limit && !(options.include && (options.limit === 1))) {
        if (options.offset) {
          query += " LIMIT " + options.offset + ", " + options.limit
        } else {

--- a/lib/dialects/postgres/query-generator.js
+++ b/lib/dialects/postgres/query-generator.js
@@ -293,7 +293,6 @@ module.exports = (function() {
      attrValueHash = Utils.removeNullValuesFromHash(attrValueHash, this.options.omitNull)
      var query     = "INSERT INTO <%= table %> (<%= attributes %>) VALUES (<%= values %>) RETURNING *;"
      // Remove serials that are null or undefined, which causes an error in PG
      Utils._.forEach(attrValueHash, function(value, key, hash) {
        if (tables[tableName]) {

--- a/lib/dialects/sqlite/query-generator.js
+++ b/lib/dialects/sqlite/query-generator.js
@@ -188,8 +188,13 @@ module.exports = (function() {
        query += " ORDER BY " + options.order
      }
+      if (options.offset && !options.limit) {
-      if (options.limit && !(options.include && (options.limit === 1))) {
+        /*
+         * If no limit is defined, our best bet is to use the max number of rows in a table. From the SQLite docs:
+         * A 140 terabytes database can hold no more than approximately 1e+13 rows
+         */
+        query += " LIMIT " + options.offset + ", " + 10000000000000;
+      } else if (options.limit && !(options.include && (options.limit === 1))) {
        if (options.offset) {
          query += " LIMIT " + options.offset + ", " + options.limit
        } else {

--- a/spec-jasmine/dao-factory.spec.js
+++ b/spec-jasmine/dao-factory.spec.js
@@ -238,9 +238,7 @@ describe('DAOFactory', function() {
            })
          })
        })
-/*
-// at time of writing (v1.6.0) Sequelize does not seem to support 'offset' on it's own consistently (goes wrong for PostGRES and SQLite)
        it("handles offset", function() {
          Helpers.async(function(done) {
            User.findAndCountAll({offset: 1}).success(function(info) {
@@ -251,7 +249,7 @@ describe('DAOFactory', function() {
            })
          })
        })
-*/
        it("handles limit", function() {
          Helpers.async(function(done) {
            User.findAndCountAll({limit: 1}).success(function(info) {

--- a/spec-jasmine/mysql/query-generator.spec.js
+++ b/spec-jasmine/mysql/query-generator.spec.js
@@ -178,9 +178,9 @@ describe('QueryGenerator', function() {
        expectation: "SELECT * FROM `myTable` LIMIT 2, 10;",
        context: QueryGenerator
      }, {
-        title: 'ignores offset if no limit was passed',
+        title: 'uses default limit if only offset is specified',
        arguments: ['myTable', {offset: 2}],
-        expectation: "SELECT * FROM `myTable`;",
+        expectation: "SELECT * FROM `myTable` LIMIT 2, 18440000000000000000;",
        context: QueryGenerator
      }, {
        title: 'multiple where arguments',