wrap request.path_parameters for request.paramters #19860
Conversation
|
I think the specification of this PR is more natural than the old implementation, but please let me know if there are any reasons that the specification must be kept as the old implementation. |
|
cc @jeremy |
|
r? @jeremy |
|
Ping @jeremy ? |
|
+1 in principle. Would merging these params break any apps? Why / why not? |
|
One thing I could see right away is that it's possible that parent's id (when you have a route such as With that said, @sonots I think we should add a test case for nested id ( |
sonots
force-pushed
the
wrap_path_parameters
branch
2 times, most recently
from
c5727dd
to
9fbbddd
Compare
| @@ -50,7 +64,23 @@ def test_filtered_parameters | |||
| with_default_wrapper_options do | |||
| @request.env['CONTENT_TYPE'] = 'application/json' | |||
| post :parse, params: { 'username' => 'sikachu' } | |||
| assert_equal @request.filtered_parameters, { 'controller' => 'params_wrapper_test/users', 'action' => 'parse', 'username' => 'sikachu', 'user' => { 'username' => 'sikachu' } } | |||
| assert_equal({ 'controller' => 'params_wrapper_test/users', 'action' => 'parse', 'username' => 'sikachu', 'user' => { 'username' => 'sikachu' } }, @request.filtered_parameters) | |||
There was a problem hiding this comment.
changed the order because assert_equal expected, actual
| else | ||
| wrapped_hash = _wrap_parameters request.request_parameters | ||
| wrapped_hash = _wrap_parameters request.parameters | ||
| wrapped_request_hash = _wrap_parameters request.request_parameters | ||
| end | ||
|
|
||
| wrapped_keys = request.request_parameters.keys | ||
| wrapped_filtered_hash = _wrap_parameters request.filtered_parameters.slice(*wrapped_keys) |
There was a problem hiding this comment.
I think the slice stuff here is now unnecessary and incorrect - previously we only wrapped request parameters, so we had to filter out query and path parameters here. Now we're wrapping all parameters, so this can just be:
wrapped_filtered_hash = _wrap_parameters request.filtered_parameters
sonots
force-pushed
the
wrap_path_parameters
branch
from
9fbbddd
to
b9e6b8c
Compare
sonots
force-pushed
the
wrap_path_parameters
branch
from
b9e6b8c
to
0ee5b61
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Currently,
wrap_parametersdoes not wraprequest.path_parameters.I propose that
request.parameterswrapsrequest.path_parametersin addition torequest.request_parametersso that we can get theidparameter often assigned as path parameters (likeget '/books/:id').request.request_parametersis kept as is so that it wraps onlyrequest.request_parameters.