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 4 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
33 changes: 33 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,39 @@ 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 unexceptable."
Copy link
Contributor

Choose a reason for hiding this comment

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

"Response Not Acceptable"

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.

context.request.path = "/response/1.text"
ResponsesController.new(context).show.should eq expected_result
context.response.status_code.should eq 406
end

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
29 changes: 20 additions & 9 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,16 @@
module Amber::Controller::Helpers
module Responders
class Content
TYPE = {html: "text/html", json: "application/json", text: "text/plain", xml: "application/xml"}
class Response
TYPE = {
html: "text/html",
json: "application/json",
txt: "text/plain",
text: "text/plain",
xml: "application/xml",
}

REGEX = /\.(#{TYPE.keys.join("|")})$/

@requested_responses : Array(String)
@available_responses = Hash(String, String).new
@type : String? = nil
Expand Down Expand Up @@ -35,13 +44,14 @@ module Amber::Controller::Helpers
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
# NOTE: If only one response is requested don't return something else.
@requested_responses << @available_responses.keys.first if @requested_responses.size != 1
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this is "non-strict" mode - where even if they didn't explicitly request the type we return the first available response (if they had more than one thing in the 'accept' header), correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

While I feel that if json is the only thing available it should be return even if the browser is asking for html, it seems like a different case if someone asks for domain.com/cats.json. In that case it will return 406 if json doesn't exist.

@requested_responses.find do |resp|
@available_responses.keys.includes?(resp)
end
Expand All @@ -51,24 +61,25 @@ module Amber::Controller::Helpers

private def requested_responses
req_responses = Array(String).new

if (accept = context.request.headers["Accept"]?) && !accept.empty?
if (ext = request.path.match(Response::REGEX).try(&.[1]))
req_responses << Response::TYPE[ext]
elsif (accept = context.request.headers["Accept"]?) && !accept.empty?
accepts = accept.split(";").first?.try(&.split(/,|,\s/))
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this regex /,|,\s/ a constant that shows intent

Copy link
Member Author

Choose a reason for hiding this comment

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

To me 4 regex characters are easier to read than tracking down the definition of a constant, but sure.

req_responses.concat(accepts) if accepts.is_a?(Array) && accepts.any?
end
req_responses
end

protected def respond_with(&block)
content = with Content.new(requested_responses) yield
content = with Response.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 unexceptable.", status_code: 406, content_type: Response::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.

Should this be - "Response Not Acceptable" ?

end
end

private def set_response(body, status_code = 200, content_type = Content::TYPE[:html])
private def set_response(body, status_code = 200, content_type = Response::TYPE[:html])
context.response.status_code = status_code
context.response.content_type = content_type
context.content = body
Expand Down
8 changes: 7 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,13 @@ module Amber::Router
end

def merge_route_params
route_params.each { |k, v| params[k] = v }
route_params.each do |k, v|
if k == route_params.keys.last
Copy link
Contributor

Choose a reason for hiding this comment

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

This 2 lines should probably be in its own method, called format. I forgot to tell you in the previous PR that we have already define a similar method in the context https://github.com/amberframework/amber/blob/master/src/amber/router/context.cr#L72

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 format currently being used for something? My need is just to remove 5 ext's from the end of the last path_param. I could extract it to a method but I don't think moving it to context 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.

If it don't remove these then paths like /pets/1.json try looking up an id of 1.json in Granite which fails. This isn't where I'm detecting the format, since it will only sometimes be on a param I have to parse it from the path, for responses.

params[k] = v.sub(Controller::Base::Response::REGEX, "")
else
params[k] = v
end
end
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
7 changes: 6 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 @@ -81,7 +81,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(/\.[^$\/]+$/, ""))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this regex be named with a constant to indicate what it is for? It is meant to chop off the extension, right? .json or .text, etc.

end
end

private def merge_params(params, context)
Expand Down