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

Replace #status_code with #status #7247

Merged
merged 27 commits into from
Apr 5, 2019
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5c241df
Break out standalone StatusCode class
dwightwatson Dec 29, 2018
74076f8
Rename default_message_for to just message
dwightwatson Dec 30, 2018
1b5c983
Add a code method
dwightwatson Dec 31, 2018
a9254f1
Return nil instead of empty string
dwightwatson Jan 2, 2019
d56b3cf
Add #status_code convenience method to response
dwightwatson Jan 2, 2019
ca20af6
Use free predicate methods
dwightwatson Jan 2, 2019
5d3768a
Don't duplicate initializer
dwightwatson Jan 2, 2019
1f81d64
Add HTTP::Status.from_code
dwightwatson Jan 2, 2019
24e0442
Switch to using HTTP::Status.from_code internally
dwightwatson Jan 2, 2019
78b50de
Additional HTTP::Status specs
dwightwatson Jan 2, 2019
c0dbecd
Revert breaking change to #respond_with_error
dwightwatson Jan 2, 2019
858a7d7
Review changes
dwightwatson Jan 2, 2019
0710db7
Adjust status code constraints
dwightwatson Jan 2, 2019
637a585
Use #code where possible
dwightwatson Jan 2, 2019
cb57c98
Validate status_code as being 3 numbers between 0 and 999
dwightwatson Jan 2, 2019
2991c21
Don’t validate length of the integer
dwightwatson Jan 2, 2019
572bf57
Review changes
dwightwatson Jan 2, 2019
fa35035
Update method description
dwightwatson Jan 2, 2019
9e92830
Use the correct method in the test
dwightwatson Jan 3, 2019
6025a32
Use status symbol where possible
dwightwatson Jan 3, 2019
4ade22a
Update src/http/server/response.cr
RX14 Jan 4, 2019
3a7b199
Inline variable
dwightwatson Jan 4, 2019
d4396fe
Merge branch 'status' of https://github.com/dwightwatson/crystal into…
dwightwatson Jan 4, 2019
3fe71c9
Merge branch 'master' into status
dwightwatson Jan 4, 2019
456d3d1
Replace .from_code with .new
dwightwatson Jan 5, 2019
4acb479
Merge branch 'master' into status
dwightwatson Feb 1, 2019
b693a90
Merge branch 'master' into status
dwightwatson Feb 2, 2019
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
35 changes: 20 additions & 15 deletions spec/std/http/client/response_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -169,14 +169,14 @@ class HTTP::Client
end

it "doesn't sets content length for 1xx, 204 or 304" do
[100, 101, 204, 304].each do |status|
[HTTP::Status::CONTINUE, HTTP::Status::SWITCHING_PROTOCOLS, HTTP::Status::NO_CONTENT, HTTP::Status::NOT_MODIFIED].each do |status|
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved
response = Response.new(status)
response.headers.size.should eq(0)
end
end

it "raises when creating 1xx, 204 or 304 with body" do
[100, 101, 204, 304].each do |status|
[HTTP::Status::CONTINUE, HTTP::Status::SWITCHING_PROTOCOLS, HTTP::Status::NO_CONTENT, HTTP::Status::NOT_MODIFIED].each do |status|
asterite marked this conversation as resolved.
Show resolved Hide resolved
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved
expect_raises ArgumentError do
Response.new(status, "hello")
end
Expand All @@ -188,7 +188,7 @@ class HTTP::Client
headers["Content-Type"] = "text/plain"
headers["Content-Length"] = "5"

response = Response.new(200, "hello", headers)
response = Response.new(:ok, "hello", headers)
io = IO::Memory.new
response.to_io(io)
io.to_s.should eq("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nhello")
Expand All @@ -200,7 +200,7 @@ class HTTP::Client
headers["Content-Length"] = "5"
headers["Set-Cookie"] = "foo=bar; path=/"

response = Response.new(200, "hello", headers)
response = Response.new(:ok, "hello", headers)

io = IO::Memory.new
response.to_io(io)
Expand All @@ -221,69 +221,74 @@ class HTTP::Client
end

it "sets content length from body" do
response = Response.new(200, "hello")
response = Response.new(:ok, "hello")
io = IO::Memory.new
response.to_io(io)
io.to_s.should eq("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\nhello")
end

it "sets content length even without body" do
response = Response.new(200)
response = Response.new(:ok)
io = IO::Memory.new
response.to_io(io)
io.to_s.should eq("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n")
end

it "serialize as chunked with body_io" do
response = Response.new(200, body_io: IO::Memory.new("hello"))
response = Response.new(:ok, body_io: IO::Memory.new("hello"))
io = IO::Memory.new
response.to_io(io)
io.to_s.should eq("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n5\r\nhello\r\n0\r\n\r\n")
end

