Better Request Parsing Parameters #331

drujensen · 2017-10-26T13:35:14Z

Description of the Change

The framework should be able to parse JSON request body appropriately by
returning a JSON::Any object.

Currently, there is a bug when trying to parse JSON parameters from the
body of a request where nested JSON payload is not being parsed correctly

{ "test": "test", "address": { "city": "New York" }
params["address"].as(Hash)["city"]

Params is now a custom Hash of the following signature
Hash(String | Symbol, Amber::Router::ParamsType) or
Amber::Router::ParamsHash.new

To give more context all parameters where being parsed using the
HTTP::Params.parse method which returns an object of type
Hash(String, Array(String)) this caused issues when parsing
JSON params from the body, It turns out that nested JSON keys where
being parsed as a string. Basically, all params values are currently
returning as a string.

With the changes presented in this PR users can now define model objects
with JSON mapping that should be evaluated correctly based on the mapping
defined.

Controller
The controller now generates the app using params.validation similar to
Rails strong params. It adds a method to the Controller and overloads the
Granite initializer and set_attributes methods.

private def animal_params
  params.validation do
    required(:name) { |v| v.str? & !v.empty? }
    required(:last_name, "Last name is required")
    optional(:phone)  
  end
end

# Validate! returns a hash of only the specified params above.
animal = Animal.new(animal_params.validate!)

Alternate Designs

It would be nice to specify a type that can be cast automatically

required(:last_name, :string, "Last name is required") do
...validation code here...
end

Benefits

Fixes Issue: Nested JSON, numbers, and boolean params being parsed as strings #310
Parses JSON body as JSON::Type. with this type user can convert to the correct type either using
.to_i, .to_f, .to_s or traverse the JSON using json = JSON.parse(params["some_key"])
Uses params.validation to validate request params before any processing. Inspired by http://hanamirb.org/guides/1.0/actions/parameters/#validations-amp-coercion
Updates Auth, scaffold generators to use the params.validation

Possible Drawbacks

-Fear something can break

Example app using this branch

https://github.com/eliasjpr/params-test-app

Issue: #310 The framework should be able to parse JSON request body appropriately by returning a `JSON::Any` object. Currently, there is a bug when trying to parse JSON parameters from the body of a request where nested JSON payload is not being parsed correctly ```crystal { "test": "test", "address": { "city": "New York" } params["address"].as(Hash)["city"] ``` Params is now a custom Hash of the following signature `Hash(String | Symbol, Amber::Router::ParamsType)` or `Amber::Router::ParamsHash.new` To give more context all parameters where being parsed using the **HTTP::Params.parse** method which returns an object of type **Hash(String, Array(String))** this caused issues when parsing JSON params from the body, It turns out that nested JSON keys where being parsed as a string. Basically, all `params` values are currently returning as a string. With the changes presented in this PR users can now define model objects with JSON mapping that should be evaluated correctly based on the mapping defined. So for instance: ```crystal params["user_id"].as(Int64) => 123 params["address"].as(Hash).each ... params["user"].as(User) => User.name User.from_json(params["user"]) => User.name ``` **Controller** The controller now generates the app using params.validation similar to Rails strong params. It adds a method to the Controller and overloads the Granite initializer and set_attributes methods. ```crystal private def animal_params params.validation do required("name") { |v| v.str? & !v.empty? } end end animal = Animal.new(animal_params.validate!) ``` Model Models generate for the Granite::ORM now includes the following line include Amber::Models::Granite this will allow models to be populated with the controller params using the params.validations. The reason this is added here and not to the Granite::ORM repo is to keep Granite loosely coupled from any framework since the params.validation is specific to amber. ```crystal class Animal < Granite::ORM include Amber::Models::Granite end ```

faustinoaq

Looks good, 👍

faustinoaq · 2017-10-27T04:15:45Z

src/amber/cli/templates/auth.cr

@@ -25,7 +25,7 @@ module Amber::CLI
      @timestamp = Time.now.to_s("%Y%m%d%H%M%S")
      @primary_key = primary_key
      @visible_fields = @fields.reject(&.hidden).map(&.name)
-      
+
      add_routes :web, <<-ROUTES
        get "/signin", SessionController, :new
          post "/session", SessionController, :create


Indentation issue

* Better Request Parsing Parameters Issue: amberframework#310 The framework should be able to parse JSON request body appropriately by returning a `JSON::Any` object. Currently, there is a bug when trying to parse JSON parameters from the body of a request where nested JSON payload is not being parsed correctly ```crystal { "test": "test", "address": { "city": "New York" } params["address"].as(Hash)["city"] ``` Params is now a custom Hash of the following signature `Hash(String | Symbol, Amber::Router::ParamsType)` or `Amber::Router::ParamsHash.new` To give more context all parameters where being parsed using the **HTTP::Params.parse** method which returns an object of type **Hash(String, Array(String))** this caused issues when parsing JSON params from the body, It turns out that nested JSON keys where being parsed as a string. Basically, all `params` values are currently returning as a string. With the changes presented in this PR users can now define model objects with JSON mapping that should be evaluated correctly based on the mapping defined. So for instance: ```crystal params["user_id"].as(Int64) => 123 params["address"].as(Hash).each ... params["user"].as(User) => User.name User.from_json(params["user"]) => User.name ``` **Controller** The controller now generates the app using params.validation similar to Rails strong params. It adds a method to the Controller and overloads the Granite initializer and set_attributes methods. ```crystal private def animal_params params.validation do required("name") { |v| v.str? & !v.empty? } end end animal = Animal.new(animal_params.validate!) ``` Model Models generate for the Granite::ORM now includes the following line include Amber::Models::Granite this will allow models to be populated with the controller params using the params.validations. The reason this is added here and not to the Granite::ORM repo is to keep Granite loosely coupled from any framework since the params.validation is specific to amber. ```crystal class Animal < Granite::ORM include Amber::Models::Granite end ``` * fixup! Better Request Parsing Parameters

* Better Request Parsing Parameters Issue: #310 The framework should be able to parse JSON request body appropriately by returning a `JSON::Any` object. Currently, there is a bug when trying to parse JSON parameters from the body of a request where nested JSON payload is not being parsed correctly ```crystal { "test": "test", "address": { "city": "New York" } params["address"].as(Hash)["city"] ``` Params is now a custom Hash of the following signature `Hash(String | Symbol, Amber::Router::ParamsType)` or `Amber::Router::ParamsHash.new` To give more context all parameters where being parsed using the **HTTP::Params.parse** method which returns an object of type **Hash(String, Array(String))** this caused issues when parsing JSON params from the body, It turns out that nested JSON keys where being parsed as a string. Basically, all `params` values are currently returning as a string. With the changes presented in this PR users can now define model objects with JSON mapping that should be evaluated correctly based on the mapping defined. So for instance: ```crystal params["user_id"].as(Int64) => 123 params["address"].as(Hash).each ... params["user"].as(User) => User.name User.from_json(params["user"]) => User.name ``` **Controller** The controller now generates the app using params.validation similar to Rails strong params. It adds a method to the Controller and overloads the Granite initializer and set_attributes methods. ```crystal private def animal_params params.validation do required("name") { |v| v.str? & !v.empty? } end end animal = Animal.new(animal_params.validate!) ``` Model Models generate for the Granite::ORM now includes the following line include Amber::Models::Granite this will allow models to be populated with the controller params using the params.validations. The reason this is added here and not to the Granite::ORM repo is to keep Granite loosely coupled from any framework since the params.validation is specific to amber. ```crystal class Animal < Granite::ORM include Amber::Models::Granite end ``` * fixup! Better Request Parsing Parameters

* Better Request Parsing Parameters Issue: #310 The framework should be able to parse JSON request body appropriately by returning a `JSON::Any` object. Currently, there is a bug when trying to parse JSON parameters from the body of a request where nested JSON payload is not being parsed correctly ```crystal { "test": "test", "address": { "city": "New York" } params["address"].as(Hash)["city"] ``` Params is now a custom Hash of the following signature `Hash(String | Symbol, Amber::Router::ParamsType)` or `Amber::Router::ParamsHash.new` To give more context all parameters where being parsed using the **HTTP::Params.parse** method which returns an object of type **Hash(String, Array(String))** this caused issues when parsing JSON params from the body, It turns out that nested JSON keys where being parsed as a string. Basically, all `params` values are currently returning as a string. With the changes presented in this PR users can now define model objects with JSON mapping that should be evaluated correctly based on the mapping defined. So for instance: ```crystal params["user_id"].as(Int64) => 123 params["address"].as(Hash).each ... params["user"].as(User) => User.name User.from_json(params["user"]) => User.name ``` **Controller** The controller now generates the app using params.validation similar to Rails strong params. It adds a method to the Controller and overloads the Granite initializer and set_attributes methods. ```crystal private def animal_params params.validation do required("name") { |v| v.str? & !v.empty? } end end animal = Animal.new(animal_params.validate!) ``` Model Models generate for the Granite::ORM now includes the following line include Amber::Models::Granite this will allow models to be populated with the controller params using the params.validations. The reason this is added here and not to the Granite::ORM repo is to keep Granite loosely coupled from any framework since the params.validation is specific to amber. ```crystal class Animal < Granite::ORM include Amber::Models::Granite end ``` * fixup! Better Request Parsing Parameters Former-commit-id: 9d9ccd6

eliasjpr force-pushed the ep/fixes-params-json-parsing branch from c2f8de7 to ff44e8f Compare October 26, 2017 23:58

eliasjpr requested review from elorest, migeorge, fridgerator, veelenga, c910335 and a team October 27, 2017 00:11

faustinoaq suggested changes Oct 27, 2017

View reviewed changes

eliasjpr and others added 2 commits October 27, 2017 06:40

fixup! Better Request Parsing Parameters

749d317

Merge branch 'master' into ep/fixes-params-json-parsing

9562c5f

eliasjpr merged commit f5d3106 into master Oct 27, 2017

eliasjpr deleted the ep/fixes-params-json-parsing branch October 27, 2017 10:53

eliasjpr mentioned this pull request Oct 27, 2017

Nested JSON, numbers, and boolean params being parsed as strings #310

Closed

This was referenced Nov 12, 2017

Document merged PRs from the last month. #384

Closed

Pull Requests which need to be documented before release. #385

Closed

faustinoaq added this to Done in Framework 2018 May 5, 2018

faustinoaq removed this from Done in Framework 2018 Jun 12, 2018

faustinoaq added this to Done in Amber Framework 2017 Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better Request Parsing Parameters #331

Better Request Parsing Parameters #331

drujensen commented Oct 26, 2017 •

edited by eliasjpr

faustinoaq left a comment

faustinoaq Oct 27, 2017

Better Request Parsing Parameters #331

Better Request Parsing Parameters #331

Conversation

drujensen commented Oct 26, 2017 • edited by eliasjpr

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Example app using this branch

faustinoaq left a comment

Choose a reason for hiding this comment

faustinoaq Oct 27, 2017

Choose a reason for hiding this comment

drujensen commented Oct 26, 2017 •

edited by eliasjpr