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 +63 -39
4 changed files with 63 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 72452a0088 - Show all commits

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

@ -85,3 +85,13 @@ test('multiple includes /team?include[]=players&include[]=city', async (t) => {
t.truthy(playerIds.includes(player2.id)); t.truthy(playerIds.includes(player2.id));
t.is(result.City.id, city1.id); t.is(result.City.id, city1.id);
}); });
test('inlcude filter /teams?include[]={"model": "City", "where": {"name": "Healdsburg"}}'
, async (t) => {
const { server } = t.context;
const url = '/teams?include[]={"model": "City", "where": {"name": "Healdsburg"}}';
const method = 'GET';
const { statusCode } = await server.inject({ url, method });
t.is(statusCode, STATUS_OK);
});

39
src/crud.js
View File

@ -13,7 +13,7 @@ const createAll = ({
prefix, prefix,
config, config,
attributeValidation, attributeValidation,
associationValidation, modelAssociations,
scopes, scopes,
}) => { }) => {
Object.keys(methods).forEach((method) => { Object.keys(methods).forEach((method) => {
@ -24,7 +24,7 @@ const createAll = ({
config: getConfigForMethod({ config: getConfigForMethod({
method, method,
attributeValidation, attributeValidation,
associationValidation, modelAssociations,
config, config,
scopes, scopes,
}), }),
@ -35,22 +35,22 @@ const createAll = ({
export { associations }; export { associations };
/* /*
The `models` option, becomes `permissions`, and can look like: The `models` option, becomes `permissions`, and can look like:
``` ```
models: ['cat', 'dog'] models: ['cat', 'dog']
``` ```
or or
``` ```
models: { models: {
cat: ['list', 'get'] cat: ['list', 'get']
, dog: true // all , dog: true // all
} }
``` ```
*/ */
export default (server, model, { prefix, defaultConfig: config, models: permissions }) => { export default (server, model, { prefix, defaultConfig: config, models: permissions }) => {
const modelName = model._singular; const modelName = model._singular;
@ -71,13 +71,6 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi
return params; return params;
}, {}); }, {});
const validAssociations = modelAssociations.length
? joi.string().valid(...modelAssociations)
: joi.valid(null);
const associationValidation = {
include: [joi.array().items(validAssociations), validAssociations],
};
const scopes = Object.keys(model.options.scopes); const scopes = Object.keys(model.options.scopes);
// if we don't have any permissions set, just create all the methods // if we don't have any permissions set, just create all the methods
@ -88,7 +81,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi
prefix, prefix,
config, config,
attributeValidation, attributeValidation,
associationValidation, modelAssociations,
scopes, scopes,
}); });
// if permissions are set, but we can't parse them, throw an error // if permissions are set, but we can't parse them, throw an error
@ -103,7 +96,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi
prefix, prefix,
config, config,
attributeValidation, attributeValidation,
associationValidation, modelAssociations,
scopes, scopes,
}); });
// if we've gotten here, we have complex permissions and need to set them // if we've gotten here, we have complex permissions and need to set them
@ -125,7 +118,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi
config: getConfigForMethod({ config: getConfigForMethod({
method, method,
attributeValidation, attributeValidation,
associationValidation, modelAssociations,
scopes, scopes,
config: permissionConfig, config: permissionConfig,
}), }),
@ -137,7 +130,7 @@ export default (server, model, { prefix, defaultConfig: config, models: permissi
model, model,
prefix, prefix,
attributeValidation, attributeValidation,
associationValidation, modelAssociations,
scopes, scopes,
config: permissionConfig, config: permissionConfig,
}); });

22
src/get-config-for-method.js
View File

@ -72,7 +72,7 @@ export const restrictMethods = [
]; ];
export default ({ export default ({
method, attributeValidation, associationValidation, scopes = [], config = {}, method, attributeValidation, modelAssociations, scopes = [], config = {},
}) => { }) => {
const hasWhere = whereMethods.includes(method); const hasWhere = whereMethods.includes(method);
const hasInclude = includeMethods.includes(method); const hasInclude = includeMethods.includes(method);
@ -96,9 +96,27 @@ export default ({
} }
if (hasInclude) { if (hasInclude) {
const modelsHasAssociations = modelAssociations && modelAssociations.length;
const validAssociationsString = modelsHasAssociations
? joi.string().valid(...modelAssociations)
: joi.valid(null);
const validAssociationsObject = modelsHasAssociations
? joi.object().keys({
model: joi.string().valid(...modelAssociations),
where: joi.object().keys({
...attributeValidation,
...sequelizeOperators,
}),
})
: joi.valid(null);
const query = concatToJoiObject(joi.object() const query = concatToJoiObject(joi.object()
.keys({ .keys({
...associationValidation, include: [
joi.array().items(validAssociationsString),
joi.array().items(validAssociationsObject),
validAssociationsString,
validAssociationsObject,
],
}), }),
get(methodConfig, 'validate.query') get(methodConfig, 'validate.query')
); );

5
src/utils.js
View File

@ -25,13 +25,16 @@ export const parseInclude = request => {
const models = getModels(request); const models = getModels(request);
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
if (models.isBoom) return models; if (models.isBoom) return models;
return include.map(a => { return include.map(b => {
const a = /^{.*}$/.test(b) ? JSON.parse(b) : b;
if (typeof a !== 'object') {
const singluarOrPluralMatch = Object.keys(models).find((modelName) => { const singluarOrPluralMatch = Object.keys(models).find((modelName) => {
const { _singular, _plural } = models[modelName]; const { _singular, _plural } = models[modelName];
return _singular === a || _plural === a; return _singular === a || _plural === a;
}); });
if (singluarOrPluralMatch) return models[singluarOrPluralMatch]; if (singluarOrPluralMatch) return models[singluarOrPluralMatch];
}
if (typeof a === 'string') return models[a]; if (typeof a === 'string') return models[a];