@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:
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
Copy Source
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!
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`
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
@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
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!
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?
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?
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
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
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
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 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
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
@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
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
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
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
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
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
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:
You are not authorized to merge this pull request.
This pull request can be merged automatically.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Added feature to allow filtering relationships/associations based on http://docs.sequelizejs.com/en/latest/docs/querying/#relations-associations
oops sorry, forgot to fix associationValidation of get-config-for-method.test.js
@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. 👍
Current coverage is 72.50% (diff: 79.36%)
Hi @mdibaiee, i have added some guide text about this feature in README, please have a check 😃
Why remove this section? 🤔
@@ -116,0 +119,4 @@// GET /players?include={"model": "Master", "as": "Couch"}// results in a Sequelize query:Players.findAll({include: Master, as: 'Couch'})Please get rid of the
includekey of the JSON passed toinclude, it's an unnecessary repetition.It should look like this:
/teams?include={"model": "City", "where": { "name": "Healdsburg" }}Thanks for the work @labibramadhan!
Generally:
get-config-for-method.js. You should be able to do all your changes incrud.jsget-config-for-methodjoi.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!
Why move this validation into
get-config-for-method? All other validation is defined in this method already.hmmm… it's weird that we're adding in custom validation here instead of just trusting that the
associationValidationis correct when we received it. We should fix this at the source incrud.js@@ -20,0 +21,4 @@const getModelInstance = (models, includeItem) => {return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {JSON.parsewill throw if not handed valid JSON. We should use a try/catch here.@@ -20,0 +21,4 @@const getModelInstance = (models, includeItem) => {return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {what code inside the catch do you prefer?
Sorry @joeybaker i have fixed this, moved to crud.js
By the way i got different indentation settings with your existing scripts, is it OK? what IDE you guys using?
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
@@ -116,0 +119,4 @@// GET /players?include={"model": "Master", "as": "Couch"}// results in a Sequelize query:Players.findAll({include: Master, as: 'Couch'})Fixed in my last commit, could you verify now?
@labibramadhan we use two spaces for indentation, almost all IDEs and editors support indentation settings. Thanks for looking at the mentioned issues.
@@ -20,0 +21,4 @@const getModelInstance = (models, includeItem) => {return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {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!
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?
@@ -20,0 +22,4 @@return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {const singluarOrPluralMatch = Object.keys(models).find((modelName) => {maybe like this @joeybaker?
@@ -20,0 +22,4 @@return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {const singluarOrPluralMatch = Object.keys(models).find((modelName) => {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
ifcondition, becausejoi.string()always returns a truthy value.What do you think of something like this in
crud.json line 77?@@ -20,0 +22,4 @@return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {const singluarOrPluralMatch = Object.keys(models).find((modelName) => {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)@@ -20,0 +22,4 @@return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {const singluarOrPluralMatch = Object.keys(models).find((modelName) => {currently my version of crud.js has this to validate its association object:
@@ -20,0 +22,4 @@return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {const singluarOrPluralMatch = Object.keys(models).find((modelName) => {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:
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 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.since nested includes feature may loop the getModelInstance function, parseInclude is now a promise based function, to call this function needs await
@@ -16,1 +16,4 @@});models.Player.belongsTo(models.Master, {foreignKey: 'coachId',as: 'Coach',needed for association/relationship alias test
@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)
this things was formatted
${prefix}/${names.a.singular}/{aid}/${names.b.plural}, but if prefix equals "/", it will produce path like//team/{aid}/playerand will output404 not foundresponse, i decided to remove the slash after${prefix}, so every prefix option definition must have trailing slash@@ -20,0 +22,4 @@return new Promise(async(resolve) => {if (includeItem) {if (typeof includeItem !== 'object') {const singluarOrPluralMatch = Object.keys(models).find((modelName) => {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
alright @joeybaker, i will try that method, thank you!
@@ -16,1 +16,4 @@});models.Player.belongsTo(models.Master, {foreignKey: 'coachId',as: 'Coach',Can you not use Teams and Players? Just so we don't get an overly-complex mock schema?
@@ -8,3 +8,3 @@test('belongsTo /team?include=city', async (t) => {test('belongsTo /team?include=city', async(t) => {const { server, instances } = t.context;Why remove all these spaces?
So… I'm still confused. How would we be getting an Object in here? Is joi smart enough to parse the JSON string?
hmmm… so two questions:
getModelInstancelooks 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.@@ -16,1 +16,4 @@});models.Player.belongsTo(models.Master, {foreignKey: 'coachId',as: 'Coach',Ah! I see. Makes sense.
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
@@ -16,1 +16,4 @@});models.Player.belongsTo(models.Master, {foreignKey: 'coachId',as: 'Coach',is it ok then?
@@ -8,3 +8,3 @@test('belongsTo /team?include=city', async (t) => {test('belongsTo /team?include=city', async(t) => {const { server, instances } = t.context;sorry this is from my IDE settings, but i don't see any eslint error
i think the only answer is we need to try. if it's not smart enough, maybe later we will find bug report
We can try with the integration tests no?
@@ -16,1 +16,4 @@});models.Player.belongsTo(models.Master, {foreignKey: 'coachId',as: 'Coach',Yup!
Cool. I'm still not sure why we need promises though?
because getModelInstance may loop itself, for example i got object like this:
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
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:
this is the both result:
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.