Fix ValuesValidator for params wrapped in with block
#2382
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
numbata
commented
Dec 5, 2023
| @@ -85,7 +85,12 @@ def except_message | |||
| end | |||
|
|
|||
| def required_for_root_scope? | |||
| @required && @scope.root? | |||
| return false unless @required | |||
There was a problem hiding this comment.
"fail fast" if the param is not required.
Comment on lines
+90
to
+91
| scope = @scope | ||
| scope = scope.parent while scope.lateral? |
There was a problem hiding this comment.
I've updated the logic here to handle nested parameters within a with block. The method now traverses up the scope hierarchy to check if the parameter's root scope is required, ensuring correct validation even when parameters are deeply nested within a `with' block.
There was a problem hiding this comment.
You could extract this into lateral_scope or something like that, although I am not sure it's worth it
numbata
changed the title
with block
|
Perfect, thank you. Update CHANGELOG and we're good to go! |
|
Merged, thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug #2376 in Grape's
ValuesValidator, specifically when parameters are nested within awithblock. Previously, the validator did not correctly handle the required validation for nested parameters in this context.Changes:
required_for_root_scope?method inValuesValidatorhas been modified to correctly detect if a parameter is required when it's nested inside awithblock. The fix involves checking the parent scope of the parameter until a non-lateral (non-withblock) scope is found, and then determining if it's the root scope.values_spec.rbto cover the scenario where a parameter nested within awithblock doesn't meet the validation criteria, ensuring that the correct error response is generated.Impact:
This fix ensures more accurate validation of parameters nested within
withblocks, and aligns the behavior with the expected norms of Grape's parameter validation. It's an important fix for Grape users who use nested parameter structures, improving the reliability and correctness of the validation logic.I welcome any feedback or suggestions on this fix and look forward to your reviews!