Improve BaseHTTPClient #875

paulcsmith · 2019-08-23T23:09:20Z

This can be used for request specs. This will be used for the default auth specs when generating an API app with Auth.

I imagine this could be used for other HTTP Clients as well, but for now the main purpose is for testing Lucky apps

spec/lucky/base_http_client_spec.cr

edwardloveall

This will be really nice to use in tests. I had some code nitpicks, but other than that I think it's great.

edwardloveall · 2019-08-24T03:28:39Z

src/lucky/base_http_client.cr

+  # Set headers for requests
+  #
+  # ```
+  # # `content_type` will be normalized to `content-type`


I think you're trying to say:

Suggested change

# # `content_type` will be normalized to `content-type`

# # `content_type` will be normalized to `Content-Type`

Actually, I can't find this anywhere. Where does this happen? I see that headers have upcase called on them but I don't understand how _ -> -.

Headers are case insensitive! https://stackoverflow.com/questions/5258977/are-http-headers-case-sensitive So technically it does not capitalize them, but it does change the underscore to a hyphen. That's done by Crystal :D

Should I add a note that headers are case insensitive to the docs?

However it doesn't appear to change the underscore to a hyphen when setting (it does when getting the header value or the test would fail):

HTTP::Headers{"Foo" => "bar", "accept" => "text/csv", "accept-encoding" => "gzip, deflate", "content-length" => "2", "content_type" => "application/json", "host" => "localhost:6226", "user-agent" => "Crystal"}

