2.9.1

Add integration tests for limit/offset

README.md

@@ -158,6 +158,7 @@ Team.findAll({order: ['name']})
 ```js
 // returns the teams ordered by the name column, descending
 // GET /teams?order[0]=name&order[0]=DESC
+// GET /teams?order=name%20DESC
 
 // results in a Sequelize query:
 Team.findAll({order: [['name', 'DESC']]})
@@ -171,6 +172,22 @@ Team.findAll({order: [['name', 'DESC']]})
 Team.findAll({order: [['name'], ['city']]})
 ```
 
+You can even order by associated models. Though there is a [sequelize bug](https://github.com/sequelize/sequelize/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20order%20join%20) that might prevent this from working properly. A workaround is to `&include` the model you're ordering by.
+```js
+// returns the players ordered by the team name
+// GET /players?order[0]={"model": "Team"}&order[0]=name
+
+// results in a Sequelize query:
+Player.findAll({order: [[{model: Team}, 'name']]})
+
+// if the above returns a Sequelize error: `No such column Team.name`,
+// you can work around this by forcing the join into the query:
+// GET /players?order[0]={"model": "Team"}&order[0]=name&include=team
+
+// results in a Sequelize query:
+Player.findAll({order: [[{model: Team}, 'name']], include: [Team]})
+```
+
 
 ## Authorization and other hooks
 You can use Hapi's [`ext` option](http://hapijs.com/api#route-options) to interact with the request both before and after this module does. This is useful if you want to enforce authorization, or modify the request before or after this module does. Hapi [has a full list of hooks](http://hapijs.com/api#request-lifecycle) you can use.

package.json

@@ -1,6 +1,6 @@
 {
   "name": "hapi-sequelize-crud",
-  "version": "2.7.3",
+  "version": "2.9.1",
   "description": "Hapi plugin that automatically generates RESTful API for CRUD",
   "main": "build/index.js",
   "config": {

src/crud-destroy.integration.test.js

@@ -4,6 +4,7 @@ import setup from '../test/integration-setup.js';
 
 const STATUS_OK = 200;
 const STATUS_NOT_FOUND = 404;
+const STATUS_BAD_REQUEST = 400;
 
 setup(test);
 
@@ -112,3 +113,65 @@ test('not found /notamodel', async (t) => {
   const { statusCode } = await server.inject({ url, method });
   t.is(statusCode, STATUS_NOT_FOUND);
 });
+
+test('destroyScope /players/returnsOne', async (t) => {
+  const { server, instances, sequelize: { models: { Player } } } = t.context;
+  const { player1, player2 } = instances;
+  // this doesn't exist in our fixtures
+  const url = '/players/returnsOne';
+  const method = 'DELETE';
+
+  const presentPlayers = await Player.findAll();
+  const playerIds = presentPlayers.map(({ id }) => id);
+  t.truthy(playerIds.includes(player1.id));
+  t.truthy(playerIds.includes(player2.id));
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  t.is(result.id, player1.id);
+
+  const nonDeletedPlayers = await Player.findAll();
+  t.is(nonDeletedPlayers.length, presentPlayers.length - 1);
+});
+
+test('destroyScope /players/returnsNone', async (t) => {
+  const { server, instances, sequelize: { models: { Player } } } = t.context;
+  const { player1, player2 } = instances;
+  // this doesn't exist in our fixtures
+  const url = '/players/returnsNone';
+  const method = 'DELETE';
+
+  const presentPlayers = await Player.findAll();
+  const playerIds = presentPlayers.map(({ id }) => id);
+  t.truthy(playerIds.includes(player1.id));
+  t.truthy(playerIds.includes(player2.id));
+
+  const { statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_NOT_FOUND);
+
+  const nonDeletedPlayers = await Player.findAll();
+  const nonDeletedPlayerIds = nonDeletedPlayers.map(({ id }) => id);
+  t.truthy(nonDeletedPlayerIds.includes(player1.id));
+  t.truthy(nonDeletedPlayerIds.includes(player2.id));
+});
+
+test('destroyScope invalid scope /players/invalid', async (t) => {
+  const { server, instances, sequelize: { models: { Player } } } = t.context;
+  const { player1, player2 } = instances;
+  // this doesn't exist in our fixtures
+  const url = '/players/invalid';
+  const method = 'DELETE';
+
+  const presentPlayers = await Player.findAll();
+  const playerIds = presentPlayers.map(({ id }) => id);
+  t.truthy(playerIds.includes(player1.id));
+  t.truthy(playerIds.includes(player2.id));
+
+  const { statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_BAD_REQUEST);
+
+  const nonDeletedPlayers = await Player.findAll();
+  const nonDeletedPlayerIds = nonDeletedPlayers.map(({ id }) => id);
+  t.truthy(nonDeletedPlayerIds.includes(player1.id));
+  t.truthy(nonDeletedPlayerIds.includes(player2.id));
+});

src/crud-list-limit-and-offset.integration.test.js

@@ -0,0 +1,94 @@
+import test from 'ava';
+import 'sinon-bluebird';
+import setup from '../test/integration-setup.js';
+
+const STATUS_OK = 200;
+const STATUS_NOT_FOUND = 404;
+
+setup(test);
+
+test('/players?limit=2', async (t) => {
+  const { server } = t.context;
+  const limit = 2;
+  const url = `/players?limit=${limit}`;
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  t.is(result.length, limit);
+});
+
+test('/players?limit=2&offset=1', async (t) => {
+  const { server } = t.context;
+  const limit = 2;
+  const url = `/players?limit=${limit}&offset=1`;
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  t.is(result.length, limit);
+});
+
+test('/players?limit=2&offset=2', async (t) => {
+  const { server } = t.context;
+  const limit = 2;
+  const url = `/players?limit=${limit}&offset=2`;
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  t.is(result.length, 1, 'with only 3 players, only get 1 back with an offset of 2');
+});
+
+test('/players?limit=2&offset=20', async (t) => {
+  const { server } = t.context;
+  const limit = 2;
+  const url = `/players?limit=${limit}&offset=20`;
+  const method = 'GET';
+
+  const { statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_NOT_FOUND, 'with a offset/limit greater than the data, returns a 404');
+});
+
+test('scope /players/returnsAll?limit=2', async (t) => {
+  const { server } = t.context;
+  const limit = 2;
+  const url = `/players/returnsAll?limit=${limit}`;
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  t.is(result.length, limit);
+});
+
+test('scope /players/returnsAll?limit=2&offset=1', async (t) => {
+  const { server } = t.context;
+  const limit = 2;
+  const url = `/players/returnsAll?limit=${limit}&offset=1`;
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  t.is(result.length, limit);
+});
+
+test('scope /players/returnsAll?limit=2&offset=2', async (t) => {
+  const { server } = t.context;
+  const limit = 2;
+  const url = `/players/returnsAll?limit=${limit}&offset=2`;
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  t.is(result.length, 1, 'with only 3 players, only get 1 back with an offset of 2');
+});
+
+test('scope /players/returnsAll?limit=2&offset=20', async (t) => {
+  const { server } = t.context;
+  const limit = 2;
+  const url = `/players/returnsAll?limit=${limit}&offset=20`;
+  const method = 'GET';
+
+  const { statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_NOT_FOUND, 'with a offset/limit greater than the data, returns a 404');
+});

src/crud-list-order.integration.test.js

@@ -0,0 +1,141 @@
+import test from 'ava';
+import 'sinon-bluebird';
+import setup from '../test/integration-setup.js';
+
+const STATUS_OK = 200;
+const STATUS_BAD_QUERY = 502;
+
+setup(test);
+
+test('/players?order=name', async (t) => {
+  const { server, instances } = t.context;
+  const { player1, player2, player3 } = instances;
+  const url = '/players?order=name';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  // this is the order we'd expect the names to be in
+  t.is(result[0].name, player1.name);
+  t.is(result[1].name, player2.name);
+  t.is(result[2].name, player3.name);
+});
+
+test('/players?order=name%20ASC', async (t) => {
+  const { server, instances } = t.context;
+  const { player1, player2, player3 } = instances;
+  const url = '/players?order=name%20ASC';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  // this is the order we'd expect the names to be in
+  t.is(result[0].name, player1.name);
+  t.is(result[1].name, player2.name);
+  t.is(result[2].name, player3.name);
+});
+
+test('/players?order=name%20DESC', async (t) => {
+  const { server, instances } = t.context;
+  const { player1, player2, player3 } = instances;
+  const url = '/players?order=name%20DESC';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  // this is the order we'd expect the names to be in
+  t.is(result[0].name, player3.name);
+  t.is(result[1].name, player2.name);
+  t.is(result[2].name, player1.name);
+});
+
+test('/players?order[]=name', async (t) => {
+  const { server, instances } = t.context;
+  const { player1, player2, player3 } = instances;
+  const url = '/players?order[]=name';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  // this is the order we'd expect the names to be in
+  t.is(result[0].name, player1.name);
+  t.is(result[1].name, player2.name);
+  t.is(result[2].name, player3.name);
+});
+
+test('/players?order[0]=name&order[0]=DESC', async (t) => {
+  const { server, instances } = t.context;
+  const { player1, player2, player3 } = instances;
+  const url = '/players?order[0]=name&order[0]=DESC';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  // this is the order we'd expect the names to be in
+  t.is(result[0].name, player3.name);
+  t.is(result[1].name, player2.name);
+  t.is(result[2].name, player1.name);
+});
+
+// multiple sorts
+test('/players?order[0]=active&order[0]=DESC&order[1]=name&order[1]=DESC', async (t) => {
+  const { server, instances } = t.context;
+  const { player1, player2, player3 } = instances;
+  const url = '/players?order[0]=name&order[0]=DESC&order[1]=teamId&order[1]=DESC';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  // this is the order we'd expect the names to be in
+  t.is(result[0].name, player3.name);
+  t.is(result[1].name, player2.name);
+  t.is(result[2].name, player1.name);
+});
+
+// this will fail b/c sequelize doesn't correctly do the join when you pass
+// an order. There are many issues for this:
+// eslint-disable-next-line
+// https://github.com/sequelize/sequelize/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20order%20join%20
+//
+// https://github.com/sequelize/sequelize/issues/5353 is a good example
+// if this test passes, that's great! Just remove the workaround note in the
+// docs
+// eslint-disable-next-line
+test.failing('sequelize bug /players?order[0]={"model":"Team"}&order[0]=name&order[0]=DESC', async (t) => {
+  const { server, instances } = t.context;
+  const { player1, player2, player3 } = instances;
+  const url = '/players?order[0]={"model":"Team"}&order[0]=name&order[0]=DESC';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  // this is the order we'd expect the names to be in
+  t.is(result[0].name, player3.name);
+  t.is(result[1].name, player1.name);
+  t.is(result[2].name, player2.name);
+});
+
+// b/c the above fails, this is a work-around
+test('/players?order[0]={"model":"Team"}&order[0]=name&order[0]=DESC&include=team', async (t) => {
+  const { server, instances } = t.context;
+  const { player1, player2, player3 } = instances;
+  const url = '/players?order[0]={"model":"Team"}&order[0]=name&order[0]=DESC&include=team';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  // this is the order we'd expect the names to be in
+  t.is(result[0].name, player3.name);
+  t.is(result[1].name, player1.name);
+  t.is(result[2].name, player2.name);
+});
+
+test('invalid column /players?order[0]=invalid', async (t) => {
+  const { server } = t.context;
+  const url = '/players?order[]=invalid';
+  const method = 'GET';
+
+  const { statusCode, result } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_BAD_QUERY);
+  t.truthy(result.message.includes('invalid'));
+});

src/crud-scope.integration.test.js

@@ -0,0 +1,40 @@
+import test from 'ava';
+import 'sinon-bluebird';
+import setup from '../test/integration-setup.js';
+
+const STATUS_OK = 200;
+const STATUS_NOT_FOUND = 404;
+const STATUS_BAD_REQUEST = 400;
+
+setup(test);
+
+test('/players/returnsOne', async (t) => {
+  const { server, instances } = t.context;
+  const { player1 } = instances;
+  const url = '/players/returnsOne';
+  const method = 'GET';
+
+  const { result, statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_OK);
+  t.is(result.length, 1);
+  t.truthy(result[0].id, player1.id);
+});
+
+test('/players/returnsNone', async (t) => {
+  const { server } = t.context;
+  const url = '/players/returnsNone';
+  const method = 'GET';
+
+  const { statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_NOT_FOUND);
+});
+
+test('invalid scope /players/invalid', async (t) => {
+  const { server } = t.context;
+  // this doesn't exist in our fixtures
+  const url = '/players/invalid';
+  const method = 'GET';
+
+  const { statusCode } = await server.inject({ url, method });
+  t.is(statusCode, STATUS_BAD_REQUEST);
+});

src/crud.js

@@ -165,6 +165,8 @@ export const list = ({ server, model, prefix = '/', config }) => {
         where, include, limit, offset, order,
       });
 
+      if (!list.length) return void reply(notFound('Nothing found.'));
+
       reply(list.map((item) => item.toJSON()));
     },
 
