search

buunguyen / koa-req-validator

Declarative request validation middleware for Koa
MIT License
19 stars 11 forks source link

Add option to disable searching scopes by colon separator #10

Closed TranKienCuong closed 5 years ago

TranKienCuong commented 5 years ago

Hi @buunguyen, cc @duylam, @ledinhphuong There are some situations that we need to search the field value that contains the : character. For example with this request body:

body = {
  "appium:deviceName": "Galaxy S7",
  "appium:platformVersion": "7.0"
}

As we have been split the scopes and field by the : then it will throw an error with the above scenario. So I think it's necessary to have an option to disable to search the scopes by : and let the field value as it is.

buunguyen commented 5 years ago

Maybe instead of this, should change the separate to |? That is much less likely to collide. So, you can just do 'appium:deviceName|body|query'. Since this is a potential breaking change, should bump the major version too to be semver compliant.

TranKienCuong commented 5 years ago

Hi @buunguyen, just consider that change the separate to | is the real breaking change on our code as it takes much time to replace and testing again all the validation code. However, if you think this is the only way to do it, just close this PR and I will make a new PR with the | separator.

duylam commented 5 years ago

should bump the major version too to be semver compliant.

@buunguyen This costs extra effort for somebody (at least with us) when we need to change our code (and test again) in order to have the new fix. The approach of giving option on middlware balances the breaking and cost for upgrading, it also supports futher enhancement when we may need to have more option later on (something like customizing the response code on validating fail)

buunguyen commented 5 years ago

Good point. But I don't like the approach in this PR either, looks like hack to me. I think a breaking change with version bump is worth it. It shouldn't be difficult for you to make the refactoring, i.e. "search for \'validate(\'", replace colon with pipe.

Unless you have a better idea than what is proposed in this PR...

buunguyen commented 5 years ago

BTW, if you decide to work on this PR. Please highlight the breaking change from 1.0 to 2.0.

TranKienCuong commented 5 years ago

Hi, as this is not our need anymore, I will close this PR. Thank you for your feedback