Add feature to allow nested include and filtering relationships/associations #36

Open
labibramadhan wants to merge 8 commits from labibramadhan/include-with-filter into master
8 changed files with 202 additions and 80 deletions
Showing only changes of commit e632f79e2b - Show all commits

View File

@ -19,14 +19,14 @@ export default (server, a, b, names, options) => {
update(server, a, b, names);
};
export const get = (server, a, b, names) => {
export const get = async (server, a, b, names) => {
server.route({
method: 'GET',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.singular}/{bid}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.singular}/{bid}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const base = await a.findOne({
where: {
@ -51,14 +51,14 @@ export const get = (server, a, b, names) => {
});
};
export const list = (server, a, b, names) => {
export const list = async (server, a, b, names) => {
server.route({
method: 'GET',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.plural}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.plural}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const base = await a.findOne({
@ -77,16 +77,16 @@ export const list = (server, a, b, names) => {
});
};
export const scope = (server, a, b, names) => {
export const scope = async (server, a, b, names) => {
const scopes = Object.keys(b.options.scopes);
server.route({
method: 'GET',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.plural}/{scope}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.plural}/{scope}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const base = await a.findOne({
@ -116,7 +116,7 @@ export const scope = (server, a, b, names) => {
});
};
export const scopeScope = (server, a, b, names) => {
export const scopeScope = async (server, a, b, names) => {
const scopes = {
a: Object.keys(a.options.scopes),
b: Object.keys(b.options.scopes),
@ -124,11 +124,11 @@ export const scopeScope = (server, a, b, names) => {
server.route({
method: 'GET',
path: `${prefix}/${names.a.plural}/{scopea}/${names.b.plural}/{scopeb}`,
path: `${prefix}${names.a.plural}/{scopea}/${names.b.plural}/{scopeb}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const list = await b.scope(request.params.scopeb).findAll({
@ -152,14 +152,14 @@ export const scopeScope = (server, a, b, names) => {
});
};
export const destroy = (server, a, b, names) => {
export const destroy = async (server, a, b, names) => {
server.route({
method: 'DELETE',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.plural}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.plural}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const base = await a.findOne({
@ -179,16 +179,16 @@ export const destroy = (server, a, b, names) => {
});
};
export const destroyScope = (server, a, b, names) => {
export const destroyScope = async (server, a, b, names) => {
const scopes = Object.keys(b.options.scopes);
server.route({
method: 'DELETE',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.plural}/{scope}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.plural}/{scope}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const base = await a.findOne({
@ -221,14 +221,14 @@ export const destroyScope = (server, a, b, names) => {
});
};
export const update = (server, a, b, names) => {
export const update = async (server, a, b, names) => {
server.route({
method: 'PUT',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.plural}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.plural}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const base = await a.findOne({

View File

@ -14,14 +14,14 @@ export default (server, a, b, names, options) => {
update(server, a, b, names);
};
export const get = (server, a, b, names) => {
export const get = async (server, a, b, names) => {
server.route({
method: 'GET',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.singular}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.singular}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const base = await a.findOne({
@ -47,7 +47,7 @@ export const get = (server, a, b, names) => {
export const create = (server, a, b, names) => {
server.route({
method: 'POST',
path: `${prefix}/${names.a.singular}/{id}/${names.b.singular}`,
path: `${prefix}${names.a.singular}/{id}/${names.b.singular}`,
@error
async handler(request, reply) {
@ -67,14 +67,14 @@ export const create = (server, a, b, names) => {
});
};
export const destroy = (server, a, b, names) => {
export const destroy = async (server, a, b, names) => {
server.route({
method: 'DELETE',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.singular}/{bid}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.singular}/{bid}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const base = await a.findOne({
@ -99,11 +99,11 @@ export const destroy = (server, a, b, names) => {
export const update = (server, a, b, names) => {
server.route({
method: 'PUT',
path: `${prefix}/${names.a.singular}/{aid}/${names.b.singular}/{bid}`,
path: `${prefix}${names.a.singular}/{aid}/${names.b.singular}/{bid}`,
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const base = await a.findOne({

View File

@ -56,6 +56,17 @@ test('hasMany /team?include=players', async (t) => {
t.truthy(playerIds.includes(player2.id));
});
test('belongsTo with alias /player?include={"model": "Master", "as": "Coach"}', async(t) => {
const { server, instances } = t.context;
const { team1, master1 } = instances;
const path = `/player/${team1.id}?include={"model": "Master", "as": "Coach"}`;
const { result, statusCode } = await server.inject(path);
t.is(statusCode, STATUS_OK);
t.is(result.id, team1.id);
t.is(result.Coach.id, master1.id);
});
test('multiple includes /team?include=players&include=city', async(t) => {
const { server, instances } = t.context;
const { team1, player1, player2, city1 } = instances;
@ -86,6 +97,21 @@ test('multiple includes /team?include[]=players&include[]=city', async (t) => {
t.is(result.City.id, city1.id);
});
test('multiple includes /team?include[]=players&include[]={"model": "City"}', async(t) => {
const { server, instances } = t.context;
const { team1, player1, player2, city1 } = instances;
const path = `/team/${team1.id}?include[]=players&include[]={"model": "City"}`;
const { result, statusCode } = await server.inject(path);
t.is(statusCode, STATUS_OK);
t.is(result.id, team1.id);
const playerIds = result.Players.map(({ id }) => id);
t.truthy(playerIds.includes(player1.id));
t.truthy(playerIds.includes(player2.id));
t.is(result.City.id, city1.id);
});
test('inlcude filter /teams?include[]={"model": "City", "where": {"name": "Healdsburg"}}'
, async(t) => {
const { server } = t.context;
@ -95,3 +121,37 @@ test('inlcude filter /teams?include[]={"model": "City", "where": {"name": "Heald
const { statusCode } = await server.inject({ url, method });
t.is(statusCode, STATUS_OK);
});
test('nested inlcude filter ' +
'/city?include[]={"model": "Team", "include": {"model": "Player", "where": {"name": "Pinot"}}}'
, async(t) => {
const { instances, server } = t.context;
const { city1, team1, player2 } = instances;
// eslint-disable-next-line max-len
const url = '/city?include[]={"model": "Team", "include": {"model": "Player", "where": {"name": "Pinot"}}}';
const method = 'GET';
const { result, statusCode } = await server.inject({ url, method });
t.is(statusCode, STATUS_OK);
t.is(result.id, city1.id);
t.is(result.Teams[0].id, team1.id);
t.is(result.Teams[0].Players[0].id, player2.id);
});
test('nested inlcude filter ' +
'/city?include[]={"model": "Team", "include": {"model": "City", "where": {"name": "Healdsburg"}}}'
, async(t) => {
const { instances, server } = t.context;
const { city1, team1, team2 } = instances;
// eslint-disable-next-line max-len
const url = '/city?include[]={"model": "Team", "include": {"model": "City", "where": {"name": "Healdsburg"}}}';
const method = 'GET';
const { result, statusCode } = await server.inject({ url, method });
t.is(statusCode, STATUS_OK);
t.is(result.id, city1.id);
const teamIds = result.Teams.map(({ id }) => id);
t.truthy(teamIds.includes(team1.id));
t.truthy(teamIds.includes(team2.id));
});

View File

@ -56,6 +56,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi
const modelName = model._singular;
const modelAttributes = Object.keys(model.attributes);
const associatedModelNames = Object.keys(model.associations);
const associatedModelAliases = _.map(model.associations, (assoc => assoc.as));
const modelAssociations = [
...associatedModelNames,
..._.flatMap(associatedModelNames, (associationName) => {
@ -82,12 +83,13 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi
...attributeValidation,
...sequelizeOperators,
}),
as: joi.string().valid(...associatedModelAliases),
include: joi.any(), // @Todo: should validate the same as associationValidation var below
})
: joi.valid(null);
const associationValidation = {
include: [
joi.array().items(validAssociationsString),
joi.array().items(validAssociationsObject),
joi.array().items(validAssociationsString, validAssociationsObject),
validAssociationsString,
validAssociationsObject,
],
@ -161,14 +163,14 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi
}
};
export const list = ({ server, model, prefix = '/', config }) => {
export const list = async ({ server, model, prefix = '/', config }) => {
server.route({
method: 'GET',
path: path.join(prefix, model._plural),
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const { limit, offset } = parseLimitAndOffset(request);
const order = parseOrder(request);
@ -188,14 +190,14 @@ export const list = ({ server, model, prefix = '/', config }) => {
});
};
export const get = ({ server, model, prefix = '/', config }) => {
export const get = async ({ server, model, prefix = '/', config }) => {
server.route({
method: 'GET',
path: path.join(prefix, model._singular, '{id?}'),
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const { id } = request.params;
if (id) where[model.primaryKeyField] = id;
@ -212,14 +214,14 @@ export const get = ({ server, model, prefix = '/', config }) => {
});
};
export const scope = ({ server, model, prefix = '/', config }) => {
export const scope = async ({ server, model, prefix = '/', config }) => {
server.route({
method: 'GET',
path: path.join(prefix, model._plural, '{scope}'),
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
const { limit, offset } = parseLimitAndOffset(request);
const order = parseOrder(request);
@ -313,14 +315,14 @@ export const destroyAll = ({ server, model, prefix = '/', config }) => {
});
};
export const destroyScope = ({ server, model, prefix = '/', config }) => {
export const destroyScope = async ({ server, model, prefix = '/', config }) => {
server.route({
method: 'DELETE',
path: path.join(prefix, model._plural, '{scope}'),
@error
async handler(request, reply) {
const include = parseInclude(request);
const include = await parseInclude(request);
const where = parseWhere(request);
if (include instanceof Error) return void reply(include);

View File

@ -1,6 +1,7 @@
import { omit, identity, toNumber, isString, isUndefined } from 'lodash';
import { notImplemented } from 'boom';
import joi from 'joi';
import Promise from 'bluebird';
const sequelizeKeys = ['include', 'order', 'limit', 'offset'];
@ -17,7 +18,9 @@ const getModels = (request) => {
return models;
};
export const parseInclude = request => {
export const parseInclude = async(request) => {
if (typeof request.query.include === 'undefined') return [];
const include = Array.isArray(request.query.include)
joeybaker commented 2016-11-04 16:21:28 +00:00 (Migrated from github.com)
Review

JSON.parse will throw if not handed valid JSON. We should use a try/catch here.

`JSON.parse` will throw if not handed valid JSON. We should use a try/catch here.
labibramadhan commented 2016-11-07 01:56:57 +00:00 (Migrated from github.com)
Review

what code inside the catch do you prefer?

what code inside the catch do you prefer?
joeybaker commented 2016-11-07 17:48:30 +00:00 (Migrated from github.com)
Review
https://github.com/mdibaiee/hapi-sequelize-crud/blob/5ba9d7d26100f1a1a82367097342a8c35d43a26f/src/utils.js#L50-L54 and https://github.com/mdibaiee/hapi-sequelize-crud/blob/5ba9d7d26100f1a1a82367097342a8c35d43a26f/src/utils.js#L78-L83 are both options. Thanks!
? request.query.include
labibramadhan commented 2016-11-08 01:29:34 +00:00 (Migrated from github.com)
Review

maybe like this @joeybaker?

maybe like this @joeybaker?
joeybaker commented 2016-11-08 17:16:04 +00:00 (Migrated from github.com)
Review

Kinda! The try/catch part is right, but the joi check should be a part of the validation. What you have here will always pass the if condition, because joi.string() always returns a truthy value.

What do you think of something like this in crud.js on line 77?

  const assocationJSONvaliation = joi.string().regex(/^\{.*?"model":.*?\}$/);
  const associationValidation = {
    include: [
      joi.array().items(validAssociations),
      joi.array().items(assocationJSONvaliation),
      validAssociations,
      assocationJSONvaliation,
    ],
  };
Kinda! The try/catch part is right, but the joi check should be a part of the validation. What you have here will always pass the `if` condition, because `joi.string()` always returns a truthy value. What do you think of something like this in `crud.js` on line 77? ``` js const assocationJSONvaliation = joi.string().regex(/^\{.*?"model":.*?\}$/); const associationValidation = { include: [ joi.array().items(validAssociations), joi.array().items(assocationJSONvaliation), validAssociations, assocationJSONvaliation, ], }; ```
labibramadhan commented 2016-11-09 01:48:34 +00:00 (Migrated from github.com)
Review

but with this code, it won't validate the "model" name and the "where" object isn't it?
if the JSON string doesn't parse good so it caught as a string, but later it won't pass because crud.js that i pushed has something like joi.string().valid(...modelAssociations)

but with this code, it won't validate the "**model**" name and the "**where**" object isn't it? if the JSON string doesn't parse good so it caught as a string, but later it won't pass because crud.js that i pushed has something like `joi.string().valid(...modelAssociations)`
labibramadhan commented 2016-11-09 01:52:20 +00:00 (Migrated from github.com)
Review

currently my version of crud.js has this to validate its association object:

joi.object().keys({
        model: joi.string().valid(...modelAssociations),
        where: joi.object().keys({
          ...attributeValidation,
          ...sequelizeOperators,
        }),
      })
currently my version of crud.js has this to validate its association object: ``` joi.object().keys({ model: joi.string().valid(...modelAssociations), where: joi.object().keys({ ...attributeValidation, ...sequelizeOperators, }), }) ```
joeybaker commented 2016-11-09 05:46:48 +00:00 (Migrated from github.com)
Review

I get it. I was opting for: "if you pass JSON, we're going to skip validating it … it's too complex to do correctly".

Here's the problem I see with:

joi.object().keys({
        model: joi.string().valid(...modelAssociations),
        where: joi.object().keys({
          ...attributeValidation,
          ...sequelizeOperators,
        }),
      })

It ensures you get a valid JS object to validate. But you won't. You'll get a string that contains JSON. That's why I was opting for the regex of a string. Did I get confused there?

I get it. I was opting for: "if you pass JSON, we're going to skip validating it … it's too complex to do correctly". Here's the problem I see with: ``` js joi.object().keys({ model: joi.string().valid(...modelAssociations), where: joi.object().keys({ ...attributeValidation, ...sequelizeOperators, }), }) ``` It ensures you get a valid JS object to validate. But you won't. You'll get a string that contains JSON. That's why I was opting for the regex of a string. Did I get confused there?
labibramadhan commented 2016-11-09 19:01:15 +00:00 (Migrated from github.com)
Review

sorry @joeybaker, but i still didn't get it, what the main purpose of adding the json validation you want? because i have tried several cases of using include parameter and i didn't find any include parameter validation problem

sorry @joeybaker, but i still didn't get it, what the main purpose of adding the json validation you want? because i have tried several cases of using include parameter and i didn't find any include parameter validation problem
: [request.query.include]
@ -26,32 +29,58 @@ export const parseInclude = request => {
const models = getModels(request);
if (models.isBoom) return models;
return include.map(b => {
const getModelInstance = includeItem => {
return new Promise(async(resolve) => {
if (includeItem) {
if (typeof includeItem !== 'object') {
const singluarOrPluralMatch = Object.keys(models).find((modelName) => {
const { _singular, _plural } = models[modelName];
return _singular === includeItem || _plural === includeItem;
});
if (singluarOrPluralMatch) {
return resolve(models[singluarOrPluralMatch]);
}
}
if (typeof includeItem === 'string' && models.hasOwnProperty(includeItem)) {
return resolve(models[includeItem]);
} else if (typeof includeItem === 'object') {
if (
typeof includeItem.model === 'string' &&
includeItem.model.length &&
models.hasOwnProperty(includeItem.model)
) {
includeItem.model = models[includeItem.model];
}
if (includeItem.hasOwnProperty('include')) {
includeItem.include = await getModelInstance(includeItem.include);
return resolve(includeItem);
} else {
return resolve(includeItem);
}
}
} else {
return resolve(includeItem);
}
});
};
const jsonValidation = joi.string().regex(/^\{.*?"model":.*?\}$/);
const includes = include.map(async(b) => {
let a = b;
try {
if (joi.string().regex(/^\{.*?"model":.*?\}$/)) {
if (!jsonValidation.validate(a).error) {
a = JSON.parse(b);
}
} catch (e) {
//
}
if (typeof a !== 'object') {
const singluarOrPluralMatch = Object.keys(models).find((modelName) => {
const { _singular, _plural } = models[modelName];
return _singular === a || _plural === a;
});
if (singluarOrPluralMatch) return models[singluarOrPluralMatch];
}
if (typeof a === 'string') return models[a];
if (a && typeof a.model === 'string' && a.model.length) {
a.model = models[a.model];
}
return a;
return getModelInstance(a);
}).filter(identity);
return await Promise.all(includes);
};
export const parseWhere = request => {

18
test/fixtures/models/master.js vendored Normal file
View File

@ -0,0 +1,18 @@
export default (sequelize, DataTypes) => {
return sequelize.define('Master', {
id: {
type: DataTypes.INTEGER,
primaryKey: true,
autoIncrement: true,
},
name: DataTypes.STRING,
}, {
classMethods: {
associate: (models) => {
models.Master.hasMany(models.Player, {
foreignKey: 'coachId'
});
},
},
});
};

View File

@ -14,6 +14,10 @@ export default (sequelize, DataTypes) => {
models.Player.belongsTo(models.Team, {
foreignKey: { name: 'teamId' },
});
models.Player.belongsTo(models.Master, {
foreignKey: 'coachId',
as: 'Coach',
labibramadhan commented 2016-11-09 18:52:09 +00:00 (Migrated from github.com)
Review

needed for association/relationship alias test

needed for association/relationship alias test
joeybaker commented 2016-11-09 23:36:02 +00:00 (Migrated from github.com)
Review

Can you not use Teams and Players? Just so we don't get an overly-complex mock schema?

Can you not use Teams and Players? Just so we don't get an overly-complex mock schema?
joeybaker commented 2016-11-10 01:22:07 +00:00 (Migrated from github.com)
Review

Ah! I see. Makes sense.

Ah! I see. Makes sense.
labibramadhan commented 2016-11-10 02:10:08 +00:00 (Migrated from github.com)
Review

is it ok then?

is it ok then?
joeybaker commented 2016-11-10 19:25:44 +00:00 (Migrated from github.com)
Review

Yup!

Yup!
});
},
},
scopes: {

View File

@ -14,6 +14,7 @@ const modelNames = [
{ Singluar: 'City', singular: 'city', Plural: 'Cities', plural: 'cities' },
{ Singluar: 'Team', singular: 'team', Plural: 'Teams', plural: 'teams' },
{ Singluar: 'Player', singular: 'player', Plural: 'Players', plural: 'players' },
{ Singluar: 'Master', singular: 'master', Plural: 'Masters', plural: 'masters' },
];
@ -55,16 +56,24 @@ export default (test) => {
});
test.beforeEach('create data', async(t) => {
const { Player, Team, City } = t.context.sequelize.models;
const { Player, Master, Team, City } = t.context.sequelize.models;
const city1 = await City.create({ name: 'Healdsburg' });
const team1 = await Team.create({ name: 'Baseballs', cityId: city1.id });
const team2 = await Team.create({ name: 'Footballs', cityId: city1.id });
const master1 = await Master.create({ name: 'Shifu' });
const master2 = await Master.create({ name: 'Oogway' });
const player1 = await Player.create({
name: 'Cat', teamId: team1.id, active: true,
name: 'Cat', teamId: team1.id, active: true, coachId: master1.id
});
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 };
const player2 = await Player.create({
name: 'Pinot', teamId: team1.id, coachId: master1.id
});
const player3 = await Player.create({
name: 'Syrah', teamId: team2.id, coachId: master2.id
});
t.context.instances = {
city1, team1, team2, player1, player2, player3, master1, master2
};
});
// kill the server so that we can exit and don't leak memory