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.

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

Use Net::HTTPHeaderSyntaxError instead of ArgumentError #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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
Development

Successfully merging this pull request may close these issues.

None yet

1 participant