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

Preserve header casing as specified #576

Merged
merged 1 commit into from
Dec 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 32 additions & 9 deletions lib/http/headers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ class Headers

# Class constructor.
def initialize
# The @pile stores each header value using a three element array:
# 0 - the normalized header key, used for lookup
# 1 - the header key as it will be sent with a request
# 2 - the value
@pile = []
end

Expand All @@ -45,20 +49,39 @@ def delete(name)

# Appends header.
#
# @param [#to_s] name header name
# @param [String, Symbol] name header name. When specified as a string, the
# name is sent as-is. When specified as a symbol, the name is converted
# to a string of capitalized words separated by a dash. Word boundaries
# are determined by an underscore (`_`) or a dash (`-`).
# Ex: `:content_type` is sent as `"Content-Type"`, and `"auth_key"` (string)
# is sent as `"auth_key"`.
# @param [Array<#to_s>, #to_s] value header value(s) to be appended
# @return [void]
def add(name, value)
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to introduce optional keyword:

def add(name, value, normalize: nil)

Naturally, when normalize is false, no normalization should be done for the wire. When true - force normalization, and when nil - type dependent normalization.

name = normalize_header name.to_s
Array(value).each { |v| @pile << [name, validate_value(v)] }
lookup_name = normalize_header(name.to_s)
wire_name = case name
when String
name
when Symbol
lookup_name
else
raise HTTP::HeaderError, "HTTP header must be a String or Symbol: #{name.inspect}"
Copy link
Member

Choose a reason for hiding this comment

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

I think it should raise only if key is no ot responding to to_s and not a Symbol. Something like:

if name.is_a? Symbol
  # ...
elsif name.respond_to? :to_s
  # ...
else
  raise
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how that is helpful. Almost everything in ruby responds to to_s (unless it derives from BasicObject to explicitly opt out of default behavior).

If you want to allow more than just String (not sure what the use case is), we could support anything that responds to to_str, which is used to support implicit conversion to strings.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I agree. Then we need to update api doc to reflect this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to want to keep my current strict implementation (only allow String or Symbol), or do you want to use the more relaxed "anything that supports an implicit conversion to string (ie. responds to #to_str)"?

I think being strict makes sense here, especially since we have different behavior based on the data type.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with your way. Strict Symbol or String.

end
Array(value).each do |v|
@pile << [
lookup_name,
wire_name,
validate_value(v)
]
end
end

# Returns list of header values if any.
#
# @return [Array<String>]
def get(name)
name = normalize_header name.to_s
@pile.select { |k, _| k == name }.map { |_, v| v }
@pile.select { |k, _| k == name }.map { |_, _, v| v }
end

# Smart version of {#get}.
Expand Down Expand Up @@ -96,7 +119,7 @@ def to_h
#
# @return [Array<[String, String]>]
def to_a
@pile.map { |pair| pair.map(&:dup) }
@pile.map { |item| item[1..2] }
end

# Returns human-readable representation of `self` instance.
Expand All @@ -110,7 +133,7 @@ def inspect
#
# @return [Array<String>]
def keys
@pile.map { |k, _| k }.uniq
@pile.map { |_, k, _| k }.uniq
end

# Compares headers to another Headers or Array of key/value pairs
Expand All @@ -119,7 +142,7 @@ def keys
def ==(other)
return false unless other.respond_to? :to_a

@pile == other.to_a
to_a == other.to_a
end

# Calls the given block once for each key/value pair in headers container.
Expand All @@ -129,7 +152,7 @@ def ==(other)
def each
return to_enum(__method__) unless block_given?

@pile.each { |arr| yield(arr) }
@pile.each { |item| yield(item[1..2]) }
self
end

Expand All @@ -152,7 +175,7 @@ def each
# @api private
def initialize_copy(orig)
super
@pile = to_a
@pile = @pile.map(&:dup)
end

# Merges `other` headers into `self`.
Expand Down
7 changes: 5 additions & 2 deletions spec/lib/http/connection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@
<<-RESPONSE.gsub(/^\s*\| */, "").gsub(/\n/, "\r\n")
| HTTP/1.1 200 OK
| Content-Type: text
| foo_bar: 123
|
RESPONSE
end
end

