Add testcase for absolute URI #42 #44
Conversation
| test "handles absoluteURI", %{port: port} do | ||
| request = """ | ||
| GET http://www.raxx.com/ HTTP/1.1 | ||
| """ |
There was a problem hiding this comment.
To make sure this is a valid request there needs to be a full empty line after headers.
| @@ -49,6 +49,15 @@ defmodule Ace.HTTP.RequestTest do | |||
| assert upload.type == "text/plain" | |||
| end | |||
|
|
|||
| test "handles absoluteURI", %{port: port} do | |||
| request = """ | |||
| GET http://www.raxx.com/ HTTP/1.1 | |||
There was a problem hiding this comment.
I'm not sure but I suspect that even though this is using an absolute url in the start-line there is still a requirement for a Host header.
I started reading the spec but found it to be inconclusive https://tools.ietf.org/html/rfc7230#section-5.3.2
There was a problem hiding this comment.
You are right.A client MUST send a Host header field in all HTTP/1.1 request messages.
There was a problem hiding this comment.
Additionally:
A server MUST respond with a 400 (Bad Request) status code to any
HTTP/1.1 request message that lacks a Host header field and to any
request message that contains more than one Host header field or a
Host header field with an invalid field-value.
|
Good point on the 400 for missing Want to have a crack at the implementation, I'm a bit busy so will probably be able to take a look more on the weekend. Happy to keep offering comments before then if you want. |
| {:ok, {:http_request, method, http_uri, _version}, rest} -> | ||
| path_string = | ||
| case http_uri do | ||
| {:abs_path, path_string} -> path_string |
There was a problem hiding this comment.
Can you start a newline after every ->
| path_string = | ||
| case http_uri do | ||
| {:abs_path, path_string} -> path_string | ||
| {:absoluteURI, _scheme, _host, _port, path_string} -> path_string # Throw away the rest of the absolute URI since we are not proxying |
There was a problem hiding this comment.
Put comment on line above as it is too long otherwise
|
There is definitely more to this issue as a absoluteURI should be received only if we are proxying yet you note that we are not proxying. 🤔 |
|
To finish this PR can you bump the patch number on the mix file version |
|
@CrowdHailer made those changes, seems like Ace.HTTP doesn't have a CHANGELOG though? |
|
@samphilipd unless you want to make a changelog it's probably staying that way for the moment. I think eventually I want to bundle AceHTTP and AceHTTP2 together and that will happily mean one changlog |
No description provided.