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

elorest · 2017-11-06T07:06:59Z

Description of the Change

A few small changes to allow extensions to work for choosing content type. I've also modified the response priority so that it will return a 406 if you only ask for one content type and it's not defined. If you ask for more it will return the first match or the first defined response.

Youtube Example

https://www.youtube.com/watch?v=6KNjWDRUo_c

…extensions

marksiemers

Just a couple small changes requested.

I do have a larger question though. Does this eagerly render all response types?

If the controller action provides 5 different response types, will it render them all and just return the correct one?

If that is the case, I think it should be designed where the methods html, xml, etc. accept a block or proc and only invoke when needed for the response.

marksiemers · 2017-11-06T07:33:08Z

src/amber/controller/helpers/responders.cr

      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])


Should this be - "Response Not Acceptable" ?

marksiemers · 2017-11-06T07:36:08Z

src/amber/router/router.cr

+        if result.found?
+          result
+        else
+          @routes.find build_node(http_verb, resource.sub(/\.[^$\/]+$/, ""))


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.

marksiemers · 2017-11-06T07:44:08Z

I know this isn't part of this change, but https://github.com/amberframework/amber/pull/367/files#diff-e43f8575da93c21cf741fdd422c2b60cR78
Should be changed from "Response unexceptable." to "Response Not Acceptable."

eliasjpr

Left some comments. Forgot to mention that theres is an existing format method in the context.cr that you can reuse/re-purpose https://github.com/amberframework/amber/blob/master/src/amber/router/context.cr#L72

Nice and clean PR!

eliasjpr · 2017-11-06T12:12:24Z

src/amber/controller/helpers/responders.cr

@@ -51,8 +61,9 @@ 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(Content::REGEX).try(&.[1]))


What about using .last instead of &.[1] since is more readable?

A regex match returns a hash instead of an array, 1 is just a key for the first capture group. Similar to $1 in perl.

Does it break with request.path.match(Content::REGEX)[1]? ?

eliasjpr · 2017-11-06T12:14:40Z

src/amber/controller/helpers/responders.cr

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


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

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

eliasjpr · 2017-11-06T12:21:02Z

spec/amber/controller/base_spec.cr

@@ -83,6 +83,39 @@ module Amber::Controller
        context.request.headers["Accept"] = "text/plain"
        ResponsesController.new(context).index.should eq expected_result
      end
+


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

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

eliasjpr · 2017-11-06T12:23:05Z

src/amber/router/params_parser.cr

@@ -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


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

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.

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.

marksiemers · 2017-11-06T18:07:07Z

Per discussion with @elorest - invoking the methods without blocks is fast. If it becomes a problem, we can revisit using blocks/procs. For this PR, eager loaded strings are good.

eliasjpr

Very small suggestions and 1 non blocking comment

eliasjpr · 2017-11-07T00:43:40Z

src/amber/router/params_parser.cr

@@ -29,7 +29,7 @@ module Amber::Router
    def parse_params
      parse_part(request.query)
      if content_type = request.headers["Content-Type"]?
-        parse_multipart if content_type.try(&.starts_with?(MULTIPART_FORM))
+          parse_multipart if content_type.try(&.starts_with?(MULTIPART_FORM))


remove extra identation

eliasjpr · 2017-11-07T00:43:44Z

src/amber/router/params_parser.cr

+    end
+
+    def route_params_without_ext
+      rparams = route.params


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

Thanks for splitting it into 2 separate methods.

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?

eliasjpr · 2017-11-07T00:46:53Z

src/amber/controller/helpers/responders.cr

@@ -61,10 +62,11 @@ module Amber::Controller::Helpers

    private def requested_responses
      req_responses = Array(String).new
-      if (ext = request.path.match(Content::REGEX).try(&.[1]))
-        req_responses << Content::TYPE[ext]
+      path_ext = request.path.match(Content::REGEX).try(&.[1])


Non blocking comment: This would probably read better in a private method called format.

def requested_format request.path.match(Content::REGEX).try(&.[1]) end

Possibly but it's only one source of format. The private method that it's in is already tasked with finding the requested response types (formats) from currently 2 locations.

marksiemers

Some naming, style, and syntax changes requested.

Also, if we can check for the correct response content-type header in the specs.

marksiemers · 2017-11-07T01:46:54Z

src/amber/router/params_parser.cr

      end
+      route.params
    end


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?

marksiemers · 2017-11-07T01:48:19Z

spec/amber/controller/base_spec.cr

+      end
+
+      it "responds with 406 for path.text when text hasn't been defined" do
+        expected_result = "Response unexceptable."


"Response Not Acceptable"

marksiemers · 2017-11-07T01:50:51Z

spec/amber/controller/base_spec.cr

+        context.request.path = "/response/1.texas"
+        context.request.headers["Accept"] = "text/html"
+        ResponsesController.new(context).index.should eq expected_result
+      end


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

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.

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

I can set it though.

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 :)

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

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.

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

marksiemers · 2017-11-07T02:00:19Z

src/amber/controller/helpers/responders.cr

@@ -35,13 +45,14 @@ module Amber::Controller::Helpers
      end


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.

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

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

marksiemers · 2017-11-07T02:03:02Z

src/amber/controller/helpers/responders.cr

      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


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?

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.

marksiemers · 2017-11-07T02:05:29Z

src/amber/controller/helpers/responders.cr

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