@@ -214,6 +216,8 @@ export const scope = ({ server, model, prefix = '/', config }) => {
         include, where, limit, offset, order,
       });
 
+      if (!list.length) return void reply(notFound('Nothing found.'));
+
       reply(list.map((item) => item.toJSON()));
     },
     config,
@@ -309,6 +313,8 @@ export const destroyScope = ({ server, model, prefix = '/', config }) => {
 
       const list = await model.scope(request.params.scope).findAll({ include, where });
 
+      if (!list.length) return void reply(notFound('Nothing found.'));
+
       await Promise.all(list.map(instance => instance.destroy()));
 
       const listAsJSON = list.map((item) => item.toJSON());

src/crud.test.js

@@ -225,7 +225,7 @@ test('crud#list handler with order', async (t) => {
 
   t.deepEqual(
     findAllArgs.order,
-    [request.query.order],
+    [[request.query.order]],
     'queries with the order as an array b/c that\'s what sequelize wants'
   );
 });

src/get-config-for-method.js

@@ -144,7 +144,7 @@ export default ({
       .keys({
         limit: joi.number().min(0).integer(),
         offset: joi.number().min(0).integer(),
-        order: joi.array(),
+        order: [joi.array(), joi.string()],
       }),
       get(methodConfig, 'validate.query')
     );

src/utils.js

@@ -3,21 +3,27 @@ import { notImplemented } from 'boom';
 
 const sequelizeKeys = ['include', 'order', 'limit', 'offset'];
 
+const getModels = (request) => {
+  const noGetDb = typeof request.getDb !== 'function';
+  const noRequestModels = !request.models;
+  if (noGetDb && noRequestModels) {
+    return notImplemented('`request.getDb` or `request.models` are not defined.'
+                   + 'Be sure to load hapi-sequelize before hapi-sequelize-crud.');
+  }
+
+  const { models } = noGetDb ? request : request.getDb();
+
+  return models;
+};
+
 export const parseInclude = request => {
   const include = Array.isArray(request.query.include)
     ? request.query.include
     : [request.query.include]
     ;
 
-  const noGetDb = typeof request.getDb !== 'function';
-  const noRequestModels = !request.models;
-
-  if (noGetDb && noRequestModels) {
-    return notImplemented('`request.getDb` or `request.models` are not defined.'
-                   + 'Be sure to load hapi-sequelize before hapi-sequelize-crud.');
-  }
-
-  const { models } = noGetDb ? request : request.getDb();
+  const models = getModels(request);
+  if (models.isBoom) return models;
 
   return include.map(a => {
     const singluarOrPluralMatch = Object.keys(models).find((modelName) => {
@@ -63,24 +69,40 @@ export const parseLimitAndOffset = (request) => {
   return out;
 };
 
+const parseOrderArray = (order, models) => {
+  return order.map((requestColumn) => {
+    if (Array.isArray(requestColumn)) {
+      return parseOrderArray(requestColumn, models);
+    }
+
+    let column;
+    try {
+      column = JSON.parse(requestColumn);
+    } catch (e) {
+      column = requestColumn;
+    }
+
+    if (column.model) column.model = models[column.model];
+
+    return column;
+  });
+};
+
 export const parseOrder = (request) => {
   const { order } = request.query;
 
   if (!order) return null;
 
+  const models = getModels(request);
+  if (models.isBoom) return models;
+
   // transform to an array so sequelize will escape the input for us and
   // maintain security. See http://docs.sequelizejs.com/en/latest/docs/querying/#ordering
-  if (isString(order)) return order.split(' ');
+  const requestOrderColumns = isString(order) ? [order.split(' ')] : order;
 
-  for (const key of Object.keys(order)) {
-    try {
-      order[key] = JSON.parse(order[key]);
-    } catch (e) {
-      //
-    }
-  }
+  const parsedOrder = parseOrderArray(requestOrderColumns, models);
 
-  return order;
+  return parsedOrder;
 };
 
 export const getMethod = (model, association, plural = true, method = 'get') => {

src/utils.test.js

@@ -2,7 +2,8 @@ import test from 'ava';
 import { parseLimitAndOffset, parseOrder, parseWhere } from './utils.js';
 
 test.beforeEach((t) => {
-  t.context.request = { query: {} };
+  const models = t.context.models = { User: {} };
+  t.context.request = { query: {}, models };
 });
 
 test('parseLimitAndOffset is a function', (t) => {
@@ -57,19 +58,18 @@ test('parseOrder returns order when a string', (t) => {
 
   t.deepEqual(
     parseOrder(request)
-    , [order]
+    , [[order]]
   );
 });
 
 test('parseOrder returns order when json', (t) => {
-  const { request } = t.context;
-  const order = [{ model: 'User' }, 'DESC'];
+  const { request,models } = t.context;
   request.query.order = [JSON.stringify({ model: 'User' }), 'DESC'];
   request.query.thing = 'hi';
 
   t.deepEqual(
     parseOrder(request)
-    , order
+    , [{ model: models.User }, 'DESC']
   );
 });
 

test/fixtures/models/player.js

@@ -7,6 +7,7 @@ export default (sequelize, DataTypes) => {
     },
     name: DataTypes.STRING,
     teamId: DataTypes.INTEGER,
+    active: DataTypes.BOOLEAN,
   }, {
     classMethods: {
       associate: (models) => {
@@ -15,5 +16,24 @@ export default (sequelize, DataTypes) => {
         });
       },
     },
+    scopes: {
+      returnsOne: {
+        where: {
+          active: true,
+        },
+      },
+      returnsNone: {
+        where: {
+          name: 'notaname',
+        },
+      },
+      returnsAll: {
+        where: {
+          name: {
+            $ne: 'notaname',
+          },
+        },
+      },
+    },
   });
 };

test/integration-setup.js

@@ -58,9 +58,13 @@ export default (test) => {
     const { Player, Team, City } = t.context.sequelize.models;
     const city1 = await City.create({ name: 'Healdsburg' });
     const team1 = await Team.create({ name: 'Baseballs', cityId: city1.id });
-    const player1 = await Player.create({ name: 'Pinot', teamId: team1.id });
-    const player2 = await Player.create({ name: 'Syrah', teamId: team1.id });
-    t.context.instances = { city1, team1, player1, player2 };
+    const team2 = await Team.create({ name: 'Footballs', cityId: city1.id });
+    const player1 = await Player.create({
+      name: 'Cat', teamId: team1.id, active: true,
+    });
+    const player2 = await Player.create({ name: 'Pinot', teamId: team1.id });
+    const player3 = await Player.create({ name: 'Syrah', teamId: team2.id });
+    t.context.instances = { city1, team1, team2, player1, player2, player3 };
   });
 
   // kill the server so that we can exit and don't leak memory