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.

Sign up for GitHub

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

Jump to bottom

Respond_with uri extensions path.[json|xml|text|txt|html] #367

Merged
merged 14 commits into from
Nov 8, 2017
Merged

Respond_with uri extensions path.[json|xml|text|txt|html] #367

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
27b082c
works but reveals another issue with params... :{
elorest Nov 5, 2017
6c26a1c
Merge branch 'master' of github.com:Amber-Crystal/amber into is/path_…
elorest Nov 6, 2017
a4b04e9
handled extensions for respond_with
elorest Nov 6, 2017
f9eae91
added tests and made video demo
elorest Nov 6, 2017
d391c70
changed from response back to content
elorest Nov 6, 2017
d8fbfdf
modified some code as requested
elorest Nov 7, 2017
dce4065
accidentally undid something before my last commit
elorest Nov 7, 2017
cfe11a1
fixed requested changes
elorest Nov 7, 2017
606f906
changed unacceptable response
elorest Nov 7, 2017
d2eac6f
refactored a bit more
elorest Nov 7, 2017
b037006
done
elorest Nov 7, 2017
a03ed80
Merge branch 'master' into is/path_extensions
elorest Nov 7, 2017
1452b8b
added content-type and status_code to tests
elorest Nov 7, 2017
25b972e
Merge branch 'master' into is/path_extensions
elorest Nov 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions spec/amber/controller/base_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,44 @@ module Amber::Controller
context.request.headers["Accept"] = "text/plain"
ResponsesController.new(context).index.should eq expected_result
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for an invalid (unsupported) extension?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'll commit that in a second. I already have a test for a supported but undefined extension.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

it "responds with json for path.json" do
expected_result = %({"type":"json","name":"Amberator"})
context.request.path = "/response/1.json"
ResponsesController.new(context).index.should eq expected_result
end

it "responds with xml for path.xml" do
expected_result = "<xml><body><h1>Sort of xml</h1></body></xml>"
context.request.path = "/response/1.xml"
ResponsesController.new(context).index.should eq expected_result
end

it "responds with text for path.txt" do
expected_result = "Hello I'm text!"
context.request.path = "/response/1.txt"
ResponsesController.new(context).index.should eq expected_result
end

it "responds with text for path.text" do
expected_result = "Hello I'm text!"
context.request.path = "/response/1.text"
ResponsesController.new(context).index.should eq expected_result
end

it "responds with 406 for path.text when text hasn't been defined" do
expected_result = "Response Not Acceptable."
context.request.path = "/response/1.text"
ResponsesController.new(context).show.should eq expected_result
context.response.status_code.should eq 406
end

it "respond_with default or accept header request if extension is invalid" do
expected_result = "<html><body><h1>Elorest <3 Amber</h1></body></html>"
context.request.path = "/response/1.texas"
context.request.headers["Accept"] = "text/html"
ResponsesController.new(context).index.should eq expected_result
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to check the content-type header in these tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. I'll check it I guess. Since they're all set at the some time I didn't think it was important to set that since the response was right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I'm getting an error that content_type doesn't exist on response. When I try to test it's value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can set it though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In base_spec we should be verifying the context context.response as well. This is ResponsesController.new(context).index.should eq expected_result totally valid, at least confirm the content-type and status code are correct. For instance

context.response.body.should
context.response.status_code.should
context.response.headers["Content-Type"].should

I think it is important that is done this way because something else could be changing the response. This is how I found the issue with the redirect :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you are covering testing status code and content-type in the unit test for Responders::Content context.response.status_code.should
context.response.headers["Content-Type"].should

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I was put off by the fact that there was no getter for content_type. Makes sense that it's getting written to the header though. I'll test that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context.response.headers["Content-Type"]? will report a failure with helpful info rather than an error when nil

end

describe "#render" do
Expand Down
7 changes: 7 additions & 0 deletions spec/support/fixtures/controller_fixtures.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,11 @@ class ResponsesController < Amber::Controller::Base
text "Hello I'm text!"
end
end

def show
respond_with do
html "<html><body><h1>Elorest <3 Amber</h1></body></html>"
json type: "json", name: "Amberator"
end
end
end
48 changes: 32 additions & 16 deletions src/amber/controller/helpers/responders.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,17 @@
module Amber::Controller::Helpers
module Responders
class Content
TYPE = {html: "text/html", json: "application/json", text: "text/plain", xml: "application/xml"}
TYPE = {
html: "text/html",
json: "application/json",
txt: "text/plain",
text: "text/plain",
xml: "application/xml",
}

TYPE_EXT_REGEX = /\.(#{TYPE.keys.join("|")})$/
ACCEPT_SEPARATOR_REGEX = /,|,\s/

@requested_responses : Array(String)
@available_responses = Hash(String, String).new
@type : String? = nil
Expand Down Expand Up @@ -31,40 +41,46 @@ module Amber::Controller::Helpers
end

def type
select_type.to_s
(@type ||= select_type).to_s
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move @type ||= into this method? rather than needing the begin statement down below.

def type
  @type ||= select_type.to_s
end

I like the convention of

def property_name
  @property_name ||= calculate_property_name
end

Of course, 'calculate' can be 'select' or 'generate' - what ever verb makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have done that but I needed to return @type.to_s so that @type wouldn't have the possibility of being Nil.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did come up with a solution here that I think we'll both like.


def body
@available_responses[type]
@available_responses[type]?
end

private def select_type
@type ||= begin
raise "You must define at least one response_type." if @available_responses.empty?
@requested_responses << @available_responses.keys.first
@requested_responses.find do |resp|
@available_responses.keys.includes?(resp)
end
raise "You must define at least one response_type." if @available_responses.empty?
# NOTE: If only one response is requested don't return something else.
@requested_responses << @available_responses.keys.first if @requested_responses.size != 1
@requested_responses.find do |resp|
@available_responses.keys.includes?(resp)
end
end
end

private def requested_responses
req_responses = Array(String).new
private def extension_request_type
path_ext = request.path.match(Content::TYPE_EXT_REGEX).try(&.[1])
return [Content::TYPE[path_ext]] if path_ext
end

if (accept = context.request.headers["Accept"]?) && !accept.empty?
accepts = accept.split(";").first?.try(&.split(/,|,\s/))
req_responses.concat(accepts) if accepts.is_a?(Array) && accepts.any?
private def accepts_request_type
accept = context.request.headers["Accept"]?
if accept && !accept.empty?
accepts = accept.split(";").first?.try(&.split(Content::ACCEPT_SEPARATOR_REGEX))
return accepts if !accepts.nil? && accepts.any?
end
req_responses
end

private def requested_responses
extension_request_type || accepts_request_type || [] of String
end

protected def respond_with(&block)
content = with Content.new(requested_responses) yield
if content.body
set_response(body: content.body, status_code: 200, content_type: content.type)
else
set_response(body: "Response unexceptable.", status_code: 406, content_type: Content::TYPE[:text])
set_response(body: "Response Not Acceptable.", status_code: 406, content_type: Content::TYPE[:text])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @elorest I didn't see it 😅

Why not raise Amber::Exception::NoAcceptable so we can use Amber::Pipe::Error here 👉 https://github.com/amberframework/amber/blob/master/src/amber/router/pipe/error.cr#L19 and be hable to customize the 406 reponse. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that we didn't want to raise exceptions because it was slower or something. I remember @eliasjpr saying that was the reason he made halt instead of just raising and exception to handle later. Also since I'm setting a response anyone it sort of seems natural to set the other response and message.

end
end

Expand Down
13 changes: 12 additions & 1 deletion src/amber/router/params_parser.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,18 @@ module Amber::Router
end

def merge_route_params
route_params.each { |k, v| params[k] = v }
route_params_without_ext.each do |k, v|
params[k] = v
end
end

def route_params_without_ext
rparams = route.params
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use _paramsor route_params instead of rparams, the latter can be confusing.

Thanks for splitting it into 2 separate methods.

Copy link
Member Author

@elorest elorest Nov 7, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted something short and temporary what wouldn't overwrite anything else. To me rparams looked better than _params since it was still more descriptive. WDYT?

unless rparams.empty?
key = rparams.keys.last
rparams[key] = rparams[key].sub(Controller::Base::Content::TYPE_EXT_REGEX, "")
end
route.params
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For line 90, rparams[key] = r.params[key] ... Is the thing on the right supposed to be rparams (sans dot)? If not, r seems like too short of a method name.

And changing the value at rparams[key] also changes it in route.params? If acting destructively on route.params and returning it, why the need for rparams?


def route_params
Expand Down
8 changes: 7 additions & 1 deletion src/amber/router/router.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module Amber
# This is the main application handler all routers should finally hit this
# handler.
class Router
PATH_EXT_REGEX = /\.[^$\/]+$/
property :routes, :socket_routes

def initialize
Expand Down Expand Up @@ -81,7 +82,12 @@ module Amber
end

def match(http_verb, resource) : Radix::Result(Amber::Route)
@routes.find build_node(http_verb, resource)
result = @routes.find build_node(http_verb, resource)
if result.found?
result
else
@routes.find build_node(http_verb, resource.sub(PATH_EXT_REGEX, ""))
end
end

private def merge_params(params, context)
Expand Down