Validate query and payload #20

2016-09-04T01:53:02Z

joeybaker commented

2016-09-04 01:53:02 +00:00

(Migrated from github.com)

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.

👍 1

mdibaiee commented

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

(Migrated from github.com)

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: 73f84b9221/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),

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), ```

joeybaker commented

2016-09-04 04:27:06 +00:00

(Migrated from github.com)

Great idea. Will fix.

joeybaker commented

2016-09-04 16:29:27 +00:00

(Migrated from github.com)

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

joeybaker commented

2016-09-04 16:31:38 +00:00

(Migrated from github.com)

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?

mdibaiee commented

2016-09-04 16:57:50 +00:00

(Migrated from github.com)

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

joeybaker commented

2016-09-04 19:03:38 +00:00

(Migrated from github.com)

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()`?

joeybaker commented

2016-09-04 19:19:48 +00:00

(Migrated from github.com)

Maybe we should apply the user config first withdefaultsDeep?

Maybe we should apply the user config first with`defaultsDeep`?

mdibaiee commented

2016-09-04 19:32:54 +00:00

(Migrated from github.com)

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?

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?

joeybaker commented

2016-09-04 19:44:53 +00:00

(Migrated from github.com)

The problem I'm seeing is that defaultsDeep lets the first key win every time. So if we set a

validate: {
  params: {
    password: joi.any()
   }
}

And then the user sets

validate: {
  params: {
    password: joi.any().forbidden()
   }
}

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 commented

2016-09-04 19:45:32 +00:00

(Migrated from github.com)

@joeybaker: Oh, then we're doing it wrong, let's swap the arguments?

joeybaker commented

2016-09-04 19:56:38 +00:00

(Migrated from github.com)

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

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

joeybaker commented

2016-09-05 00:38:55 +00:00

(Migrated from github.com)

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

2016-09-05 19:32:10 +00:00

(Migrated from github.com)

@mdibaiee If you get a chance, can you take a look at this? I'd appreciate a second pair of eyes.

mdibaiee commented

2016-09-06 05:08:43 +00:00

(Migrated from github.com)

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

joeybaker commented

2016-09-06 18:29:34 +00:00

(Migrated from github.com)

PR renamed. File moved, and tests added. I'm feeling good about this, so I'm going to merge and release!

👍 1

joeybaker commented

2016-09-06 18:31:02 +00:00

(Migrated from github.com)

Released as 2.6.0

Sign in to join this conversation.

No reviewers