From 85eea5aea9a3d32bdf125c2e2700dd680b006c1f Mon Sep 17 00:00:00 2001 From: idantovi Date: Tue, 16 Aug 2016 18:07:56 +0300 Subject: [PATCH 1/3] add default for non-parsable limit --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index cc718e1..124016a 100644 --- a/index.js +++ b/index.js @@ -95,7 +95,7 @@ exports.middleware = function middleware(limit, maxLimit) { req.query.page = (typeof req.query.page === 'string') ? parseInt(req.query.page, 10) || 1 : 1; - req.query.limit = (typeof req.query.limit === 'string') ? parseInt(req.query.limit, 10) : _limit; + req.query.limit = (typeof req.query.limit === 'string') ? parseInt(req.query.limit, 10) || 0 : _limit; req.skip = req.offset = (req.query.page * req.query.limit) - req.query.limit; From 7493923edea581313dc4d4ae3de82eda03bc1481 Mon Sep 17 00:00:00 2001 From: idantovi Date: Tue, 16 Aug 2016 18:08:31 +0300 Subject: [PATCH 2/3] move the offset calculation after the negative limit check --- index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 124016a..ddc76e0 100644 --- a/index.js +++ b/index.js @@ -97,8 +97,6 @@ exports.middleware = function middleware(limit, maxLimit) { req.query.limit = (typeof req.query.limit === 'string') ? parseInt(req.query.limit, 10) || 0 : _limit; - req.skip = req.offset = (req.query.page * req.query.limit) - req.query.limit; - if (req.query.limit > _maxLimit) req.query.limit = _maxLimit; @@ -108,6 +106,8 @@ exports.middleware = function middleware(limit, maxLimit) { if (req.query.limit < 0) req.query.limit = 0; + req.skip = req.offset = (req.query.page * req.query.limit) - req.query.limit; + res.locals.paginate = {}; res.locals.paginate.page = req.query.page; res.locals.paginate.limit = req.query.limit; From 18a57b4ddfa0b15d9c1615edd4f2a342c5698bbc Mon Sep 17 00:00:00 2001 From: idantovi Date: Tue, 16 Aug 2016 18:23:51 +0300 Subject: [PATCH 3/3] cover limit and page edge cases with tests --- test/index-test.js | 247 ++++++++++++++++++++++++++++----------------- 1 file changed, 157 insertions(+), 90 deletions(-) diff --git a/test/index-test.js b/test/index-test.js index 1dbc62d..81cefde 100644 --- a/test/index-test.js +++ b/test/index-test.js @@ -5,11 +5,11 @@ var url = require('url'); var reqres = require('reqres'); var async = require('async'); -describe('paginate', function() { +describe('paginate', function () { - describe('.href(req)', function() { + describe('.href(req)', function () { - beforeEach(function() { + beforeEach(function () { this.req = { originalUrl: 'http://niftylettuce.com/', query: { @@ -18,33 +18,33 @@ describe('paginate', function() { }; }); - it('should return function', function() { + it('should return function', function () { paginate.href(this.req).should.be.a('function'); }); - describe('the returned function', function() { + describe('the returned function', function () { - it('should return the next page when invoked with no arguments', function() { + it('should return the next page when invoked with no arguments', function () { paginate.href(this.req)().should.equal(util.format('%s?page=%d', url.parse(this.req.originalUrl).pathname, this.req.query.page + 1)); }); - it('should return the next page when invoked with `false` as the first argument', function() { + it('should return the next page when invoked with `false` as the first argument', function () { paginate.href(this.req)().should.equal(util.format('%s?page=%d', url.parse(this.req.originalUrl).pathname, this.req.query.page + 1)); }); - it('should return the previous page href when invoked with `true` as the first argument', function() { + it('should return the previous page href when invoked with `true` as the first argument', function () { paginate.href(this.req)(true).should.equal(util.format('%s?page=%d', url.parse(this.req.originalUrl).pathname, this.req.query.page - 1)); }); - it('should return the previous page href and sorted by title when invoked with `true` and a `params` object', function() { + it('should return the previous page href and sorted by title when invoked with `true` and a `params` object', function () { paginate.href(this.req)(true, { sort: 'title' }).should.equal(util.format('%s?page=%d&sort=title', url.parse(this.req.originalUrl).pathname, this.req.query.page - 1)); }); - it('should return the next page href and sorted by title when invoked with `false` and a `params` object', function() { + it('should return the next page href and sorted by title when invoked with `false` and a `params` object', function () { paginate.href(this.req)(false, { sort: 'title' }).should.equal(util.format('%s?page=%d&sort=title', url.parse(this.req.originalUrl).pathname, this.req.query.page + 1)); }); - it('should return the current page sorted by title when invoked with just a `params` object as the first argument', function() { + it('should return the current page sorted by title when invoked with just a `params` object as the first argument', function () { paginate.href(this.req)({ sort: 'title' }).should.equal(util.format('%s?page=%d&sort=title', url.parse(this.req.originalUrl).pathname, this.req.query.page)); }); @@ -52,9 +52,9 @@ describe('paginate', function() { }); - describe('.hasNextPages(req)', function() { + describe('.hasNextPages(req)', function () { - beforeEach(function() { + beforeEach(function () { this.req = { query: { page: 3 @@ -62,28 +62,28 @@ describe('paginate', function() { }; }); - it('should return function', function() { + it('should return function', function () { paginate.hasNextPages(this.req).should.be.a('function'); }); - describe('the returned function', function() { + describe('the returned function', function () { - it('should return true when there are more pages', function() { + it('should return true when there are more pages', function () { paginate.hasNextPages(this.req)(4).should.be.true; }); - it('should return false when there are no more pages', function() { + it('should return false when there are no more pages', function () { paginate.hasNextPages(this.req)(3).should.be.false; }); - it('should throw an error when pageCount is not a number', function(){ - (function(){ + it('should throw an error when pageCount is not a number', function () { + (function () { paginate.hasNextPages(this.req)(''); }).should.throw(/not a number/); }); - it('should throw an error when pageCount is less than zero', function() { - (function(){ + it('should throw an error when pageCount is less than zero', function () { + (function () { paginate.hasNextPages(this.req)(''); }).should.throw(/\>= 0/); }); @@ -92,72 +92,140 @@ describe('paginate', function() { }); - describe('.middleware(limit, maxLimit)', function() { - - describe('should be a pure function with successive calls not mutating', function() { - - var firstMiddleware, secondMiddleware; - - beforeEach(function() { - firstMiddleware = paginate.middleware(10, 20); - secondMiddleware = paginate.middleware(30, 40); - }); - - it('limit in previous calls', function(done) { - async.parallel( - [ - function(callback) { - var req = reqres.req(); - firstMiddleware(req, reqres.res(), function(err) { - req.query.limit.should.equal(10); - callback(null, null); - }) - }, - function(callback) { - var req = reqres.req(); - secondMiddleware(req, reqres.res(), function(err) { - req.query.limit.should.equal(30); - callback(null, null); - }) - } - ], - done - ); - }); - - it('maxLimit in previous calls', function(done) { - async.parallel( - [ - function(callback) { - var req = reqres.req({query: {limit: '100'}}); - firstMiddleware(req, reqres.res(), function(err) { - req.query.limit.should.equal(20); - callback(null, null); - }) - }, - function(callback) { - var req = reqres.req({query: {limit: '100'}}); - secondMiddleware(req, reqres.res(), function(err) { - req.query.limit.should.equal(40); - callback(null, null); - }) - } - ], - done - ); - }); + describe('.middleware(limit, maxLimit)', function () { + describe('should be a pure function with successive calls not mutating', function () { + + var firstMiddleware, secondMiddleware; + + beforeEach(function () { + firstMiddleware = paginate.middleware(10, 20); + secondMiddleware = paginate.middleware(30, 40); + }); + + it('limit in previous calls', function (done) { + async.parallel( + [ + function (callback) { + var req = reqres.req(); + firstMiddleware(req, reqres.res(), function (err) { + req.query.limit.should.equal(10); + callback(null, null); + }) + }, + function (callback) { + var req = reqres.req(); + secondMiddleware(req, reqres.res(), function (err) { + req.query.limit.should.equal(30); + callback(null, null); + }) + } + ], + done + ); + }); + + it('maxLimit in previous calls', function (done) { + async.parallel( + [ + function (callback) { + var req = reqres.req({ query: { limit: '100' } }); + firstMiddleware(req, reqres.res(), function (err) { + req.query.limit.should.equal(20); + callback(null, null); + }) + }, + function (callback) { + var req = reqres.req({ query: { limit: '100' } }); + secondMiddleware(req, reqres.res(), function (err) { + req.query.limit.should.equal(40); + callback(null, null); + }) + } + ], + done + ); }); + it('limit, skip and offset should be zero, page should be one', function (done) { + async.parallel( + [ + function (callback) { + var req = reqres.req({ query: { page: '0', limit: '0' } }); + firstMiddleware(req, reqres.res(), function (err) { + req.query.page.should.equal(1); + req.query.limit.should.equal(0); + req.skip.should.equal(0); + req.offset.should.equal(0); + callback(null, null); + }) + }, + function (callback) { + var req = reqres.req({ query: { page: '0', limit: '0' } }); + secondMiddleware(req, reqres.res(), function (err) { + req.query.page.should.equal(1); + req.query.limit.should.equal(0); + req.skip.should.equal(0); + req.offset.should.equal(0); + callback(null, null); + }) + }, + function (callback) { + var req = reqres.req({ query: { page: '-1', limit: '-1' } }); + firstMiddleware(req, reqres.res(), function (err) { + req.query.page.should.equal(1); + req.query.limit.should.equal(0); + req.skip.should.equal(0); + req.offset.should.equal(0); + callback(null, null); + }) + }, + function (callback) { + var req = reqres.req({ query: { page: '-10', limit: '-10' } }); + secondMiddleware(req, reqres.res(), function (err) { + req.query.page.should.equal(1); + req.query.limit.should.equal(0); + req.skip.should.equal(0); + req.offset.should.equal(0); + callback(null, null); + }) + }, + function (callback) { + var req = reqres.req({ query: { page: 'notInt', limit: 'notInt' } }); + firstMiddleware(req, reqres.res(), function (err) { + req.query.limit.should.equal(0); + req.query.page.should.equal(1); + req.skip.should.equal(0); + req.offset.should.equal(0); + callback(null, null); + }) + }, + function (callback) { + var req = reqres.req({ query: { page: 'notInt', limit: 'notInt' } }); + secondMiddleware(req, reqres.res(), function (err) { + req.query.limit.should.equal(0); + req.query.page.should.equal(1); + req.skip.should.equal(0); + req.offset.should.equal(0); + callback(null, null); + }) + } + ], + done + ); + }) + + }); + }); - describe('.getArrayPages(limit, pageCount, currentPaget)', function() { + describe('.getArrayPages(limit, pageCount, currentPaget)', function () { var pages; var limit = 5, fakeMaxCount = 10; - beforeEach(function() { + beforeEach(function () { this.req = { originalUrl: 'http://niftylettuce.com/', query: { @@ -165,50 +233,49 @@ describe('paginate', function() { } }; - pages = paginate.getArrayPages(this.req)(limit, fakeMaxCount, this.req.query.page); + pages = paginate.getArrayPages(this.req)(limit, fakeMaxCount, this.req.query.page); }); - it('should return arrays of pages', function() { + it('should return arrays of pages', function () { pages.should.be.a('array'); }); - it('it should contain correct page numbers and page urls', function() { - pages.forEach(function(p, i) { + it('it should contain correct page numbers and page urls', function () { + pages.forEach(function (p, i) { // index from 1 var idx = i + 1; p.number.should.equal(idx); - p.url.should.contain('page='+(idx)); + p.url.should.contain('page=' + (idx)); }) }) - it('it should contain correct page numbers and page urls', function() { - pages.forEach(function(p, i) { + it('it should contain correct page numbers and page urls', function () { + pages.forEach(function (p, i) { // index from 1 var idx = i + 1; p.number.should.equal(idx); - p.url.should.contain('page='+(idx)); + p.url.should.contain('page=' + (idx)); }) }) - it('check validations', function() { + it('check validations', function () { var contextReq = this.req; // negative limit - (function(){ + (function () { paginate.getArrayPages(contextReq)(-1, fakeMaxCount, contextReq.query.page); }).should.throw(/\>= 0/); // pageCount is string - (function(){ + (function () { paginate.getArrayPages(contextReq)(limit, '', contextReq.query.page); }).should.throw(/\>= 0/); // currentPage is object - (function(){ + (function () { paginate.getArrayPages(contextReq)(limit, fakeMaxCount, {}); }).should.throw(/\>= 0/); }) }) - });