Validate path, query and request parameters separately #349
Conversation
OpenAPI specifies 3 different types of parameters, let's validate them separately.
|
👋🏻 |
| @@ -435,7 +435,7 @@ def app | |||
| assert_equal 200, last_response.status | |||
| end | |||
|
|
|||
| it "corce form params" do | |||
| it "coerce form params" do | |||
| @@ -412,12 +412,12 @@ def app | |||
| get "/coerce_path_params/1" | |||
| end | |||
|
|
|||
| it "corce string and save path hash" do | |||
| it "coerce string and save path hash" do | |||
| rescue => e | ||
| raise Committee::InvalidRequest.new(e.message, original_error: e) | ||
| end | ||
|
|
||
| def validate_path_and_query_params(path_params, query_params, headers, validator_option) | ||
| # it's currently impossible to validate path params and query params separately | ||
| # so we have to resort to this workaround |
There was a problem hiding this comment.
openapi_parser have validate_path_params method.
https://github.com/ota42y/openapi_parser/blob/f6a975791fc58b19d40498daeb88b80e01c70c16/lib/openapi_parser/request_operation.rb#L42
That should have made it compatible with separate validation... I'll do check that code 🙋
There was a problem hiding this comment.
openapi_parser have validate_path_params but operation_wrapper doesn't delegate it so we can't use it.
Ok, this code is no problem 🙆♀️
| request.env[validator_option.params_key].merge!(Committee::Utils.deep_copy(request.env[validator_option.request_body_hash_key])) | ||
| request.env[validator_option.params_key].merge!(Committee::Utils.deep_copy(request.env[validator_option.query_hash_key])) |
There was a problem hiding this comment.
I think it's very good to prioritize path, query, and body in that order, just like Rails.
But this is breaking changes so I add backward compatibility code for the users which want to old priority.
(This is memo. You do not need to fix it :) )
|
Hey people, would you accept this PR? We are waiting for it too. Pleaaaaseeeee! |
|
Hey, we are still affected by this issue and would not like to fork this repo just to get this merged. |
|
Sorry for the very delay, thanks for the changes! |
|
I released 5.0.0 with this changes! |
fixes #339
fixes #328