From c289fb2ed4183ffffa0d3c27e7b3beb0c02a46d7 Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Mon, 31 Oct 2016 12:48:34 -0700 Subject: [PATCH] Feat ordering by associated models now works It's now possible to order by associated models. This technically might have worked before b/c we were parsing JSON sent to `order`, but I'm pretty sure it wouldn't actually work b/c we never grabbed the actual model to associate by. Regardless, this actually enables things and adds tests to prove it. Note: there's a sequelize bug that's poorly reported but definitely known where `order` with associated models can fail because the sql generated doesn't include a join. So, I added docs noting that and a `test.failing` so that we'll be notified when that bug is fixed and can remove the note. --- README.md | 16 +++++++ src/crud-list-order.integration.test.js | 52 ++++++++++++++++++++++ src/utils.js | 58 +++++++++++++++++-------- src/utils.test.js | 8 ++-- 4 files changed, 112 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index f9e11ef..bf52e64 100644 --- a/README.md +++ b/README.md @@ -172,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. diff --git a/src/crud-list-order.integration.test.js b/src/crud-list-order.integration.test.js index 4899382..d184164 100644 --- a/src/crud-list-order.integration.test.js +++ b/src/crud-list-order.integration.test.js @@ -75,7 +75,59 @@ test('/players?order[0]=name&order[0]=DESC', async (t) => { 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) => { diff --git a/src/utils.js b/src/utils.js index dae90c0..6f547b5 100644 --- a/src/utils.js +++ b/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') => { diff --git a/src/utils.test.js b/src/utils.test.js index bd91198..178bcf0 100644 --- a/src/utils.test.js +++ b/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) => { @@ -62,14 +63,13 @@ test('parseOrder returns order when a string', (t) => { }); 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'] ); });