Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

a-alhusaini
Copy link

Before:

class SomeController
  def index
    params.validation { # ... }
    
    params["x"] # => string "[1,2,3]"
end

After

class SomeController
  def index
    params.validation { # ... }
    
    params["x"] # => Array(JSON::Any)
end

src/amber/validators/params.cr Outdated Show resolved Hide resolved
src/amber/validators/params.cr Outdated Show resolved Hide resolved
src/amber/validators/params.cr Show resolved Hide resolved
@a-alhusaini a-alhusaini changed the title json arrays are now validated as Array(JSON::Any) Handle nested JSON and JSON arrays in params properly Aug 8, 2023
@@ -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
Copy link
Contributor

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?

Copy link
Member

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)
Copy link
Contributor

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?

Copy link
Member

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)
Copy link
Contributor

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. 🤔

Copy link
Author

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.

Copy link
Member

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

https://crystal-lang.org/api/1.10.1/JSON/PullParser.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants