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

2016-11-04T05:25:07Z

labibramadhan commented

2016-11-04 05:25:07 +00:00

(Migrated from github.com)

Added feature to allow filtering relationships/associations based on http://docs.sequelizejs.com/en/latest/docs/querying/#relations-associations

labibramadhan commented

2016-11-04 05:28:50 +00:00

(Migrated from github.com)

oops sorry, forgot to fix associationValidation of get-config-for-method.test.js

oops sorry, forgot to fix **associationValidation** of **get-config-for-method.test.js**

mdibaiee commented

2016-11-04 06:13:52 +00:00

(Migrated from github.com)

@labibramadhan Thanks Muhammad. May you please document the changes in README, too? The code looks fine to me (though there are lots of space changes, I'm not sure if this style persists throughout the whole codebase) but @joeybaker should take a look, too. 👍

@labibramadhan Thanks Muhammad. May you please document the changes in README, too? The code looks fine to me (though there are lots of space changes, I'm not sure if this style persists throughout the whole codebase) but @joeybaker should take a look, too. :+1:

codecov-io commented

2016-11-04 07:24:43 +00:00

(Migrated from github.com)

Current coverage is 72.50% (diff: 79.36%)

Merging #36 into master will increase coverage by 9.12%

@@             master        #36   diff @@
==========================================
  Files             9          9          
  Lines           456        480    +24   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            289        348    +59   
+ Misses          167        132    -35   
  Partials          0          0

Powered by Codecov. Last update 5ba9d7d...05793eb

## [Current coverage](https://codecov.io/gh/mdibaiee/hapi-sequelize-crud/pull/36?src=pr) is 72.50% (diff: 79.36%) > Merging [#36](https://codecov.io/gh/mdibaiee/hapi-sequelize-crud/pull/36?src=pr) into [master](https://codecov.io/gh/mdibaiee/hapi-sequelize-crud/branch/master?src=pr) will increase coverage by **9.12%** ``` diff @@ master #36 diff @@ ========================================== Files 9 9 Lines 456 480 +24 Methods 0 0 Messages 0 0 Branches 0 0 ========================================== + Hits 289 348 +59 + Misses 167 132 -35 Partials 0 0 ``` > Powered by [Codecov](https://codecov.io?src=pr). Last update [5ba9d7d...05793eb](https://codecov.io/gh/mdibaiee/hapi-sequelize-crud/compare/5ba9d7d26100f1a1a82367097342a8c35d43a26f...05793eb7499a1393058a2e35b2c46aefa3994345?src=pr)

labibramadhan commented

2016-11-04 07:29:48 +00:00

(Migrated from github.com)

Hi @mdibaiee, i have added some guide text about this feature in README, please have a check 😃

Hi @mdibaiee, i have added some guide text about this feature in README, please have a check :smiley:

mdibaiee (Migrated from github.com) requested changes 2016-11-04 11:41:59 +00:00

README.md

mdibaiee (Migrated from github.com) commented

2016-11-04 11:40:47 +00:00

Why remove this section? 🤔

README.md

						
				@ -116,0 +119,4 @@

				// GET /players?include={"model": "Master", "as": "Couch"}

				// results in a Sequelize query:

				Players.findAll({include: Master, as: 'Couch'})

mdibaiee (Migrated from github.com) commented

2016-11-04 11:40:15 +00:00

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" }}`

joeybaker (Migrated from github.com) requested changes 2016-11-04 16:27:02 +00:00

joeybaker (Migrated from github.com) left a comment

Thanks for the work @labibramadhan!

Generally:

I don't think we need to make changes to get-config-for-method.js. You should be able to do all your changes in crud.js
Given that, I think all you really need to do is add a integration test or two, no need to change the unit tests for get-config-for-method
The joi validation should try to validate that it received a JSON string right? Given that. I think your best bet is something like: joi.string().regex(/^\{.*?"model":.*?\}$/)

Let us know if you have questions, and if you don't have time to get to this, that's okay too!

Thanks for the work @labibramadhan! Generally: - I don't think we need to make changes to `get-config-for-method.js`. You should be able to do all your changes in `crud.js` - Given that, I think all you really need to do is add a integration test or two, no need to change the unit tests for `get-config-for-method` - The joi validation should try to validate that it received a JSON string right? Given that. I think your best bet is something like: `joi.string().regex(/^\{.*?"model":.*?\}$/)` Let us know if you have questions, and if you don't have time to get to this, that's okay too!

src/crud.js

joeybaker (Migrated from github.com) commented

2016-11-04 16:24:13 +00:00

Why move this validation into get-config-for-method? All other validation is defined in this method already.

Why move this validation into `get-config-for-method`? All other validation is defined in this method already.

src/get-config-for-method.js

joeybaker (Migrated from github.com) commented

2016-11-04 16:20:39 +00:00

hmmm… it's weird that we're adding in custom validation here instead of just trusting that the associationValidation is correct when we received it. We should fix this at the source in crud.js

hmmm… it's weird that we're adding in custom validation here instead of just trusting that the `associationValidation` is correct when we received it. We should fix this at the source in `crud.js`

src/utils.js

						
				@ -20,0 +21,4 @@

				const getModelInstance = (models, includeItem) => {

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

joeybaker (Migrated from github.com) commented

2016-11-04 16:21:28 +00:00

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 (Migrated from github.com) reviewed 2016-11-07 01:56:57 +00:00

src/utils.js

						
				@ -20,0 +21,4 @@

				const getModelInstance = (models, includeItem) => {

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

labibramadhan (Migrated from github.com) commented

2016-11-07 01:56:57 +00:00

what code inside the catch do you prefer?

labibramadhan (Migrated from github.com) reviewed 2016-11-07 01:57:48 +00:00

src/crud.js

labibramadhan (Migrated from github.com) commented

2016-11-07 01:57:48 +00:00

Sorry @joeybaker i have fixed this, moved to crud.js

labibramadhan commented

2016-11-07 01:59:29 +00:00

(Migrated from github.com)

By the way i got different indentation settings with your existing scripts, is it OK? what IDE you guys using?

labibramadhan (Migrated from github.com) reviewed 2016-11-07 02:01:57 +00:00

README.md

labibramadhan (Migrated from github.com) commented

2016-11-07 02:01:57 +00:00

Hi @mdibaiee, sorry for this README.md lines that i've removed before, i thought it was for relationship query example, but i just realized maybe it was for object property method filter, but it's now in README.md again

labibramadhan (Migrated from github.com) reviewed 2016-11-07 02:03:04 +00:00

README.md

						
				@ -116,0 +119,4 @@

				// GET /players?include={"model": "Master", "as": "Couch"}

				// results in a Sequelize query:

				Players.findAll({include: Master, as: 'Couch'})

labibramadhan (Migrated from github.com) commented

2016-11-07 02:03:04 +00:00

Fixed in my last commit, could you verify now?

mdibaiee commented

2016-11-07 12:27:02 +00:00

(Migrated from github.com)

@labibramadhan we use two spaces for indentation, almost all IDEs and editors support indentation settings. Thanks for looking at the mentioned issues.

mdibaiee (Migrated from github.com) approved these changes 2016-11-07 12:27:45 +00:00

joeybaker (Migrated from github.com) reviewed 2016-11-07 17:48:30 +00:00

src/utils.js

						
				@ -20,0 +21,4 @@

				const getModelInstance = (models, includeItem) => {

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

joeybaker (Migrated from github.com) commented

2016-11-07 17:48:30 +00:00

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!

labibramadhan commented

2016-11-08 01:25:46 +00:00

(Migrated from github.com)

anytime @mdibaiee, thank you too @mdibaiee and @joeybaker for this amazing module! i'll give this module a try to my project, but i need to find ACL module to go along, any suggestion?

😆 2

labibramadhan (Migrated from github.com) reviewed 2016-11-08 01:29:34 +00:00

src/utils.js

						
				@ -20,0 +22,4 @@

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

				        const singluarOrPluralMatch = Object.keys(models).find((modelName) => {

labibramadhan (Migrated from github.com) commented

2016-11-08 01:29:34 +00:00

maybe like this @joeybaker?

joeybaker (Migrated from github.com) reviewed 2016-11-08 17:16:04 +00:00

src/utils.js

						
				@ -20,0 +22,4 @@

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

				        const singluarOrPluralMatch = Object.keys(models).find((modelName) => {

joeybaker (Migrated from github.com) commented

2016-11-08 17:16:04 +00:00

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 (Migrated from github.com) reviewed 2016-11-09 01:48:34 +00:00

src/utils.js

						
				@ -20,0 +22,4 @@

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

				        const singluarOrPluralMatch = Object.keys(models).find((modelName) => {

labibramadhan (Migrated from github.com) commented

2016-11-09 01:48:34 +00:00

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 (Migrated from github.com) reviewed 2016-11-09 01:52:20 +00:00

src/utils.js

						
				@ -20,0 +22,4 @@

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

				        const singluarOrPluralMatch = Object.keys(models).find((modelName) => {

labibramadhan (Migrated from github.com) commented

2016-11-09 01:52:20 +00:00

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 (Migrated from github.com) reviewed 2016-11-09 05:46:48 +00:00

src/utils.js

						
				@ -20,0 +22,4 @@

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

				        const singluarOrPluralMatch = Object.keys(models).find((modelName) => {

joeybaker (Migrated from github.com) commented

2016-11-09 05:46:48 +00:00

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?

joeybaker commented

2016-11-09 05:49:45 +00:00

(Migrated from github.com)

@labibramadhan google tells me that ACL = "access control list", which I take to mean you want find-grain user permissions over REST access?

If so: we're solving this in 2 ways: using the built-in Hapi auth and adding it to our REST routes in defaultConfig, and via scopes.

@labibramadhan google tells me that ACL = "access control list", which I take to mean you want find-grain user permissions over REST access? If so: we're solving this in 2 ways: using the built-in [Hapi auth](http://hapijs.com/tutorials/auth?lang=en_US) and adding it to our REST routes in `defaultConfig`, and via scopes.

labibramadhan (Migrated from github.com) reviewed 2016-11-09 18:50:21 +00:00

src/utils.js

labibramadhan (Migrated from github.com) commented

2016-11-09 18:50:21 +00:00

since nested includes feature may loop the getModelInstance function, parseInclude is now a promise based function, to call this function needs await

since nested includes feature may loop the **getModelInstance** function, **parseInclude** is now a promise based function, to call this function needs **await**

labibramadhan (Migrated from github.com) reviewed 2016-11-09 18:52:09 +00:00

test/fixtures/models/player.js

						
				@ -16,1 +16,4 @@

				        });

				        models.Player.belongsTo(models.Master, {

				          foreignKey: 'coachId',

				          as: 'Coach',

labibramadhan (Migrated from github.com) commented

2016-11-09 18:52:09 +00:00

needed for association/relationship alias test

labibramadhan (Migrated from github.com) reviewed 2016-11-09 18:54:00 +00:00

src/crud.js

labibramadhan (Migrated from github.com) commented

2016-11-09 18:54:00 +00:00

@joeybaker and @mdibaiee, i need help to apply include child joi validation to be the same as its parent include validation (nested validations with the same schema)

labibramadhan (Migrated from github.com) reviewed 2016-11-09 18:55:16 +00:00

src/associations/one-to-many.js

labibramadhan (Migrated from github.com) commented

2016-11-09 18:55:16 +00:00

this things was formatted ${prefix}/${names.a.singular}/{aid}/${names.b.plural}, but if prefix equals "/", it will produce path like //team/{aid}/player and will output 404 not found response, i decided to remove the slash after ${prefix}, so every prefix option definition must have trailing slash

this things was formatted `${prefix}/${names.a.singular}/{aid}/${names.b.plural}`, but if prefix equals "/", it will produce path like `//team/{aid}/player` and will output `404 not found` response, i decided to remove the slash after `${prefix}`, so every prefix option definition must have trailing slash

labibramadhan (Migrated from github.com) reviewed 2016-11-09 19:01:15 +00:00

src/utils.js

						
				@ -20,0 +22,4 @@

				  return new Promise(async(resolve) => {

				    if (includeItem) {

				      if (typeof includeItem !== 'object') {

				        const singluarOrPluralMatch = Object.keys(models).find((modelName) => {

labibramadhan (Migrated from github.com) commented

2016-11-09 19:01:15 +00:00

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

labibramadhan commented

2016-11-09 19:03:03 +00:00

(Migrated from github.com)

alright @joeybaker, i will try that method, thank you!

joeybaker (Migrated from github.com) reviewed 2016-11-09 23:36:02 +00:00

test/fixtures/models/player.js

						
				@ -16,1 +16,4 @@

				        });

				        models.Player.belongsTo(models.Master, {

				          foreignKey: 'coachId',

				          as: 'Coach',

joeybaker (Migrated from github.com) commented

2016-11-09 23:36:02 +00:00

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

joeybaker (Migrated from github.com) reviewed 2016-11-09 23:37:15 +00:00

src/crud-include.integration.test.js

						
				@ -8,3 +8,3 @@

				test('belongsTo /team?include=city', async (t) => {

				test('belongsTo /team?include=city', async(t) => {

				  const { server, instances } = t.context;

joeybaker (Migrated from github.com) commented

2016-11-09 23:37:15 +00:00

Why remove all these spaces?

joeybaker (Migrated from github.com) reviewed 2016-11-09 23:44:30 +00:00

src/crud.js

joeybaker (Migrated from github.com) commented

2016-11-09 23:44:30 +00:00

So… I'm still confused. How would we be getting an Object in here? Is joi smart enough to parse the JSON string?

joeybaker (Migrated from github.com) reviewed 2016-11-10 01:21:50 +00:00

src/utils.js

joeybaker (Migrated from github.com) commented

2016-11-10 01:21:50 +00:00

hmmm… so two questions:

Why does it need to be async? Would a simple recurse do the job?
getModelInstance looks really dense, maybe it should be it's own method instead of a closure? It would allow us to unit test it, which might be a good idea.

hmmm… so two questions: 1. Why does it need to be async? Would a simple recurse do the job? 2. `getModelInstance` looks really dense, maybe it should be it's own method instead of a closure? It would allow us to unit test it, which might be a good idea.

joeybaker (Migrated from github.com) reviewed 2016-11-10 01:22:08 +00:00

test/fixtures/models/player.js

						
				@ -16,1 +16,4 @@

				        });

				        models.Player.belongsTo(models.Master, {

				          foreignKey: 'coachId',

				          as: 'Coach',

joeybaker (Migrated from github.com) commented

2016-11-10 01:22:07 +00:00

Ah! I see. Makes sense.

labibramadhan (Migrated from github.com) reviewed 2016-11-10 02:09:20 +00:00

src/utils.js

labibramadhan (Migrated from github.com) commented

2016-11-10 02:09:20 +00:00

because i have tried not to use promise while looping, and it didn't work so i gotta use async. yes you're right, better to separate getModelInstance function, i will refactor it

labibramadhan (Migrated from github.com) reviewed 2016-11-10 02:10:08 +00:00

test/fixtures/models/player.js

						
				@ -16,1 +16,4 @@

				        });

				        models.Player.belongsTo(models.Master, {

				          foreignKey: 'coachId',

				          as: 'Coach',

labibramadhan (Migrated from github.com) commented

2016-11-10 02:10:08 +00:00

is it ok then?

labibramadhan (Migrated from github.com) reviewed 2016-11-10 02:10:42 +00:00

src/crud-include.integration.test.js

						
				@ -8,3 +8,3 @@

				test('belongsTo /team?include=city', async (t) => {

				test('belongsTo /team?include=city', async(t) => {

				  const { server, instances } = t.context;

labibramadhan (Migrated from github.com) commented

2016-11-10 02:10:42 +00:00

sorry this is from my IDE settings, but i don't see any eslint error

labibramadhan (Migrated from github.com) reviewed 2016-11-10 02:12:42 +00:00

src/crud.js

labibramadhan (Migrated from github.com) commented

2016-11-10 02:12:42 +00:00

i think the only answer is we need to try. if it's not smart enough, maybe later we will find bug report

joeybaker (Migrated from github.com) reviewed 2016-11-10 19:25:20 +00:00

src/crud.js

joeybaker (Migrated from github.com) commented

2016-11-10 19:25:20 +00:00

We can try with the integration tests no?

joeybaker (Migrated from github.com) reviewed 2016-11-10 19:25:44 +00:00

test/fixtures/models/player.js

						
				@ -16,1 +16,4 @@

				        });

				        models.Player.belongsTo(models.Master, {

				          foreignKey: 'coachId',

				          as: 'Coach',

joeybaker (Migrated from github.com) commented

2016-11-10 19:25:44 +00:00

Yup!

joeybaker (Migrated from github.com) reviewed 2016-11-10 19:26:09 +00:00

src/utils.js

joeybaker (Migrated from github.com) commented

2016-11-10 19:26:09 +00:00

Cool. I'm still not sure why we need promises though?

labibramadhan (Migrated from github.com) reviewed 2016-11-11 02:09:19 +00:00

src/utils.js

labibramadhan (Migrated from github.com) commented

2016-11-11 02:09:19 +00:00

because getModelInstance may loop itself, for example i got object like this:

{
  include: {
   model: Team,
   include: {
     model: Player,
     where: {
       name: Pinot
     },
     include: {
       model: Master,
       as: 'Coach',
       where: {
         name: 'Shifu'
       }
     }
   }
  }
}

the getModelInstance function will loop through all of its include children to convert each model name string to be its model instance, notice that conversion here needs logic like looping all existing models to find the right model instance

because **getModelInstance** may loop itself, for example i got object like this: ``` { include: { model: Team, include: { model: Player, where: { name: Pinot }, include: { model: Master, as: 'Coach', where: { name: 'Shifu' } } } } } ``` the **getModelInstance** function will loop through all of its **include** children to convert each model name string to be its model instance, notice that conversion here needs logic like looping all existing models to find the right model instance

labibramadhan (Migrated from github.com) reviewed 2016-11-11 02:18:57 +00:00

src/crud.js

labibramadhan (Migrated from github.com) commented

2016-11-11 02:18:57 +00:00

yes maybe. especially if you can express what the possibility of vulnerability that might happen to this include feature. i have tried something like this to mess the JSON include parameter:

/cities?include={ model: "Team" } // it should be: /cities?include={ "model": "Team" }
or
/cities?include=model: "Team" } // it should be: /cities?include={ "model": "Team" }

this is the both result:

yes maybe. especially if you can express what the possibility of vulnerability that might happen to this include feature. i have tried something like this to mess the JSON include parameter: ``` /cities?include={ model: "Team" } // it should be: /cities?include={ "model": "Team" } or /cities?include=model: "Team" } // it should be: /cities?include={ "model": "Team" } ``` this is the both result: ![screen shot 2016-11-11 at 9 17 57 am](https://cloud.githubusercontent.com/assets/6038766/20202161/cc39e6ac-a7ef-11e6-93cf-4936799a51fd.png)

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions.