Skip to content

Commit

Permalink
Better Request Parsing Parameters
Browse files Browse the repository at this point in the history
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
```
  • Loading branch information
Elias J. Perez authored and eliasjpr committed Oct 26, 2017
1 parent 44055a5 commit ff44e8f
Show file tree
Hide file tree
Showing 16 changed files with 136 additions and 89 deletions.
10 changes: 5 additions & 5 deletions spec/amber/amber_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@ struct UserSocket < Amber::WebSockets::ClientSocket; end

struct RoomSocket < Amber::WebSockets::ClientSocket; end


describe Amber do
describe Amber do
describe ".env" do
it "should return test" do
Amber.env.test?.should be_truthy
Expand Down Expand Up @@ -54,9 +53,10 @@ describe Amber do
settings.secret_key_base.should eq "mV6kTmG3k1yVFh-fPYpugSn0wbZveDvrvfQuv88DPF8"
end

it "retains environment.yml settings that haven't been overwritten" do # NOTE: Any changes to settings here remain for all specs run afterwards.
# This is a problem.

it "retains environment.yml settings that haven't been overwritten" do
# NOTE: Any changes to settings here remain for all specs run afterwards.
# Added for convenience until settings is change to an instances
# instead of singleton this is still an "a problem".
Amber::Server.configure do |server|
server.name = "Hello World App"
server.port = 8080
Expand Down
6 changes: 4 additions & 2 deletions spec/amber/cli/commands/generator_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ module Amber::CLI
context "scaffold" do
it "generates and compile generated app" do
ENV["AMBER_ENV"] = "test"

MainCommand.run ["new", TESTING_APP]
Dir.cd(TESTING_APP)
MainCommand.run ["generate", "scaffold", "Animal", "name:string"]
Amber::CLI::Spec.prepare_yaml(Dir.current)

`shards build`

File.exists?("bin/#{TESTING_APP.split("/").last}").should be_true
File.exists?("bin/#{TEST_APP_NAME}").should be_true

Amber::CLI::Spec.cleanup
end
end
Expand Down Expand Up @@ -49,6 +50,7 @@ module Amber::CLI
File.read("./config/routes.cr").includes?(routes_get).should be_true
File.read("./config/routes.cr").includes?(routes_delete).should be_true
File.read("./src/controllers/animal_controller.cr").should eq output_class

Amber::CLI::Spec.cleanup
end
end
Expand Down
9 changes: 5 additions & 4 deletions spec/amber/router/context_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ require "../../../spec_helper"

describe HTTP::Server::Context do
describe "#override_request_method!" do
describe "when X-HTTP-Method-Overrid is present" do
context "when X-HTTP-Method-Override is present" do
it "overrides form POST method to PUT, PATCH, DELETE" do
%w(PUT PATCH DELETE).each do |method|
header = HTTP::Headers.new
Expand Down Expand Up @@ -147,23 +147,24 @@ describe HTTP::Server::Context do
it "parses json hash" do
headers = HTTP::Headers.new
headers["Content-Type"] = "application/json"
body = "{\"test\":\"test\"}"
body = %({ "test": "test", "address": { "city": "New York" }})
request = HTTP::Request.new("POST", "/", headers, body)

context = create_context(request)

context.params["test"].should eq "test"
context.params["address"].as(Hash)["city"].should eq "New York"
end

it "parses json array" do
headers = HTTP::Headers.new
headers["Content-Type"] = "application/json"
body = "[\"test\",\"test2\"]"
body = %(["test", "test2"])
request = HTTP::Request.new("POST", "/", headers, body)

context = create_context(request)

context.params["_json"].should eq "[\"test\", \"test2\"]"
context.params["_json"].should eq %w(test test2)
end

it "parses files from multipart forms" do
Expand Down
88 changes: 58 additions & 30 deletions spec/amber/validations/params_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module Amber::Validators
describe BaseRule do
describe "#apply" do
it "raises error when missing field" do
params = HTTP::Params.parse("")
params = params_builder("")
rule = BaseRule.new("field", "") do
false
end
Expand All @@ -15,7 +15,7 @@ module Amber::Validators
end

it "returns true for given block" do
params = HTTP::Params.parse("field=val")
params = params_builder("field=val")
rule = BaseRule.new("field", "") do
true
end
Expand All @@ -24,7 +24,7 @@ module Amber::Validators
end

it "returns false for the given block" do
params = HTTP::Params.parse("field=val")
params = params_builder("field=val")
rule = BaseRule.new("field", "") do
false
end
Expand All @@ -35,7 +35,7 @@ module Amber::Validators

describe "#error" do
it "returns default error message" do
params = HTTP::Params.parse("field=val")
params = params_builder("field=val")
error_message = "Field field is required"
rule = BaseRule.new("field", nil) do
false
Expand All @@ -47,7 +47,7 @@ module Amber::Validators
end

it "returns the given error message" do
params = HTTP::Params.parse("field=val")
params = params_builder("field=val")
error_message = "You must provide this field"
rule = BaseRule.new("field", error_message) do
false
Expand All @@ -63,17 +63,17 @@ module Amber::Validators
describe OptionalRule do
describe "#apply" do
it "does not apply rule when field is missing" do
params = HTTP::Params.parse("")
params = params_builder("")
error_message = "You must provide this field"
rule = OptionalRule.new("field", error_message) do
false
true
end

rule.apply(params).should be_true
end

it "applies rule when field is present" do
params = HTTP::Params.parse("field=val")
params = params_builder("field=val")
error_message = "You must provide this field"
rule = OptionalRule.new("field", error_message) do
true
Expand All @@ -87,12 +87,12 @@ module Amber::Validators
describe Params do
describe "#validation" do
it "validates required field" do
http_params = HTTP::Params.parse("name=elias&last_name=perez&middle=j")
http_params = params_builder("name=elias&last_name=perez&middle=j")
validator = Validators::Params.new(http_params)

result = validator.validation do
required(:name) { |v| v.str? & !v.empty? }
required("last_name") { |v| v.str? & !v.empty? }
required(:name) { |v| !v.nil? }
required("last_name") { |v| !v.nil? }
end

validator.valid?.should be_true
Expand All @@ -102,12 +102,12 @@ module Amber::Validators
context "optional params" do
context "when missing" do
it "does not validate optional field" do
http_params = HTTP::Params.parse("last_name=&middle=j")
http_params = params_builder("last_name=&middle=j")
validator = Validators::Params.new(http_params)

result = validator.validation do
optional(:name) { |v| v.empty? }
required("last_name") { |v| v.empty? }
optional(:name) { |v| !v.nil? }
required("last_name") { |v| !v.nil? }
end

validator.valid?.should be_true
Expand All @@ -117,40 +117,58 @@ module Amber::Validators

context "when present" do
it "validates optional field" do
http_params = HTTP::Params.parse("name=")
http_params = params_builder("name=")
validator = Validators::Params.new(http_params)

result = validator.validation do
optional(:name) { |v| !v.empty? }
optional(:name) { |v| v.nil? }
end

validator.valid?.should be_false
validator.errors.size.should eq 1
end
end

context "casting" do
it "returns false for the given block" do
params = params_builder("name=john&number=1&price=3.45&list=[1,2,3]")
validator = Validators::Params.new(params)
validator.validation do
required(:name) { |f| !f.to_s.empty? }
required(:number) { |f| f.as(String).to_i > 0 }
required(:price) { |f| f.as(String).to_f == 3.45 }
required(:list) do |f|
list = JSON.parse(f.as(String)).as_a
(list == [1, 2, 3] && !list.includes? 6)
end
end

validator.valid?.should be_truthy
end
end
end
end

describe "#valid?" do
it "returns false with invalid fields" do
http_params = HTTP::Params.parse("name=john&last_name=doe&middle=j")
http_params = params_builder("name=john&last_name=doe&middle=j")
validator = Validators::Params.new(http_params)

validator.validation do
required("name") { |v| v.empty? }
required("last_name") { |v| v.str? & !v.empty? }
required("name") { |v| v.nil? }
required("last_name") { |v| !v.nil? }
end

validator.valid?.should be_false
end

it "returns false when key does not exist" do
http_params = HTTP::Params.parse("name=elias")
http_params = params_builder("name=elias")
validator = Validators::Params.new(http_params)
result = {"nonexisting" => {nil, "Param [nonexisting] does not exist."}}

validator.validation do
required("nonexisting") { |v| v.str? & !v.empty? }
required("nonexisting") { |v| !v.nil? }
end

expect_raises Exceptions::Validator::InvalidParam do
Expand All @@ -159,12 +177,12 @@ module Amber::Validators
end

it "returns true with valid fields" do
http_params = HTTP::Params.parse("name=elias&last_name=perez&middle=j")
http_params = params_builder("name=elias&last_name=perez&middle=j")
validator = Validators::Params.new(http_params)

validator.validation do
required("name") { |v| v.str? & !v.empty? }
required("last_name") { |v| v.str? & !v.empty? }
required("name") { |v| !v.nil? }
required("last_name") { |v| !v.nil? }
end

validator.valid?.should be_true
Expand All @@ -173,12 +191,12 @@ module Amber::Validators

describe "#validate!" do
it "raises error on failed validation" do
http_params = HTTP::Params.parse("name=elias&last_name=perez&middle=j")
http_params = params_builder("name=&last_name=&middle=j")
validator = Validators::Params.new(http_params)

validator.validation do
required("name") { |v| v.str? & v.empty? }
required("last_name") { |v| v.str? & !v.empty? }
required("name") { |v| !v.to_s.empty? }
required("last_name") { |v| !v.to_s.empty? }
end

expect_raises Exceptions::Validator::ValidationFailed do
Expand All @@ -187,17 +205,27 @@ module Amber::Validators
end

it "returns validated params on successful validation" do
http_params = HTTP::Params.parse("name=elias&last_name=perez&middle=j")
http_params = params_builder("name=elias&last_name=perez&middle=j")
validator = Validators::Params.new(http_params)
result : Hash(String, String) = {"name" => "elias", "last_name" => "perez"}

validator.validation do
required("name") { |v| v.str? & !v.empty? }
required("last_name") { |v| v.str? & !v.empty? }
required("name") { |v| !v.nil? }
required("last_name") { |v| !v.nil? }
end

validator.validate!.should eq result
end
end
end
end

def params_builder(body = "")
params = {} of String | Symbol => Amber::Router::ParamsType
return params if body.empty?

body.tr("?", "").split("&").each_with_object(params) do |item, params|
key, value = item.split("=")
params[key] = value
end
end
13 changes: 7 additions & 6 deletions spec/spec_helper.cr
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
# NOTE: Constants should be set before require begins.

ENV["AMBER_ENV"] = "test"
TEST_PATH = "spec/support/sample"
PUBLIC_PATH = TEST_PATH + "/public"
VIEWS_PATH = TEST_PATH + "/views"
TESTING_APP = "./tmp/test_app"
APP_TPL_PATH = "./src/amber/cli/templates/app"
CURRENT_DIR = Dir.current
TEST_PATH = "spec/support/sample"
PUBLIC_PATH = TEST_PATH + "/public"
VIEWS_PATH = TEST_PATH + "/views"
TEST_APP_NAME = "test_app"
TESTING_APP = "./tmp/#{TEST_APP_NAME}"
APP_TPL_PATH = "./src/amber/cli/templates/app"
CURRENT_DIR = Dir.current

require "http"
require "spec"
Expand Down
2 changes: 1 addition & 1 deletion src/amber/cli/commands.cr
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ module Amber::CLI
help desc: "# Describe available commands and usages"
string ["-t", "--template"], desc: "# Preconfigure for selected template engine. Options: slang | ecr", default: "slang"
string ["-d", "--database"], desc: "# Preconfigure for selected database. Options: pg | mysql | sqlite", default: "pg"
string ["-m", "--model"], desc: "# Preconfigure for selected model. Options: granite | crecto", default: "granite"
string ["-m", "--model"], desc: "# Preconfigure for selected model. Options: granite | crecto", default: "granite"
end
end
end
4 changes: 2 additions & 2 deletions src/amber/cli/templates/auth.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -37,7 +37,7 @@ module Amber::CLI
add_plugs :web, <<-PLUGS
plug Authenticate.new
PLUGS

add_dependencies <<-DEPENDENCY
require "../src/models/**"
require "../src/handlers/**"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ class RegistrationController < ApplicationController
end

def create
<%= @name %> = <%= @name.capitalize %>.new(params.to_h.select(<%= @visible_fields %>))
<%= @name %>.password = params["password"]
<%= @name %> = <%= @name.capitalize %>.new(registration_params.validate!)
<%= @name %>.password = params["password"].to_s

if <%= @name %>.valid? && <%= @name %>.save
session[:<%= @name %>_id] = <%= @name %>.id
Expand All @@ -17,4 +17,11 @@ class RegistrationController < ApplicationController
render("new.<%= @language %>")
end
end

private def registration_params
params.validation do
required(:email) { |f| !f.nil? }
required(:password) { |f| !f.nil? }
end
end
end

0 comments on commit ff44e8f

Please sign in to comment.