thereadme/hapi-sequelize-crud
1
0
Fork 0
Code Issues 4 Pull Requests 1 Projects Releases Wiki Activity

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
pull from: labibramadhan/include-with-filter
merge into: thereadme:master
Conversation 9 Commits 8 Files Changed 9 +111 -61
3 changed files with 111 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 05793eb749 - Show all commits

50
README.md
View File

@ -112,9 +112,14 @@ Getting related models is easy, just use a query parameter `include`.
// GET /teams?include=city or
// GET /teams?include={"model": "City"}
// results in a Sequelize query:
Team.findAll({include: City})
or if association defined with an alias
// GET /players?include={"model": "Master", "as": "Couch"}
// results in a Sequelize query:
Players.findAll({include: Master, as: 'Couch'})
mdibaiee commented 2016-11-04 11:40:15 +00:00 (Migrated from github.com)
Review
Copy Link

Please get rid of the include key of the JSON passed to include, it's an unnecessary repetition.

It should look like this:
/teams?include={"model": "City", "where": { "name": "Healdsburg" }}

Please get rid of the `include` key of the JSON passed to `include`, it's an unnecessary repetition. It should look like this: `/teams?include={"model": "City", "where": { "name": "Healdsburg" }}`
labibramadhan commented 2016-11-07 02:03:04 +00:00 (Migrated from github.com)
Review
Copy Link

Fixed in my last commit, could you verify now?

Fixed in my last commit, could you verify now?
```
If you want to get multiple related models, just pass multiple `include` parameters.
@ -135,7 +140,7 @@ For models that have a many-to-many relationship, you can also pass the plural v
Team.findAll({include: [Player]})
```
Filtering by related models property, you can pass **where** paremeter inside each **include** item(s) object.
Filtering by related models property, you can pass **where** paremeter inside each **include** items object.
```js
// returns all team with their related City where City property name equals Healdsburg
// GET /teams?include={"model": "City", "where": {"name": "Healdsburg"}}
@ -144,6 +149,47 @@ Filtering by related models property, you can pass **where** paremeter inside ea
Team.findAll({include: {model: City, where: {name: 'Healdsburg'}}})
```
More complex example with nested include, association alias and association filtering.
```js
// returns all team with its players along with its couch of each player
// GET /cities?include[]={
// "model": "Team",
// "include": {
// "model": "Player",
// "where": {
// "name": "Pinot"
// },
// "include": {
// "model": "Master",
// "as": "Coach",
// "where": {
// "name": "Shifu"
// }
// }
// }
// }
// results in a Sequelize query:
City.findAll({
include: {
model: Team,
include: {
model: Player,
where: {
name: Pinot
},
include: {
model: Master,
as: 'Coach',
where: {
name: 'Shifu'
}
}
}
}
})
```
## `limit` and `offset` queries
Restricting list (`GET`) and scope queries to a restricted count can be done by passing `limit=<number>` and/or `offset=<number>`.

47
src/crud-include.integration.test.js
View File

@ -112,7 +112,7 @@ test('multiple includes /team?include[]=players&include[]={"model": "City"}', as
t.is(result.City.id, city1.id);
});
test('inlcude filter /teams?include[]={"model": "City", "where": {"name": "Healdsburg"}}'
test('include filter /teams?include[]={"model": "City", "where": {"name": "Healdsburg"}}'
, async(t) => {
const { server } = t.context;
const url = '/teams?include[]={"model": "City", "where": {"name": "Healdsburg"}}';
@ -122,29 +122,14 @@ test('inlcude filter /teams?include[]={"model": "City", "where": {"name": "Heald
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"}}}'
test('nested include filter ' +
'/citiy?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 url = '/city?include[]=' +
'{"model": "Team", "include": {"model": "City", "where": {"name": "Healdsburg"}}}';
const method = 'GET';
const { result, statusCode } = await server.inject({ url, method });
@ -155,3 +140,23 @@ test('nested inlcude filter ' +
t.truthy(teamIds.includes(team1.id));
t.truthy(teamIds.includes(team2.id));
});
test('complex include ' +
'/cities?include[]={"model":"Team", ' +
'"include":{ "model":"Player", "where":{"name": "Pinot"}, ' +
'"include":{ "model":"Master", "as":"Coach", "where":{"name": "Shifu"}}}}'
, async(t) => {
const { instances, server } = t.context;
const { city1, master1, player2, team1 } = instances;
const method = 'GET';
const url = '/cities?include[]={"model":"Team", ' +
'"include":{ "model":"Player", "where":{"name": "Pinot"}, ' +
'"include":{ "model":"Master", "as":"Coach", "where":{"name": "Shifu"}}}}';
const { result, statusCode } = await server.inject({ url, method });
t.is(statusCode, STATUS_OK);
t.is(result[0].id, city1.id);
t.is(result[0].Teams[0].id, team1.id);
t.is(result[0].Teams[0].Players[0].id, player2.id);
t.is(result[0].Teams[0].Players[0].Coach.id, master1.id);
});

75
src/utils.js
View File

@ -18,6 +18,42 @@ const getModels = (request) => {
return models;
};
const getModelInstance = (models, includeItem) => {
return new Promise(async(resolve) => {
if (includeItem) {
if (typeof includeItem !== 'object') {
joeybaker commented 2016-11-04 16:21:28 +00:00 (Migrated from github.com)
Review
Copy Link

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
Copy Link

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
Copy Link

5ba9d7d261/src/utils.js (L50-L54) and 5ba9d7d261/src/utils.js (L78-L83) are both options. Thanks!

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!
const singluarOrPluralMatch = Object.keys(models).find((modelName) => {
labibramadhan commented 2016-11-08 01:29:34 +00:00 (Migrated from github.com)
Review
Copy Link

maybe like this @joeybaker?

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

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
Copy Link

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
Copy Link

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
Copy Link

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
Copy Link

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
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(models, includeItem.include);
return resolve(includeItem);
} else {
return resolve(includeItem);
}
}
}
return resolve(includeItem);
});
};
export const parseInclude = async(request) => {
if (typeof request.query.include === 'undefined') return [];
@ -29,43 +65,6 @@ export const parseInclude = async(request) => {
const models = getModels(request);
if (models.isBoom) return models;
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;
@ -77,7 +76,7 @@ export const parseInclude = async(request) => {
//
}
return getModelInstance(a);
return getModelInstance(models, a);
}).filter(identity);
return await Promise.all(includes);