When using a query parameter to send a where query, we now have Joi error if the model attribute doesn't exist.
parseInclude errors are now handled in places where they weren't before, and a correct Boom error is used so the user will get useful feedback.
Docs improvements related to the above.
This fixes an issue were the user's config options could be overridden by our own.
This is a minor release.
1. When using a query parameter to send a `where` query, we now have Joi error if the model attribute doesn't exist.
2. `parseInclude` errors are now handled in places where they weren't before, and a correct Boom error is used so the user will get useful feedback.
3. Docs improvements related to the above.
4. This fixes an issue were the user's config options could be overridden by our own.
This is a minor release.
const attributes = {};
Object.keys(model.attributes).forEach(key => {
attributes[key] = joi[model.attributes[key].type]] // matching the type would be ideal, but if that's too hard, we can just go with a simple validation
attributes[key] = joi.any.allow()
});
//...
config: _.defaultsDeep({
validate: {
params: joi.object().keys({
scope: joi.string().valid(...scopes),
..attributes
}),
},
}, config),
Thank you Joey :+1:
Just a comment on `parseWhere`: You're checking for validity of keys, but I think we can leave it to Hapi to handle (like I did with scopes: https://github.com/Getable/hapi-sequelize-crud/blob/73f84b92212e05c2fbce50ce7c43ed47b0935908/src/crud.js#L146), what do you think?
It would be something like this:
```
const attributes = {};
Object.keys(model.attributes).forEach(key => {
attributes[key] = joi[model.attributes[key].type]] // matching the type would be ideal, but if that's too hard, we can just go with a simple validation
attributes[key] = joi.any.allow()
});
//...
config: _.defaultsDeep({
validate: {
params: joi.object().keys({
scope: joi.string().valid(...scopes),
..attributes
}),
},
}, config),
```
@mdibaiee I took a look at this some more and had a thought: if we use validate.params to do this validation, we'll disallow the user from overriding the defaults. I can see an user wanting to dissallow where queries by certain fields. (e.g. password). Right now, config.validate gives users the ability to restrict the where and include params to whatever they want.
Two paths I can see:
Use config.validate, but allow users to override
config:{validate:{params:{...attributesAsJoi},// allow the user to complete override the default validation
// we'll need to add docs to make this obvious
...config.validate,}}
Keep things as-is in this PR. It's slightly less idomatic Hapi code, but it always gives the user full control over validation and always ensures that non-valid keys error.
Thoughts?
@mdibaiee I took a look at this some more and had a thought: if we use `validate.params` to do this validation, we'll disallow the user from overriding the defaults. I can see an user wanting to dissallow where queries by certain fields. (e.g. `password`). Right now, `config.validate` gives users the ability to restrict the `where` and `include` params to whatever they want.
Two paths I can see:
1. Use `config.validate`, but allow users to override
``` js
config: {
validate: {
params: {
...attributesAsJoi
},
// allow the user to complete override the default validation
// we'll need to add docs to make this obvious
...config.validate,
}
}
```
2. Keep things as-is in this PR. It's slightly less idomatic Hapi code, but it always gives the user full control over validation and always ensures that non-valid keys error.
Thoughts?
One other thought: we could use joi-sequelize to generate the Joi schema. We've been slowly implementing that ourselves, but it might make sense to build it into hapi-sequelize-crud, and this might be the right place to start to do it?
One other thought: we could use [joi-sequelize](https://www.npmjs.com/package/joi-sequelize) to generate the Joi schema. We've been slowly implementing that ourselves, but it might make sense to build it into `hapi-sequelize-crud`, and this might be the right place to start to do it?
@joeybaker: what if the user wants to use a special parameter? Using this code he won't be able to because his special parameter would error out, but if we use Joi validation and let him override it, that would work. (it's just an example)
Also, let's use _.defaultsDeep instead of ...config.validate, would that work?
That's completely up to you to use joi-sequelize or not 👍
@joeybaker: what if the user wants to use a special parameter? Using this code he won't be able to because his special parameter would error out, but if we use Joi validation and let him override it, that would work. (it's just an example)
Also, let's use `_.defaultsDeep` instead of `...config.validate`, would that work?
That's completely up to you to use `joi-sequelize` or not :+1:
What sort special parameter would they want to use? I can't think of a good example?
The problem is see with defaultsDeep is that it doesn't allow the user to modify anything after we've set it right?
So if a model has a password field, how would we allow the user to set password: joi.any().forbidden()?
What sort special parameter would they want to use? I can't think of a good example?
The problem is see with `defaultsDeep` is that it doesn't allow the user to modify anything after we've set it right?
So if a model has a `password` field, how would we allow the user to set `password: joi.any().forbidden()`?
So user's config is already applied, why can't the user just use the validate key of their config? Wouldn't that work?
Let's see, we already have
```
config: _.defaultsDeep({
validate: {
params: joi.object().keys({
scope: joi.string().valid(...scopes),
..attributes
}),
},
}, config)
```
So user's config is already applied, why can't the user just use the `validate` key of their config? Wouldn't that work?
Ours will win because defaultsDeep(ours, theirs) will always takes ours instead of theirs. Right?
The problem I'm seeing is that `defaultsDeep` lets the first key win every time. So if we set a
``` js
validate: {
params: {
password: joi.any()
}
}
```
And then the user sets
``` js
validate: {
params: {
password: joi.any().forbidden()
}
}
```
Ours will win because `defaultsDeep(ours, theirs)` will always takes ours instead of theirs. Right?
@mdibaiee I had some time, so I made the changes :)
This is basically an all-new PR – let me know what you think. It probably makes most sense to look at this commit-by-commit b/c I tossed some tangentially related stuff in here. (sorry)
@mdibaiee I had some time, so I made the changes :)
This is basically an all-new PR – let me know what you think. It probably makes most sense to look at this commit-by-commit b/c I tossed some tangentially related stuff in here. (sorry)
@joeybaker: Thanks Joey! 👍🎉
Looks good to me, may you please move the getConfigForMethod function to it's own file?
So, basically we're now validating query parameters, not handling where query more gracefully, is that right? Please rename the PR as appropriate. Thank you very much!
@joeybaker: Thanks Joey! :+1: :tada:
Looks good to me, may you please move the `getConfigForMethod` function to it's own file?
So, basically we're now validating query parameters, not handling `where` query more gracefully, is that right? Please rename the PR as appropriate. Thank you very much!
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.
wherequery, we now have Joi error if the model attribute doesn't exist.parseIncludeerrors are now handled in places where they weren't before, and a correct Boom error is used so the user will get useful feedback.This is a minor release.
Thank you Joey 👍
Just a comment on
parseWhere: You're checking for validity of keys, but I think we can leave it to Hapi to handle (like I did with scopes: https://github.com/Getable/hapi-sequelize-crud/blob/73f84b92212e05c2fbce50ce7c43ed47b0935908/src/crud.js#L146), what do you think?It would be something like this:
Great idea. Will fix.
@mdibaiee I took a look at this some more and had a thought: if we use
validate.paramsto do this validation, we'll disallow the user from overriding the defaults. I can see an user wanting to dissallow where queries by certain fields. (e.g.password). Right now,config.validategives users the ability to restrict thewhereandincludeparams to whatever they want.Two paths I can see:
Use
config.validate, but allow users to overrideKeep things as-is in this PR. It's slightly less idomatic Hapi code, but it always gives the user full control over validation and always ensures that non-valid keys error.
Thoughts?
One other thought: we could use joi-sequelize to generate the Joi schema. We've been slowly implementing that ourselves, but it might make sense to build it into
hapi-sequelize-crud, and this might be the right place to start to do it?@joeybaker: what if the user wants to use a special parameter? Using this code he won't be able to because his special parameter would error out, but if we use Joi validation and let him override it, that would work. (it's just an example)
Also, let's use
_.defaultsDeepinstead of...config.validate, would that work?That's completely up to you to use
joi-sequelizeor not 👍What sort special parameter would they want to use? I can't think of a good example?
The problem is see with
defaultsDeepis that it doesn't allow the user to modify anything after we've set it right?So if a model has a
passwordfield, how would we allow the user to setpassword: joi.any().forbidden()?Maybe we should apply the user config first with
defaultsDeep?Let's see, we already have
So user's config is already applied, why can't the user just use the
validatekey of their config? Wouldn't that work?The problem I'm seeing is that
defaultsDeeplets the first key win every time. So if we set aAnd then the user sets
Ours will win because
defaultsDeep(ours, theirs)will always takes ours instead of theirs. Right?@joeybaker: Oh, then we're doing it wrong, let's swap the arguments?
Cool. Works for me!
I'm AFK for the rest of the day, but I can fix up this PR in the next couple of days (unless you feel like getting to it first).
@mdibaiee I had some time, so I made the changes :)
This is basically an all-new PR – let me know what you think. It probably makes most sense to look at this commit-by-commit b/c I tossed some tangentially related stuff in here. (sorry)
@mdibaiee If you get a chance, can you take a look at this? I'd appreciate a second pair of eyes.
@joeybaker: Thanks Joey! 👍 🎉
Looks good to me, may you please move the
getConfigForMethodfunction to it's own file?So, basically we're now validating query parameters, not handling
wherequery more gracefully, is that right? Please rename the PR as appropriate. Thank you very much!PR renamed. File moved, and tests added. I'm feeling good about this, so I'm going to merge and release!
Released as 2.6.0