Skip to content

Conversation

@kevindew
Copy link

This change proposes changing the exception class used when invalid syntax is used in a HTTP Header from ArgumentError to Net::HTTPHeaderSyntaxError.

The reason I am proposing this change is to make it easier for applications and libraries to catch errors that result as part of HTTP response.

For example, if you're calling: Net::HTTP.get("www.example.com", "/") it is confusing to receive an ArgumentError exception as this isn't a problem with the arguments provided to Net::HTTP#get and instead is a lower-level parsing concern of the work done by Net::HTTP#get.

This exception isn't caught by common libraries that wrap Net::HTTP, for example Faraday: https://github.com/lostisland/faraday-net_http/blob/616bd1c35c058da01f91af8c4f504141b4f2b427/lib/faraday/adapter/net_http.rb#L15-L31 and is a somewhat delicate error to catch when making a request as a developer likely wants to distinguish between a genuine ArgumentError to usage of Net::HTTP and what is an error in the syntax in the HTTP request/response.

There was existing precedence for the use of this exception in Net::HTTPHeader, so this seemed like a change that could made and make the class behave consistently.

An example of usage is below.

Before this change:

irb(main):001:0> require 'webmock'; include WebMock::API; WebMock.enable!; stub_request(:any, "www.example.com").to_return
(headers: { "Invalid" => "Item with \r character" })
irb(main):002:0> Net::HTTP.get("www.example.com", "/")
/Users/kevin.dew/dev/net-http/lib/net/http/header.rb:85:in `set_field': header field value cannot include CR/LF (ArgumentError)

After this change:


irb(main):001:0> require 'webmock'; include WebMock::API; WebMock.enable!; stub_request(:any, "www.example.com").to_return
(headers: { "Invalid" => "Item with \r character" })
irb(main):002:0> Net::HTTP.get("www.example.com", "/")
/Users/kevin.dew/dev/net-http/lib/net/http/header.rb:87:in `set_field': header field value cannot include CR/LF (Net::HTTPHeaderSyntaxError)    => "Item with \r character" })
This change proposes changing the exception class used when invalid
syntax is used in a HTTP Header from ArgumentError to
Net::HTTPHeaderSyntaxError.

The reason I am proposing this change is to make it easier for
applications and libraries to catch errors that result as part of HTTP
response.

For example, if you're calling: `Net::HTTP.get("www.example.com", "/")`
it is confusing to receive an ArgumentError exception as this isn't a
problem with the arguments provided to `Net::HTTP#get` and instead is a
lower-level parsing concern of the work done by `Net::HTTP#get`.

This exception isn't caught by common libraries that wrap Net::HTTP, for
example Faraday: https://github.com/lostisland/faraday-net_http/blob/616bd1c35c058da01f91af8c4f504141b4f2b427/lib/faraday/adapter/net_http.rb#L15-L31
and is a somewhat delicate error to catch when making a request as a
developer likely wants to distinguish between a genuine ArgumentError to
usage of `Net::HTTP` and what is an error in the syntax in the HTTP
request/response.

There was existing precedence for the use of this exception in
Net::HTTPHeader, so this seemed like a change that could made and make
the class behave consistently.

An example of usage is below.

Before this change:

```
irb(main):001:0> require 'webmock'; include WebMock::API; WebMock.enable!; stub_request(:any, "www.example.com").to_return
(headers: { "Invalid" => "Item with \r character" })
irb(main):002:0> Net::HTTP.get("www.example.com", "/")
/Users/kevin.dew/dev/net-http/lib/net/http/header.rb:85:in `set_field': header field value cannot include CR/LF (ArgumentError)
```

After this change:

```

irb(main):001:0> require 'webmock'; include WebMock::API; WebMock.enable!; stub_request(:any, "www.example.com").to_return
(headers: { "Invalid" => "Item with \r character" })
irb(main):002:0> Net::HTTP.get("www.example.com", "/")
/Users/kevin.dew/dev/net-http/lib/net/http/header.rb:87:in `set_field': header field value cannot include CR/LF (Net::HTTPHeaderSyntaxError)    => "Item with \r character" })
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant