motiv-labs / janus

An API Gateway written in Go
https://hellofresh.gitbooks.io/janus
MIT License
2.8k stars 320 forks source link

Add validation to (*Definition).Name #268

Closed al3rez closed 6 years ago

al3rez commented 6 years ago

Why?

So that it cannot contain non-URL firendly chars.

How?

By validating and matching against the pattern below: /^[A-Za-z0-9]+(?:-[A-Za-z0-9]+)*$/

al3rez commented 6 years ago

@vgarvardt matches(pattern) doesn't work as expected I think it's due to escaping mechanism 'cause regex patterns should be back-quoted and struct tags are already back-quoted so there's no way to do that, am I wrong?

Also I think Validate() function is a good place to put various validation logic explicitly instead of adding more advanced validators to struct tags ;)

vgarvardt commented 6 years ago

@azbshiri I created a branch from your branch and made your regexp to work with govalidator.

Here is the branch https://github.com/hellofresh/janus/tree/feature/name-slug-validation, and here is my commit https://github.com/hellofresh/janus/commit/55fb6256f8f42969aad733a7d9842eeff51ffe66 that implements the validation.

Do you mind if I close your PR and open new one from my branch that includes both your and my commits with the validation implementation?

al3rez commented 6 years ago

@vgarvardt I have to ask a question, what if a new validation needed? you're gonna add one more validator to the struct tag?

vgarvardt commented 6 years ago

@azbshiri My personal opinion - yes, we need to use as few ways of doing one thing as possible, but not less. At the moment we're using govalidator, so we're trying to avoid cusomt validation code if it is possible. If we'll need to add one more validator - the code may become too unreadable (validator+error text, too long string), so I'd created new custom validator, moved all the validation logic there (we still can call govalidator.Matches(...) etc. from it) and used something like:

type Definition struct {
    Name string `valid:"apiDefinitionName"`
    ...
}
al3rez commented 6 years ago

great

vgarvardt commented 6 years ago

Closing in favour of https://github.com/hellofresh/janus/pull/272 that is based on this PR.