ACCEPT_SEPARATOR_REGEX

marksiemers · 2017-11-07T02:05:42Z

src/amber/controller/helpers/responders.cr

+        xml:  "application/xml",
+      }
+
+      REGEX                     = /\.(#{TYPE.keys.join("|")})$/


CONTENT_TYPE_REGEX

marksiemers · 2017-11-07T02:35:41Z

src/amber/controller/helpers/responders.cr

+      if (path_ext)
+        req_responses << Content::TYPE[path_ext]
+      elsif (accept = context.request.headers["Accept"]?) && !accept.empty?
+        accepts = accept.split(";").first?.try(&.split(Content::COMMA_OR_COMMASPACE_REGEX))


This is my inner ruby-ist thinking out loud.

Take it or leave it.

private def type_from_path_extension : String | Nil return @type_from_path_extension if @type_from_path_extension_checked @type_from_path_extension_checked = true @type_from_path_extension = request.path.match(Content::REGEX).try(&.[1]) end private def accepts_from_headers : Array(String) @accepts_from_headers ||= parse_accepts_from_headers end private def parse_accepts_from_headers : Array(String) accept_header = context.request.headers["Accept"]? if accept_header accept_header.split(";").first?.try(&.split(Content::COMMA_OR_COMMASPACE_REGEX)) else [] of String end end private def requested_responses : Array(String) return [type_from_path_extension] unless type_from_path_extension.nil? accepts_from_headers end

I do like the idea of not defining the Array in the beginning.

I guess the main thing for me here is avoiding as many branches and memory allocations as I can.

Maybe this:

private def type_from_path_extension : String | Nil request.path.match(Content::REGEX).try(&.[1]) end private def parse_accepts_from_headers : Array(String) accept_header = context.request.headers["Accept"]? if accept_header accept_header.split(";").first?.try(&.split(Content::COMMA_OR_COMMASPACE_REGEX)) else [] of String end end private def requested_responses : Array(String) if (type_from_path = type_from_path_extension) [type_from_path] else parse_accepts_from_headers end end

drujensen

Lgtm

marksiemers

If testing for content-type isn't a huge uphill and is possible, add that, otherwise good to go.

faustinoaq · 2017-11-10T19:57:44Z

src/amber/controller/helpers/responders.cr

    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])


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?

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.

* works but reveals another issue with params... :{ * handled extensions for respond_with * added tests and made video demo * changed from response back to content * modified some code as requested * accidentally undid something before my last commit * fixed requested changes * changed unacceptable response * refactored a bit more * done * added content-type and status_code to tests

* works but reveals another issue with params... :{ * handled extensions for respond_with * added tests and made video demo * changed from response back to content * modified some code as requested * accidentally undid something before my last commit * fixed requested changes * changed unacceptable response * refactored a bit more * done * added content-type and status_code to tests Former-commit-id: 8362a62

elorest added 4 commits November 5, 2017 00:38

works but reveals another issue with params... :{

27b082c

Merge branch 'master' of github.com:Amber-Crystal/amber into is/path_…

6c26a1c

…extensions

handled extensions for respond_with

a4b04e9

added tests and made video demo

f9eae91

elorest requested review from drujensen, marksiemers, eliasjpr, faustinoaq and fridgerator November 6, 2017 07:08

changed from response back to content

d391c70

marksiemers suggested changes Nov 6, 2017

View reviewed changes

faustinoaq approved these changes Nov 6, 2017

View reviewed changes

eliasjpr reviewed Nov 6, 2017

View reviewed changes

modified some code as requested

d8fbfdf

eliasjpr reviewed Nov 7, 2017

View reviewed changes

elorest added 2 commits November 6, 2017 18:04

accidentally undid something before my last commit

dce4065

fixed requested changes

cfe11a1

marksiemers suggested changes Nov 7, 2017

View reviewed changes

drujensen approved these changes Nov 7, 2017

View reviewed changes

elorest added 3 commits November 6, 2017 20:20

changed unacceptable response

606f906

refactored a bit more

d2eac6f

done

b037006

marksiemers approved these changes Nov 7, 2017

View reviewed changes

elorest added 3 commits November 7, 2017 16:05

Merge branch 'master' into is/path_extensions

a03ed80

added content-type and status_code to tests

1452b8b

Merge branch 'master' into is/path_extensions

25b972e

elorest merged commit 5dded7e into master Nov 8, 2017

eliasjpr deleted the is/path_extensions branch November 8, 2017 00:34

faustinoaq reviewed Nov 10, 2017

View reviewed changes

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

eliasjpr mentioned this pull request Nov 14, 2017

render text / plain text in controller response? #306

Closed

eliasjpr added this to the 0.3.5 milestone Nov 14, 2017

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

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

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

Conversation

elorest commented Nov 6, 2017 • edited

Description of the Change

Youtube Example

marksiemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marksiemers commented Nov 6, 2017

eliasjpr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marksiemers commented Nov 6, 2017

eliasjpr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorest Nov 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorest Nov 7, 2017 • edited

Choose a reason for hiding this comment

marksiemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorest Nov 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drujensen left a comment

Choose a reason for hiding this comment

marksiemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elorest commented Nov 6, 2017 •

edited

elorest Nov 7, 2017 •

edited

elorest Nov 7, 2017 •

edited

elorest Nov 7, 2017 •

edited