it "serialize as not chunked with body_io if HTTP/1.0" do
response = Response.new(200, version: "HTTP/1.0", body_io: IO::Memory.new("hello"))
response = Response.new(:ok, version: "HTTP/1.0", body_io: IO::Memory.new("hello"))
io = IO::Memory.new
response.to_io(io)
io.to_s.should eq("HTTP/1.0 200 OK\r\nContent-Length: 5\r\n\r\nhello")
end

it "returns no content_type when header is missing" do
response = Response.new(200, "")
response = Response.new(:ok, "")
response.content_type.should be_nil
response.charset.should be_nil
end

it "returns content type and no charset" do
response = Response.new(200, "", headers: HTTP::Headers{"Content-Type" => "text/plain"})
response = Response.new(:ok, "", headers: HTTP::Headers{"Content-Type" => "text/plain"})
response.content_type.should eq("text/plain")
response.charset.should be_nil
end

it "returns content type and charset, removes semicolon" do
response = Response.new(200, "", headers: HTTP::Headers{"Content-Type" => "text/plain ; charset=UTF-8"})
response = Response.new(:ok, "", headers: HTTP::Headers{"Content-Type" => "text/plain ; charset=UTF-8"})
response.content_type.should eq("text/plain")
response.charset.should eq("UTF-8")
end

it "returns content type and charset, removes quotes" do
response = Response.new(200, "", headers: HTTP::Headers{"Content-Type" => %(text/plain ; charset="UTF-8")})
response = Response.new(:ok, "", headers: HTTP::Headers{"Content-Type" => %(text/plain ; charset="UTF-8")})
response.content_type.should eq("text/plain")
response.charset.should eq("UTF-8")
end

it "returns content type and no charset, other parameter (#2520)" do
response = Response.new(200, "", headers: HTTP::Headers{"Content-Type" => "text/plain ; colenc=U"})
response = Response.new(:ok, "", headers: HTTP::Headers{"Content-Type" => "text/plain ; colenc=U"})
response.content_type.should eq("text/plain")
response.charset.should be_nil
end

it "returns content type and charset, removes semicolon, with multiple parameters (#2520)" do
response = Response.new(200, "", headers: HTTP::Headers{"Content-Type" => "text/plain ; colenc=U ; charset=UTF-8"})
response = Response.new(:ok, "", headers: HTTP::Headers{"Content-Type" => "text/plain ; colenc=U ; charset=UTF-8"})
response.content_type.should eq("text/plain")
response.charset.should eq("UTF-8")
end

it "returns status_code" do
response = Response.new(:created)
response.status_code.should eq 201
end

it "creates Response with status code 204, no body and Content-Length == 0 (#2512)" do
response = Response.new(204, version: "HTTP/1.0", body: "", headers: HTTP::Headers{"Content-Length" => "0"})
response.status_code.should eq(204)
Expand All @@ -292,7 +297,7 @@ class HTTP::Client

describe "success?" do
it "returns true for the 2xx" do
response = Response.new(200)
response = Response.new(:ok)

response.success?.should eq(true)
end
Expand Down
10 changes: 0 additions & 10 deletions spec/std/http/http_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,4 @@ describe HTTP do
end
end
end

describe ".default_status_message_for" do
it "returns a default message for status 200" do
HTTP.default_status_message_for(200).should eq("OK")
end

it "returns an empty string on non-existent status" do
HTTP.default_status_message_for(0).should eq("")
end
end
end
16 changes: 15 additions & 1 deletion spec/std/http/server/server_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,24 @@ module HTTP
io.to_s.should eq("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nHello")
end

it "sets status code" do
io = IO::Memory.new
response = Response.new(io)
response.status_code = 201
response.status.should eq HTTP::Status::CREATED
end

it "retrieves status code" do
io = IO::Memory.new
response = Response.new(io)
response.status = :created
response.status_code.should eq 201
end

it "changes status and others" do
io = IO::Memory.new
response = Response.new(io)
response.status_code = 404
response.status = :not_found
response.version = "HTTP/1.0"
response.close
io.to_s.should eq("HTTP/1.0 404 Not Found\r\nContent-Length: 0\r\n\r\n")
Expand Down
86 changes: 86 additions & 0 deletions spec/std/http/status_spec.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
require "spec"
require "http"

describe HTTP::Status do
describe ".from_code" do
it "raises when given invalid status code" do
expect_raises(ArgumentError, "Invalid HTTP status code: 1000") do
HTTP::Status.from_code(1000)
end
end

it "returns an instance when given defined status code" do
HTTP::Status.from_code(201).should eq HTTP::Status::CREATED
end

it "returns an instance when given undefined status code" do
HTTP::Status.from_code(418).should eq HTTP::Status.new(418)
end
end

describe "#code" do
it "returns the status code" do
HTTP::Status::INTERNAL_SERVER_ERROR.code.should eq 500
end
end

describe "#informational?" do
it "returns true when given 1xx status code" do
HTTP::Status.new(100).informational?.should be_true
end

