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 +327 -109
labibramadhan commented 2016-11-04 05:25:07 +00:00 (Migrated from github.com)
Copy Link

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

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

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

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

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

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

Why remove this section? 🤔

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
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" }}`
joeybaker (Migrated from github.com) requested changes 2016-11-04 16:27:02 +00:00
joeybaker (Migrated from github.com) left a comment
Copy Link

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

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

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

what code inside the catch do you prefer?

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

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

Sorry @joeybaker i have fixed this, moved to crud.js
labibramadhan commented 2016-11-07 01:59:29 +00:00 (Migrated from github.com)
Copy Link

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

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

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

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

Fixed in my last commit, could you verify now?

Fixed in my last commit, could you verify now?
mdibaiee commented 2016-11-07 12:27:02 +00:00 (Migrated from github.com)
Copy Link

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

@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
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!
labibramadhan commented 2016-11-08 01:25:46 +00:00 (Migrated from github.com)
Copy Link

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?

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

maybe like this @joeybaker?

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
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 (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
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 (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
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 (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
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?
joeybaker commented 2016-11-09 05:49:45 +00:00 (Migrated from github.com)
Copy Link

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

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

needed for association/relationship alias test

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

@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)

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

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
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
labibramadhan commented 2016-11-09 19:03:03 +00:00 (Migrated from github.com)
Copy Link

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

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

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

Why remove all these spaces?

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

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

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

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

Ah! I see. Makes sense.

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

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

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

is it ok then?

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

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

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

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

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

We can try with the integration tests no?

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

Yup!

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

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

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

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

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

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.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin labibramadhan/include-with-filter:labibramadhan/include-with-filter
git checkout labibramadhan/include-with-filter
Sign in to join this conversation.
Reviewers
No reviewers
joeybaker
mdibaiee
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
help wanted

   
invalid

   
question

   
wontfix
No Label
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: thereadme/hapi-sequelize-crud#36
No description provided.
Delete Branch "labibramadhan/include-with-filter"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?