I think most implementations of header parsers convert underscore to hyphen (like Crystal's does) but I think we should do it before setting just in case :)

https://github.com/crystal-lang/crystal/blob/5e6a1b672a8b06d2a819210b9905b806ec3d7c71/src/http/headers.cr#L3-L4

edwardloveall · 2019-08-24T12:53:25Z

spec/lucky/base_http_client_spec.cr

+          request = server.last_request
+          request.path.should eq "/hello"
+          request.method.should eq("POST")
+          request.body.not_nil!.gets_to_end.should eq(NamedTuple.new.to_json)


This caught my eye. As far as I can tell we're not sending anything as the post body. Is an empty post body somehow initialized to the json of a NamedTuple?

Now I see it down in the code with **params. Maybe it would be worth giving that param a type or a default so we didn't have to call not_nil! on it. It'd be cool if we could get rid of gets_to_end also, but I don't think that's as simple. Thoughts also on changing NamedTuple.new.to_json to "{}"?

The request is an HTTP::Request from Crystal so I can't easily change that without monkey patching it. Since end users won't be checking the request body I don't think it is worth monkey patching it.

I do like the idea about just using a string! That's a lot clearer

edwardloveall · 2019-08-24T13:00:26Z

spec/lucky/base_http_client_spec.cr

  {% end %}
 end

+private def with_fake_server(path : String, response_body : String)
+  TestServer.route(path: path, response_body: response_body)
+  Lucky::Server.temp_config(host: "localhost", port: 6226) do


Any reason behind the 6226 port? Could we name it in a constant? Does it need to be configurable?

Extracted a private method. It's the port of the test server

paulcsmith

Thanks for the reviews! I think I addressed everything

paulcsmith · 2019-08-24T13:17:32Z

spec/lucky/base_http_client_spec.cr

+          request = server.last_request
+          request.path.should eq "/hello"
+          request.method.should eq("POST")
+          request.body.not_nil!.gets_to_end.should eq(NamedTuple.new.to_json)


The request is an HTTP::Request from Crystal so I can't easily change that without monkey patching it. Since end users won't be checking the request body I don't think it is worth monkey patching it.

I do like the idea about just using a string! That's a lot clearer

paulcsmith · 2019-08-24T13:18:43Z

spec/lucky/base_http_client_spec.cr

  {% end %}
 end

+private def with_fake_server(path : String, response_body : String)
+  TestServer.route(path: path, response_body: response_body)
+  Lucky::Server.temp_config(host: "localhost", port: 6226) do


Extracted a private method. It's the port of the test server

paulcsmith · 2019-08-24T13:19:44Z

src/lucky/base_http_client.cr

+  # Set headers for requests
+  #
+  # ```
+  # # `content_type` will be normalized to `content-type`


Headers are case insensitive! https://stackoverflow.com/questions/5258977/are-http-headers-case-sensitive So technically it does not capitalize them, but it does change the underscore to a hyphen. That's done by Crystal :D

Should I add a note that headers are case insensitive to the docs?

paulcsmith · 2019-08-24T13:23:40Z

src/lucky/base_http_client.cr

+  # Set headers for requests
+  #
+  # ```
+  # # `content_type` will be normalized to `content-type`


However it doesn't appear to change the underscore to a hyphen when setting (it does when getting the header value or the test would fail):

HTTP::Headers{"Foo" => "bar", "accept" => "text/csv", "accept-encoding" => "gzip, deflate", "content-length" => "2", "content_type" => "application/json", "host" => "localhost:6226", "user-agent" => "Crystal"}

I think most implementations of header parsers convert underscore to hyphen (like Crystal's does) but I think we should do it before setting just in case :)

https://github.com/crystal-lang/crystal/blob/5e6a1b672a8b06d2a819210b9905b806ec3d7c71/src/http/headers.cr#L3-L4

edwardloveall · 2019-08-24T14:21:43Z

src/lucky/base_http_client.cr

+    @client.before_request do |request|
+      header_values = header_values
+        .to_h
+        .transform_keys(&.gsub("_", "_"))


Did you mean

Suggested change

.transform_keys(&.gsub("_", "_"))

.transform_keys(&.gsub("_", "-"))

Yeah :D And I had to tweak this a bit because there was a compile error

paulcsmith · 2019-08-24T14:57:44Z

Now I think it's good to go 🤣

paulcsmith requested review from edwardloveall and jwoertink August 23, 2019 23:09

paulcsmith force-pushed the pcs/add-base-http-client branch from 77adbc2 to 06c54c1 Compare August 23, 2019 23:10

jwoertink reviewed Aug 23, 2019

View reviewed changes

spec/lucky/base_http_client_spec.cr Outdated Show resolved Hide resolved

paulcsmith force-pushed the pcs/add-base-http-client branch from 06c54c1 to c1a3f40 Compare August 23, 2019 23:39

edwardloveall approved these changes Aug 24, 2019

View reviewed changes

paulcsmith commented Aug 24, 2019

View reviewed changes

edwardloveall reviewed Aug 24, 2019

View reviewed changes

Improve BaseHTTPClient

7e6185c

paulcsmith force-pushed the pcs/add-base-http-client branch from c611181 to 7e6185c Compare August 24, 2019 14:53

paulcsmith merged commit 0356c38 into master Aug 24, 2019

paulcsmith deleted the pcs/add-base-http-client branch August 24, 2019 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve BaseHTTPClient #875

Improve BaseHTTPClient #875

paulcsmith commented Aug 23, 2019 •

edited

Loading

edwardloveall left a comment

edwardloveall Aug 24, 2019

edwardloveall Aug 24, 2019

paulcsmith Aug 24, 2019

paulcsmith Aug 24, 2019 •

edited

Loading

edwardloveall Aug 24, 2019

edwardloveall Aug 24, 2019

paulcsmith Aug 24, 2019

edwardloveall Aug 24, 2019

paulcsmith Aug 24, 2019

paulcsmith left a comment

paulcsmith Aug 24, 2019

paulcsmith Aug 24, 2019

paulcsmith Aug 24, 2019

paulcsmith Aug 24, 2019 •

edited

Loading

edwardloveall Aug 24, 2019

paulcsmith Aug 24, 2019

paulcsmith commented Aug 24, 2019

	# # `content_type` will be normalized to `content-type`
	# # `content_type` will be normalized to `Content-Type`

	.transform_keys(&.gsub("_", "_"))
	.transform_keys(&.gsub("_", "-"))

Improve BaseHTTPClient #875

Improve BaseHTTPClient #875

Conversation

paulcsmith commented Aug 23, 2019 • edited Loading

edwardloveall 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

paulcsmith Aug 24, 2019 • edited Loading

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

paulcsmith 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

paulcsmith Aug 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcsmith commented Aug 24, 2019

paulcsmith commented Aug 23, 2019 •

edited

Loading

paulcsmith Aug 24, 2019 •

edited

Loading

paulcsmith Aug 24, 2019 •

edited

Loading