it "returns false unless given 1xx status code" do
HTTP::Status.new(999).informational?.should be_false
end
end

describe "#success?" do
it "returns true when given 2xx status code" do
HTTP::Status.new(200).success?.should be_true
end

it "returns false unless given 2xx status code" do
HTTP::Status.new(999).success?.should be_false
end
end

describe "#redirection?" do
it "returns true when given 3xx status code" do
HTTP::Status.new(300).redirection?.should be_true
end

it "returns false unless given 3xx status code" do
HTTP::Status.new(999).redirection?.should be_false
end
end

describe "#client_error?" do
it "returns true when given 4xx status code" do
HTTP::Status.new(400).client_error?.should be_true
end

it "returns false unless given 4xx status code" do
HTTP::Status.new(999).client_error?.should be_false
end
end

describe "#server_error?" do
it "returns true when given 5xx status code" do
HTTP::Status.new(500).server_error?.should be_true
end

it "returns false unless given 5xx status code" do
HTTP::Status.new(999).server_error?.should be_false
end
end

describe "#description" do
it "returns default description for status 200" do
HTTP::Status.new(200).description.should eq("OK")
end

it "returns nil on non-existent status" do
HTTP::Status.new(0).description.should eq(nil)
end
end
end
34 changes: 22 additions & 12 deletions src/http/client/response.cr
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,28 @@ require "../common"

class HTTP::Client::Response
getter version : String
getter status_code : Int32
getter status_message : String
getter status : HTTP::Status
getter status_message : String?
getter headers : Headers
getter! body_io : IO
@cookies : Cookies?

def initialize(@status_code, @body : String? = nil, @headers : Headers = Headers.new, status_message = nil, @version = "HTTP/1.1", @body_io = nil)
@status_message = status_message || HTTP.default_status_message_for(@status_code)
def initialize(@status : HTTP::Status, @body : String? = nil, @headers : Headers = Headers.new, status_message = nil, @version = "HTTP/1.1", @body_io = nil)
@status_message = status_message || @status.description

if Response.mandatory_body?(@status_code)
if Response.mandatory_body?(@status)
@body = "" unless @body || @body_io
else
if (@body || @body_io) && (headers["Content-Length"]? != "0")
raise ArgumentError.new("Status #{status_code} should not have a body")
raise ArgumentError.new("Status #{status.code} should not have a body")
end
end
end

def self.new(status_code : Int32, body : String? = nil, headers : Headers = Headers.new, status_message = nil, version = "HTTP/1.1", body_io = nil)
new(HTTP::Status.from_code(status_code), body, headers, status_message, version, body_io)
end

def body
@body || ""
end
Expand All @@ -31,7 +35,7 @@ class HTTP::Client::Response

# Returns `true` if the response status code is between 200 and 299.
def success?
(200..299).includes?(status_code)
@status.success?
end

# Returns a convenience wrapper around querying and setting cookie related
Expand All @@ -48,6 +52,11 @@ class HTTP::Client::Response
process_content_type_header.content_type
end

# Convenience method to retrieve the HTTP status code.
def status_code
status.code
end

def charset
process_content_type_header.charset
end
Expand All @@ -61,7 +70,7 @@ class HTTP::Client::Response
end

def to_io(io)
io << @version << ' ' << @status_code << ' ' << @status_message << "\r\n"
io << @version << ' ' << @status.code << ' ' << @status_message << "\r\n"
cookies = @cookies
headers = cookies ? cookies.add_response_headers(@headers) : @headers
HTTP.serialize_headers_and_body(io, headers, @body, @body_io, @version)
Expand All @@ -75,8 +84,8 @@ class HTTP::Client::Response
end
end

def self.mandatory_body?(status_code) : Bool
!(status_code / 100 == 1 || status_code == 204 || status_code == 304)
def self.mandatory_body?(status : HTTP::Status) : Bool
!(status.informational? || status.no_content? || status.not_modified?)
end

def self.supports_chunked?(version) : Bool
Expand Down Expand Up @@ -131,14 +140,15 @@ class HTTP::Client::Response
raise "Invalid HTTP status code: #{pieces[1]}"
end

status = HTTP::Status.from_code(status_code)
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved
status_message = pieces[2]? ? pieces[2].chomp : ""

body_type = HTTP::BodyType::OnDemand
body_type = HTTP::BodyType::Mandatory if mandatory_body?(status_code)
body_type = HTTP::BodyType::Mandatory if mandatory_body?(status)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could move this method to a body_mandatory? in the HTTP::Status enum (it could be done in a separate PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to tackle that one separately.

body_type = HTTP::BodyType::Prohibited if ignore_body

HTTP.parse_headers_and_body(io, body_type: body_type, decompress: decompress) do |headers, body|
return yield new status_code, nil, headers, status_message, http_version, body
return yield new status, nil, headers, status_message, http_version, body
end
end
end
Loading