-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle nested JSON and JSON arrays in params properly #1338
base: master
Are you sure you want to change the base?
Handle nested JSON and JSON arrays in params properly #1338
Conversation
@@ -6,7 +6,7 @@ module Amber::Validators | |||
class BaseRule | |||
getter predicate : (String -> Bool) | |||
getter field : String | |||
getter value : String? | |||
getter value : String | Array(JSON::Any) | JSON::Any | Nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON::Any::Type
includes Array(JSON::Any)
, so is it redundant to include both, or is there something I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my thought as well, so I don't think we need the extra Array(JSON::Any)
@@ -1,4 +1,9 @@ | |||
module ValidationsHelper | |||
def json_params_builder(body = "") | |||
request = HTTP::Request.new("POST", "/", HTTP::Headers{"Content-Type" => "application/json"}, body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is hard coded as a POST request. Does that mean that it can only be used with POST requests, or can it be used with PUT and PATCH requests too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is just a test, but we should be testing POST
PATCH
AND PUT
required("y") | ||
end | ||
|
||
validator.validate!["x"].should be_a Array(JSON::Any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe returning a JSON::PullParser
would be better here, so people could use JSON::Serializable
to decode these parameters. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When was that introduced in the language? I don't recall seeing this at the time I wrote this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@a-alhusaini it's been part of the std library for a few years
Before:
After