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

raise HTTP::Request::InvalidURIError for wrong url's #723

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/http/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ def make_request_uri(uri, opts)
uri.path = "/" if uri.path.empty?

uri
rescue Addressable::URI::InvalidURIError => e
raise HTTP::Request::InvalidURIError, e.message
Comment on lines +157 to +158
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should move this to HTTP::URI.parse instead? And probably call it HTTP::URI::InvalidError

end

# Creates request headers with cookies (if any) merged in
Expand Down
12 changes: 10 additions & 2 deletions lib/http/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ class UnsupportedMethodError < RequestError; end
# The scheme of given URI was not understood
class UnsupportedSchemeError < RequestError; end

# Provided url is missing or wrong
class InvalidURIError < RequestError; end

# Default User-Agent header value
USER_AGENT = "http.rb/#{HTTP::VERSION}"

Expand Down Expand Up @@ -90,8 +93,7 @@ def initialize(opts)
@uri = @uri_normalizer.call(opts.fetch(:uri))
@scheme = @uri.scheme.to_s.downcase.to_sym if @uri.scheme

raise(UnsupportedMethodError, "unknown method: #{verb}") unless METHODS.include?(@verb)
raise(UnsupportedSchemeError, "unknown scheme: #{scheme}") unless SCHEMES.include?(@scheme)
check_errors

@proxy = opts[:proxy] || {}
@version = opts[:version] || "1.1"
Expand Down Expand Up @@ -222,6 +224,12 @@ def inspect
# @return [String]
def_delegator :@uri, :host

def check_errors
raise(InvalidURIError, "empty uri provided") if @uri.host.nil?
raise(UnsupportedMethodError, "unknown method: #{verb}") unless METHODS.include?(@verb)
raise(UnsupportedSchemeError, "unknown scheme: #{scheme}") unless SCHEMES.include?(@scheme)
Comment on lines +228 to +230
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, all of the above should raise ArgumentError. Please make this method private. Furthermore, what are your thoughts on renaming it to validate!?

end

# @!attribute [r] port
# @return [Fixnum]
def port
Expand Down
2 changes: 1 addition & 1 deletion spec/lib/http/request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

it "requires URI to have scheme part" do
expect { HTTP::Request.new(:verb => :get, :uri => "example.com/") }.to \
raise_error(HTTP::Request::UnsupportedSchemeError)
raise_error(HTTP::Request::InvalidURIError)
end

it "provides a #scheme accessor" do
Expand Down
17 changes: 17 additions & 0 deletions spec/lib/http_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,23 @@ def setsockopt(*args)
end
end

describe "gives a proper error for invalid URLs" do
it "throws InvalidURIError on nil" do
expect { HTTP.get(nil) }.to raise_error(HTTP::Request::InvalidURIError)
expect { HTTP.post(nil, :body => "Hello") }.to raise_error(HTTP::Request::InvalidURIError)
end

it "throws InvalidURIError on empty" do
expect { HTTP.get("") }.to raise_error(HTTP::Request::InvalidURIError)
expect { HTTP.post("", :body => "Hello") }.to raise_error(HTTP::Request::InvalidURIError)
end

it "throws InvalidURIError for incorrect url" do
expect { HTTP.get("/") }.to raise_error(HTTP::Request::InvalidURIError)
expect { HTTP.get(":") }.to raise_error(HTTP::Request::InvalidURIError)
end
end

describe ".use" do
it "turns on given feature" do
client = HTTP.use :auto_deflate
Expand Down