it "reads data in parts" do
it "populates headers collection, preserving casing" do
connection.read_headers!
expect(connection.headers).to eq("Content-Type" => "text")
expect(connection.headers).to eq("Content-Type" => "text", "foo_bar" => "123")
expect(connection.headers["Foo-Bar"]).to eq("123")
expect(connection.headers["foo_bar"]).to eq("123")
end
end

Expand Down
31 changes: 24 additions & 7 deletions spec/lib/http/headers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
expect(headers["Accept"]).to eq "application/json"
end

it "normalizes header name" do
it "allows retrieval via normalized header name" do
headers.set :content_type, "application/json"
expect(headers["Content-Type"]).to eq "application/json"
end
Expand Down Expand Up @@ -54,7 +54,7 @@
expect(headers["Accept"]).to eq "application/json"
end

it "normalizes header name" do
it "allows retrieval via normalized header name" do
headers[:content_type] = "application/json"
expect(headers["Content-Type"]).to eq "application/json"
end
Expand All @@ -80,7 +80,7 @@
expect(headers["Content-Type"]).to be_nil
end

it "normalizes header name" do
it "removes header that matches normalized version of specified name" do
headers.delete :content_type
expect(headers["Content-Type"]).to be_nil
end
Expand All @@ -104,13 +104,13 @@
expect(headers["Accept"]).to eq "application/json"
end

it "normalizes header name" do
it "allows retrieval via normalized header name" do
headers.add :content_type, "application/json"
expect(headers["Content-Type"]).to eq "application/json"
end

it "appends new value if header exists" do
headers.add :set_cookie, "hoo=ray"
headers.add "Set-Cookie", "hoo=ray"
headers.add :set_cookie, "woo=hoo"
expect(headers["Set-Cookie"]).to eq %w[hoo=ray woo=hoo]
end
Expand All @@ -137,6 +137,11 @@
expect { headers.add "foo", "bar\nEvil-Header: evil-value" }.
to raise_error HTTP::HeaderError
end

it "fails when header name is not a String or Symbol" do
expect { headers.add 2, "foo" }.
to raise_error HTTP::HeaderError
end
end

describe "#get" do
Expand Down Expand Up @@ -314,6 +319,17 @@
)
end

it "yields header keys specified as symbols in normalized form" do
keys = headers.each.map(&:first)
expect(keys).to eq(["Set-Cookie", "Content-Type", "Set-Cookie"])
end

it "yields headers specified as strings without conversion" do
headers.add "X_kEy", "value"
keys = headers.each.map(&:first)
expect(keys).to eq(["Set-Cookie", "Content-Type", "Set-Cookie", "X_kEy"])
end

it "returns self instance if block given" do
expect(headers.each { |*| }).to be headers
end
Expand Down Expand Up @@ -490,14 +506,15 @@
end

context "with duplicate header keys (mixed case)" do
let(:headers) { {"Set-Cookie" => "hoo=ray", "set-cookie" => "woo=hoo"} }
let(:headers) { {"Set-Cookie" => "hoo=ray", "set_cookie" => "woo=hoo", :set_cookie => "ta=da"} }

it "adds all headers" do
expect(described_class.coerce(headers).to_a).
to match_array(
[
%w[Set-Cookie hoo=ray],
%w[Set-Cookie woo=hoo]
%w[set_cookie woo=hoo],
%w[Set-Cookie ta=da]
]
)
end
Expand Down
2 changes: 1 addition & 1 deletion spec/lib/http/options/headers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
end

it "may be specified with with_headers" do
opts2 = opts.with_headers("accept" => "json")
opts2 = opts.with_headers(:accept => "json")
expect(opts.headers).to be_empty
expect(opts2.headers).to eq([%w[Accept json]])
end
Expand Down
12 changes: 12 additions & 0 deletions spec/lib/http/request/writer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@
end
end

context "when headers are specified as strings with mixed case" do
let(:headers) { HTTP::Headers.coerce "content-Type" => "text", "X_MAX" => "200" }

it "writes the headers with the same casing" do
writer.stream
expect(io.string).to eq [
"#{headerstart}\r\n",
"content-Type: text\r\nX_MAX: 200\r\nContent-Length: 0\r\n\r\n"
].join
end
end

context "when body is nonempty" do
let(:body) { HTTP::Request::Body.new("content") }

Expand Down