From 5c241dfe6bf2781b2379ead1b947e72d61a6e7d7 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Sat, 29 Dec 2018 19:12:14 +1100 Subject: [PATCH 01/23] Break out standalone StatusCode class Update some relevant specs Fix method call Update expectations Convert class to enum Revert to hard-coded messages Actually revert to class method Fix overload implementation Rename StatusCode to Status --- spec/std/http/http_spec.cr | 10 -- spec/std/http/status_spec.cr | 64 +++++++++ src/http/client/response.cr | 4 +- src/http/common.cr | 80 +---------- src/http/server/handler.cr | 2 +- src/http/server/handlers/error_handler.cr | 2 +- src/http/server/response.cr | 6 +- src/http/status.cr | 163 ++++++++++++++++++++++ 8 files changed, 237 insertions(+), 94 deletions(-) create mode 100644 spec/std/http/status_spec.cr create mode 100644 src/http/status.cr diff --git a/spec/std/http/http_spec.cr b/spec/std/http/http_spec.cr index cb0fe186fa41..a490f8e32977 100644 --- a/spec/std/http/http_spec.cr +++ b/spec/std/http/http_spec.cr @@ -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 diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr new file mode 100644 index 000000000000..ebd3646c451b --- /dev/null +++ b/spec/std/http/status_spec.cr @@ -0,0 +1,64 @@ +require "spec" +require "http" + +describe HTTP::Status do + describe ".informational?" do + it "returns true when given 1xx status code" do + HTTP::Status.informational?(100).should be_true + end + + it "returns false unless given 1xx status code" do + HTTP::Status.informational?(999).should be_false + end + end + + describe ".success?" do + it "returns true when given 2xx status code" do + HTTP::Status.success?(200).should be_true + end + + it "returns false unless given 2xx status code" do + HTTP::Status.success?(999).should be_false + end + end + + describe ".redirection?" do + it "returns true when given 3xx status code" do + HTTP::Status.redirection?(300).should be_true + end + + it "returns false unless given 3xx status code" do + HTTP::Status.redirection?(999).should be_false + end + end + + describe ".client_error?" do + it "returns true when given 4xx status code" do + HTTP::Status.client_error?(400).should be_true + end + + it "returns false unless given 4xx status code" do + HTTP::Status.client_error?(999).should be_false + end + end + + describe ".server_error?" do + it "returns true when given 5xx status code" do + HTTP::Status.server_error?(500).should be_true + end + + it "returns false unless given 5xx status code" do + HTTP::Status.server_error?(999).should be_false + end + end + + describe ".default_message_for" do + it "returns a default message for status 200" do + HTTP::Status.default_message_for(200).should eq("OK") + end + + it "returns an empty string on non-existent status" do + HTTP::Status.default_message_for(0).should eq("") + end + end +end diff --git a/src/http/client/response.cr b/src/http/client/response.cr index 2f85b8508674..1600b1ec6165 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -10,7 +10,7 @@ class HTTP::Client::Response @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) + @status_message = status_message || HTTP::Status.default_message_for(@status_code) if Response.mandatory_body?(@status_code) @body = "" unless @body || @body_io @@ -31,7 +31,7 @@ class HTTP::Client::Response # Returns `true` if the response status code is between 200 and 299. def success? - (200..299).includes?(status_code) + HTTP::Status.success?(status_code) end # Returns a convenience wrapper around querying and setting cookie related diff --git a/src/http/common.cr b/src/http/common.cr index bc6689c55592..459efda228b9 100644 --- a/src/http/common.cr +++ b/src/http/common.cr @@ -330,87 +330,9 @@ module HTTP quote_string(string, io) end end - - # Returns the default status message of the given HTTP status code. - # - # Based on [Hypertext Transfer Protocol (HTTP) Status Code Registry](https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml) - # - # Last Updated 2017-04-14 - # - # HTTP Status Codes (source: [http-status-codes-1.csv](https://www.iana.org/assignments/http-status-codes/http-status-codes-1.csv)) - # - # * 1xx: Informational - Request received, continuing process - # * 2xx: Success - The action was successfully received, understood, and accepted - # * 3xx: Redirection - Further action must be taken in order to complete the request - # * 4xx: Client Error - The request contains bad syntax or cannot be fulfilled - # * 5xx: Server Error - The server failed to fulfill an apparently valid request - # - def self.default_status_message_for(status_code : Int) : String - case status_code - when 100 then "Continue" - when 101 then "Switching Protocols" - when 102 then "Processing" - when 200 then "OK" - when 201 then "Created" - when 202 then "Accepted" - when 203 then "Non-Authoritative Information" - when 204 then "No Content" - when 205 then "Reset Content" - when 206 then "Partial Content" - when 207 then "Multi-Status" - when 208 then "Already Reported" - when 226 then "IM Used" - when 300 then "Multiple Choices" - when 301 then "Moved Permanently" - when 302 then "Found" - when 303 then "See Other" - when 304 then "Not Modified" - when 305 then "Use Proxy" - when 307 then "Temporary Redirect" - when 308 then "Permanent Redirect" - when 400 then "Bad Request" - when 401 then "Unauthorized" - when 402 then "Payment Required" - when 403 then "Forbidden" - when 404 then "Not Found" - when 405 then "Method Not Allowed" - when 406 then "Not Acceptable" - when 407 then "Proxy Authentication Required" - when 408 then "Request Timeout" - when 409 then "Conflict" - when 410 then "Gone" - when 411 then "Length Required" - when 412 then "Precondition Failed" - when 413 then "Payload Too Large" - when 414 then "URI Too Long" - when 415 then "Unsupported Media Type" - when 416 then "Range Not Satisfiable" - when 417 then "Expectation Failed" - when 421 then "Misdirected Request" - when 422 then "Unprocessable Entity" - when 423 then "Locked" - when 424 then "Failed Dependency" - when 426 then "Upgrade Required" - when 428 then "Precondition Required" - when 429 then "Too Many Requests" - when 431 then "Request Header Fields Too Large" - when 451 then "Unavailable For Legal Reasons" - when 500 then "Internal Server Error" - when 501 then "Not Implemented" - when 502 then "Bad Gateway" - when 503 then "Service Unavailable" - when 504 then "Gateway Timeout" - when 505 then "HTTP Version Not Supported" - when 506 then "Variant Also Negotiates" - when 507 then "Insufficient Storage" - when 508 then "Loop Detected" - when 510 then "Not Extended" - when 511 then "Network Authentication Required" - else "" - end - end end +require "./status" require "./request" require "./client/response" require "./headers" diff --git a/src/http/server/handler.cr b/src/http/server/handler.cr index 2bac84ac7a9e..3fffe21af143 100644 --- a/src/http/server/handler.cr +++ b/src/http/server/handler.cr @@ -23,7 +23,7 @@ module HTTP::Handler if next_handler = @next next_handler.call(context) else - context.response.status_code = 404 + context.response.status_code = HTTP::Status::NOT_FOUND context.response.headers["Content-Type"] = "text/plain" context.response.puts "Not Found" end diff --git a/src/http/server/handlers/error_handler.cr b/src/http/server/handlers/error_handler.cr index 09e965f4a3b0..7b3327ca895d 100644 --- a/src/http/server/handlers/error_handler.cr +++ b/src/http/server/handlers/error_handler.cr @@ -16,7 +16,7 @@ class HTTP::ErrorHandler rescue ex : Exception if @verbose context.response.reset - context.response.status_code = 500 + context.response.status_code = HTTP::Status::INTERNAL_SERVER_ERROR context.response.content_type = "text/plain" context.response.print("ERROR: ") ex.inspect_with_backtrace(context.response) diff --git a/src/http/server/response.cr b/src/http/server/response.cr index 615bec1dcaf5..2ab63b6f25b1 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -62,6 +62,10 @@ class HTTP::Server headers["Content-Length"] = content_length.to_s end + def status_code=(status : HTTP::Status) + self.status_code = status.value + end + # See `IO#write(slice)`. def write(slice : Bytes) return if slice.empty? @@ -122,7 +126,7 @@ class HTTP::Server end protected def write_headers - status_message = HTTP.default_status_message_for(@status_code) + status_message = HTTP::Status.default_message_for(@status_code) @io << @version << ' ' << @status_code << ' ' << status_message << "\r\n" headers.each do |name, values| values.each do |value| diff --git a/src/http/status.cr b/src/http/status.cr new file mode 100644 index 000000000000..38c5b865b497 --- /dev/null +++ b/src/http/status.cr @@ -0,0 +1,163 @@ +# A support class that provides additional around HTTP status codes. +# +# Based on [Hypertext Transfer Protocol (HTTP) Status Code Registry](https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml) +# +# It provides constants for the defined HTTP status codes as well as helper +# methods to easily identify the type of response. +enum HTTP::Status + CONTINUE = 100 + SWITCHING_PROTOCOLS = 101 + PROCESSING = 102 + EARLY_HINTS = 103 + OK = 200 + CREATED = 201 + ACCEPTED = 202 + NON_AUTHORITATIVE_INFORMATION = 203 + NO_CONTENT = 204 + RESET_CONTENT = 205 + PARTIAL_CONTENT = 206 + MULTI_STATUS = 207 + ALREADY_REPORTED = 208 + IM_USED = 226 + MULTIPLE_CHOICES = 300 + MOVED_PERMANENTLY = 301 + FOUND = 302 + SEE_OTHER = 303 + NOT_MODIFIED = 304 + USE_PROXY = 305 + SWITCH_PROXY = 306 + TEMPORARY_REDIRECT = 307 + PERMANENT_REDIRECT = 308 + BAD_REQUEST = 400 + UNAUTHORIZED = 401 + PAYMENT_REQUIRED = 402 + FORBIDDEN = 403 + NOT_FOUND = 404 + METHOD_NOT_ALLOWED = 405 + NOT_ACCEPTABLE = 406 + PROXY_AUTHENTICATION_REQUIRED = 407 + REQUEST_TIMEOUT = 408 + CONFLICT = 409 + GONE = 410 + LENGTH_REQUIRED = 411 + PRECONDITION_FAILED = 412 + PAYLOAD_TOO_LARGE = 413 + URI_TOO_LONG = 414 + UNSUPPORTED_MEDIA_TYPE = 415 + RANGE_NOT_SATISFIABLE = 416 + EXPECTATION_FAILED = 417 + IM_A_TEAPOT = 418 + MISDIRECTED_REQUEST = 421 + UNPROCESSABLE_ENTITY = 422 + LOCKED = 423 + FAILED_DEPENDENCY = 424 + UPGRADE_REQUIRED = 426 + PRECONDITION_REQUIRED = 428 + TOO_MANY_REQUESTS = 429 + REQUEST_HEADER_FIELDS_TOO_LARGE = 431 + UNAVAILABLE_FOR_LEGAL_REASONS = 451 + INTERNAL_SERVER_ERROR = 500 + NOT_IMPLEMENTED = 501 + BAD_GATEWAY = 502 + SERVICE_UNAVAILABLE = 503 + GATEWAY_TIMEOUT = 504 + HTTP_VERSION_NOT_SUPPORTED = 505 + VARIANT_ALSO_NEGOTIATES = 506 + INSUFFICIENT_STORAGE = 507 + LOOP_DETECTED = 508 + NOT_EXTENDED = 510 + NETWORK_AUTHENTICATION_REQUIRED = 511 + + # Returns `true` if the response status code is between 100 and 199. + def self.informational?(status_code : Int) + 100 <= status_code <= 199 + end + + # Returns `true` if the response status code is between 200 and 299. + def self.success?(status_code : Int) + 200 <= status_code <= 299 + end + + # Returns `true` if the response status code is between 300 and 399. + def self.redirection?(status_code : Int) + 300 <= status_code <= 399 + end + + # Returns `true` if the response status code is between 400 and 499. + def self.client_error?(status_code : Int) + 400 <= status_code <= 499 + end + + # Returns `true` if the response status code is between 500 and 599. + def self.server_error?(status_code : Int) + 500 <= status_code <= 599 + end + + # Returns the default status message of the given HTTP status code. + def self.default_message_for(status_code : Int) : String + case status_code + when 100 then "Continue" + when 101 then "Switching Protocols" + when 102 then "Processing" + when 200 then "OK" + when 201 then "Created" + when 202 then "Accepted" + when 203 then "Non-Authoritative Information" + when 204 then "No Content" + when 205 then "Reset Content" + when 206 then "Partial Content" + when 207 then "Multi-Status" + when 208 then "Already Reported" + when 226 then "IM Used" + when 300 then "Multiple Choices" + when 301 then "Moved Permanently" + when 302 then "Found" + when 303 then "See Other" + when 304 then "Not Modified" + when 305 then "Use Proxy" + when 306 then "Switch Proxy" + when 307 then "Temporary Redirect" + when 308 then "Permanent Redirect" + when 400 then "Bad Request" + when 401 then "Unauthorized" + when 402 then "Payment Required" + when 403 then "Forbidden" + when 404 then "Not Found" + when 405 then "Method Not Allowed" + when 406 then "Not Acceptable" + when 407 then "Proxy Authentication Required" + when 408 then "Request Timeout" + when 409 then "Conflict" + when 410 then "Gone" + when 411 then "Length Required" + when 412 then "Precondition Failed" + when 413 then "Payload Too Large" + when 414 then "URI Too Long" + when 415 then "Unsupported Media Type" + when 416 then "Range Not Satisfiable" + when 417 then "Expectation Failed" + when 418 then "I'm a teapot" + when 421 then "Misdirected Request" + when 422 then "Unprocessable Entity" + when 423 then "Locked" + when 424 then "Failed Dependency" + when 426 then "Upgrade Required" + when 428 then "Precondition Required" + when 429 then "Too Many Requests" + when 431 then "Request Header Fields Too Large" + when 451 then "Unavailable For Legal Reasons" + when 500 then "Internal Server Error" + when 501 then "Not Implemented" + when 502 then "Bad Gateway" + when 503 then "Service Unavailable" + when 504 then "Gateway Timeout" + when 505 then "HTTP Version Not Supported" + when 506 then "Variant Also Negotiates" + when 507 then "Insufficient Storage" + when 508 then "Loop Detected" + when 510 then "Not Extended" + when 511 then "Network Authentication Required" + else "" + end + end +end From 74076f802c5fc616f0c083ccd5d7372963c75a9c Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Sun, 30 Dec 2018 11:34:02 +1100 Subject: [PATCH 02/23] Rename default_message_for to just message Flip methdos back to instance methods Actually flip methods Update more uses Rename #message to #description --- spec/std/http/status_spec.cr | 40 +++++++++---------- src/http/client/response.cr | 4 +- src/http/formdata.cr | 2 +- .../server/handlers/static_file_handler.cr | 8 ++-- src/http/server/handlers/websocket_handler.cr | 6 +-- src/http/server/response.cr | 2 +- src/http/status.cr | 26 ++++++------ 7 files changed, 44 insertions(+), 44 deletions(-) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index ebd3646c451b..29bb314639de 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -2,63 +2,63 @@ require "spec" require "http" describe HTTP::Status do - describe ".informational?" do + describe "#informational?" do it "returns true when given 1xx status code" do - HTTP::Status.informational?(100).should be_true + HTTP::Status.new(100).informational?.should be_true end it "returns false unless given 1xx status code" do - HTTP::Status.informational?(999).should be_false + HTTP::Status.new(999).informational?.should be_false end end - describe ".success?" do + describe "#success?" do it "returns true when given 2xx status code" do - HTTP::Status.success?(200).should be_true + HTTP::Status.new(200).success?.should be_true end it "returns false unless given 2xx status code" do - HTTP::Status.success?(999).should be_false + HTTP::Status.new(999).success?.should be_false end end - describe ".redirection?" do + describe "#redirection?" do it "returns true when given 3xx status code" do - HTTP::Status.redirection?(300).should be_true + HTTP::Status.new(300).redirection?.should be_true end it "returns false unless given 3xx status code" do - HTTP::Status.redirection?(999).should be_false + HTTP::Status.new(999).redirection?.should be_false end end - describe ".client_error?" do + describe "#client_error?" do it "returns true when given 4xx status code" do - HTTP::Status.client_error?(400).should be_true + HTTP::Status.new(400).client_error?.should be_true end it "returns false unless given 4xx status code" do - HTTP::Status.client_error?(999).should be_false + HTTP::Status.new(999).client_error?.should be_false end end - describe ".server_error?" do + describe "#server_error?" do it "returns true when given 5xx status code" do - HTTP::Status.server_error?(500).should be_true + HTTP::Status.new(500).server_error?.should be_true end it "returns false unless given 5xx status code" do - HTTP::Status.server_error?(999).should be_false + HTTP::Status.new(999).server_error?.should be_false end end - describe ".default_message_for" do - it "returns a default message for status 200" do - HTTP::Status.default_message_for(200).should eq("OK") + describe "#description" do + it "returns default description for status 200" do + HTTP::Status.new(200).description.should eq("OK") end - it "returns an empty string on non-existent status" do - HTTP::Status.default_message_for(0).should eq("") + it "returns empty string on non-existent status" do + HTTP::Status.new(0).description.should eq("") end end end diff --git a/src/http/client/response.cr b/src/http/client/response.cr index 1600b1ec6165..878e9f2b368f 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -10,7 +10,7 @@ class HTTP::Client::Response @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::Status.default_message_for(@status_code) + @status_message = status_message || HTTP::Status.new(@status_code).description if Response.mandatory_body?(@status_code) @body = "" unless @body || @body_io @@ -31,7 +31,7 @@ class HTTP::Client::Response # Returns `true` if the response status code is between 200 and 299. def success? - HTTP::Status.success?(status_code) + HTTP::Status.new(status_code).success? end # Returns a convenience wrapper around querying and setting cookie related diff --git a/src/http/formdata.cr b/src/http/formdata.cr index f7d63526f836..636cd3c5d156 100644 --- a/src/http/formdata.cr +++ b/src/http/formdata.cr @@ -27,7 +27,7 @@ require "./formdata/**" # end # # unless name && file -# context.response.status_code = 400 +# context.response.status_code = HTTP::Status::BAD_REQUEST # next # end # diff --git a/src/http/server/handlers/static_file_handler.cr b/src/http/server/handlers/static_file_handler.cr index 5e2d3d78fae4..30f02b0ccfb5 100644 --- a/src/http/server/handlers/static_file_handler.cr +++ b/src/http/server/handlers/static_file_handler.cr @@ -29,7 +29,7 @@ class HTTP::StaticFileHandler if @fallthrough call_next(context) else - context.response.status_code = 405 + context.response.status_code = HTTP::Status::METHOD_NOT_ALLOWED context.response.headers.add("Allow", "GET, HEAD") end return @@ -42,7 +42,7 @@ class HTTP::StaticFileHandler # File path cannot contains '\0' (NUL) because all filesystem I know # don't accept '\0' character as file name. if request_path.includes? '\0' - context.response.status_code = 400 + context.response.status_code = HTTP::Status::BAD_REQUEST return end @@ -69,7 +69,7 @@ class HTTP::StaticFileHandler add_cache_headers(context.response.headers, last_modified) if cache_request?(context, last_modified) - context.response.status_code = 304 + context.response.status_code = HTTP::Status::NOT_MODIFIED return end @@ -90,7 +90,7 @@ class HTTP::StaticFileHandler end private def redirect_to(context, url) - context.response.status_code = 302 + context.response.status_code = HTTP::Status::FOUND url = URI.escape(url) { |byte| URI.unreserved?(byte) || byte.chr == '/' } context.response.headers.add "Location", url diff --git a/src/http/server/handlers/websocket_handler.cr b/src/http/server/handlers/websocket_handler.cr index 0e25ddd6a516..c22120b964fb 100644 --- a/src/http/server/handlers/websocket_handler.cr +++ b/src/http/server/handlers/websocket_handler.cr @@ -19,7 +19,7 @@ class HTTP::WebSocketHandler version = context.request.headers["Sec-WebSocket-Version"]? unless version == WebSocket::Protocol::VERSION - response.status_code = 426 + response.status_code = HTTP::Status::UPGRADE_REQUIRED response.headers["Sec-WebSocket-Version"] = WebSocket::Protocol::VERSION return end @@ -27,13 +27,13 @@ class HTTP::WebSocketHandler key = context.request.headers["Sec-WebSocket-Key"]? unless key - response.status_code = 400 + response.status_code = HTTP::Status::BAD_REQUEST return end accept_code = WebSocket::Protocol.key_challenge(key) - response.status_code = 101 + response.status_code = HTTP::Status::SWITCHING_PROTOCOLS response.headers["Upgrade"] = "websocket" response.headers["Connection"] = "Upgrade" response.headers["Sec-WebSocket-Accept"] = accept_code diff --git a/src/http/server/response.cr b/src/http/server/response.cr index 2ab63b6f25b1..334beb79913d 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -126,7 +126,7 @@ class HTTP::Server end protected def write_headers - status_message = HTTP::Status.default_message_for(@status_code) + status_message = HTTP::Status.new(@status_code).description @io << @version << ' ' << @status_code << ' ' << status_message << "\r\n" headers.each do |name, values| values.each do |value| diff --git a/src/http/status.cr b/src/http/status.cr index 38c5b865b497..3ff77d4f0c68 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -69,33 +69,33 @@ enum HTTP::Status NETWORK_AUTHENTICATION_REQUIRED = 511 # Returns `true` if the response status code is between 100 and 199. - def self.informational?(status_code : Int) - 100 <= status_code <= 199 + def informational? + 100 <= value <= 199 end # Returns `true` if the response status code is between 200 and 299. - def self.success?(status_code : Int) - 200 <= status_code <= 299 + def success? + 200 <= value <= 299 end # Returns `true` if the response status code is between 300 and 399. - def self.redirection?(status_code : Int) - 300 <= status_code <= 399 + def redirection? + 300 <= value <= 399 end # Returns `true` if the response status code is between 400 and 499. - def self.client_error?(status_code : Int) - 400 <= status_code <= 499 + def client_error? + 400 <= value <= 499 end # Returns `true` if the response status code is between 500 and 599. - def self.server_error?(status_code : Int) - 500 <= status_code <= 599 + def server_error? + 500 <= value <= 599 end - # Returns the default status message of the given HTTP status code. - def self.default_message_for(status_code : Int) : String - case status_code + # Returns the default status description of the given HTTP status code. + def description : String + case value when 100 then "Continue" when 101 then "Switching Protocols" when 102 then "Processing" From 1b5c983a7db275b6bec751cc671d80736d22e061 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Mon, 31 Dec 2018 12:56:10 +1100 Subject: [PATCH 03/23] Add a code method --- spec/std/http/status_spec.cr | 6 ++++++ src/http/status.cr | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index 29bb314639de..48c79b5c6c43 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -2,6 +2,12 @@ require "spec" require "http" describe HTTP::Status do + describe "#code" do + it "returns the status code" do + HTTP::Status::INTERNAL_SERVER_ERROR.value.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 diff --git a/src/http/status.cr b/src/http/status.cr index 3ff77d4f0c68..094abf37433d 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -68,6 +68,10 @@ enum HTTP::Status NOT_EXTENDED = 510 NETWORK_AUTHENTICATION_REQUIRED = 511 + def code + value + end + # Returns `true` if the response status code is between 100 and 199. def informational? 100 <= value <= 199 From a9254f19be5ccb852eb8b431c7b04c4a2403e285 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Wed, 2 Jan 2019 11:14:37 +1100 Subject: [PATCH 04/23] Return nil instead of empty string Update file comment Co-Authored-By: dwightwatson Replace #status_code with #status Update OAuth2::Client --- spec/std/http/client/response_spec.cr | 50 ++++++++--------- .../server/handlers/error_handler_spec.cr | 2 +- spec/std/http/server/handlers/handler_spec.cr | 2 +- .../handlers/static_file_handler_spec.cr | 56 +++++++++---------- spec/std/http/server/server_spec.cr | 12 ++-- spec/std/http/status_spec.cr | 4 +- src/http/client/response.cr | 38 +++++++++---- src/http/formdata.cr | 2 +- src/http/server/handler.cr | 2 +- src/http/server/handlers/error_handler.cr | 2 +- src/http/server/handlers/log_handler.cr | 2 +- .../server/handlers/static_file_handler.cr | 8 +-- src/http/server/handlers/websocket_handler.cr | 6 +- src/http/server/request_processor.cr | 2 +- src/http/server/response.cr | 23 ++++---- src/http/status.cr | 6 +- src/http/web_socket/protocol.cr | 4 +- src/oauth2/client.cr | 4 +- 18 files changed, 120 insertions(+), 105 deletions(-) diff --git a/spec/std/http/client/response_spec.cr b/spec/std/http/client/response_spec.cr index 83b50652234e..6726cad601bc 100644 --- a/spec/std/http/client/response_spec.cr +++ b/spec/std/http/client/response_spec.cr @@ -6,7 +6,7 @@ class HTTP::Client it "parses response with body" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nhelloworld")) response.version.should eq("HTTP/1.1") - response.status_code.should eq(200) + response.status.code.should eq(200) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("5") @@ -16,7 +16,7 @@ class HTTP::Client it "parses response with streamed body" do Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nhelloworld")) do |response| response.version.should eq("HTTP/1.1") - response.status_code.should eq(200) + response.status.code.should eq(200) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("5") @@ -34,7 +34,7 @@ class HTTP::Client it "parses response with body without \\r" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\nContent-Type: text/plain\nContent-Length: 5\n\nhelloworld")) response.version.should eq("HTTP/1.1") - response.status_code.should eq(200) + response.status.code.should eq(200) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("5") @@ -43,7 +43,7 @@ class HTTP::Client it "parses response with body but without content-length" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\r\n\r\nhelloworld")) - response.status_code.should eq(200) + response.status.code.should eq(200) response.status_message.should eq("OK") response.headers.size.should eq(0) response.body.should eq("helloworld") @@ -51,7 +51,7 @@ class HTTP::Client it "parses response with empty body but without content-length" do response = Response.from_io(IO::Memory.new("HTTP/1.1 404 Not Found\r\n\r\n")) - response.status_code.should eq(404) + response.status.code.should eq(404) response.status_message.should eq("Not Found") response.headers.size.should eq(0) response.body.should eq("") @@ -59,7 +59,7 @@ class HTTP::Client it "parses response without body" do response = Response.from_io(IO::Memory.new("HTTP/1.1 100 Continue\r\n\r\n")) - response.status_code.should eq(100) + response.status.code.should eq(100) response.status_message.should eq("Continue") response.headers.size.should eq(0) response.body?.should be_nil @@ -67,7 +67,7 @@ class HTTP::Client it "parses response without status message" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200\r\n\r\n")) - response.status_code.should eq(200) + response.status.code.should eq(200) response.status_message.should eq("") response.headers.size.should eq(0) response.body.should eq("") @@ -106,7 +106,7 @@ class HTTP::Client it "parses response ignoring body" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nhelloworld"), true) response.version.should eq("HTTP/1.1") - response.status_code.should eq(200) + response.status.code.should eq(200) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("5") @@ -116,7 +116,7 @@ class HTTP::Client it "parses 204 response without body but Content-Length == 0 (#2512)" do response = Response.from_io(IO::Memory.new("HTTP/1.1 204 OK\r\nContent-Type: text/plain\r\nContent-Length: 0\r\n\r\n")) response.version.should eq("HTTP/1.1") - response.status_code.should eq(204) + response.status.code.should eq(204) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("0") @@ -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| 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| expect_raises ArgumentError do Response.new(status, "hello") end @@ -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(HTTP::Status::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") @@ -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(HTTP::Status::OK, "hello", headers) io = IO::Memory.new response.to_io(io) @@ -221,78 +221,78 @@ class HTTP::Client end it "sets content length from body" do - response = Response.new(200, "hello") + response = Response.new(HTTP::Status::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(HTTP::Status::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(HTTP::Status::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(HTTP::Status::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(HTTP::Status::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(HTTP::Status::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(HTTP::Status::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(HTTP::Status::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(HTTP::Status::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(HTTP::Status::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 "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) + response.status.code.should eq(204) response.body.should eq("") end describe "success?" do it "returns true for the 2xx" do - response = Response.new(200) + response = Response.new(HTTP::Status::OK) response.success?.should eq(true) end diff --git a/spec/std/http/server/handlers/error_handler_spec.cr b/spec/std/http/server/handlers/error_handler_spec.cr index d0f1540d7b07..ac0b86503fd2 100644 --- a/spec/std/http/server/handlers/error_handler_spec.cr +++ b/spec/std/http/server/handlers/error_handler_spec.cr @@ -16,7 +16,7 @@ describe HTTP::ErrorHandler do io.rewind response2 = HTTP::Client::Response.from_io(io) - response2.status_code.should eq(500) + response2.status.code.should eq(500) response2.status_message.should eq("Internal Server Error") (response2.body =~ /ERROR: OH NO!/).should be_truthy end diff --git a/spec/std/http/server/handlers/handler_spec.cr b/spec/std/http/server/handlers/handler_spec.cr index b38337f03c9b..f98c773c230b 100644 --- a/spec/std/http/server/handlers/handler_spec.cr +++ b/spec/std/http/server/handlers/handler_spec.cr @@ -22,7 +22,7 @@ describe HTTP::Handler do io.rewind response = HTTP::Client::Response.from_io(io) - response.status_code.should eq(404) + response.status.code.should eq(404) response.body.should eq("Not Found\n") end end diff --git a/spec/std/http/server/handlers/static_file_handler_spec.cr b/spec/std/http/server/handlers/static_file_handler_spec.cr index b9c5a4c5b9a2..c2f0d23afed2 100644 --- a/spec/std/http/server/handlers/static_file_handler_spec.cr +++ b/spec/std/http/server/handlers/static_file_handler_spec.cr @@ -17,7 +17,7 @@ describe HTTP::StaticFileHandler do it "serves a file" do response = handle HTTP::Request.new("GET", "/test.txt"), ignore_body: false - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end @@ -39,7 +39,7 @@ describe HTTP::StaticFileHandler do headers["If-Modified-Since"] = initial_response.headers["Last-Modified"] response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status_code.should eq(304) + response.status.code.should eq(304) response.headers["Last-Modified"].should eq initial_response.headers["Last-Modified"] response.headers["Content-Type"]?.should be_nil @@ -50,7 +50,7 @@ describe HTTP::StaticFileHandler do headers["If-Modified-Since"] = HTTP.format_time(File.info(datapath("static_file_handler", "test.txt")).modification_time + 1.hour) response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status_code.should eq(304) + response.status.code.should eq(304) end it "serves file if file mtime is younger" do @@ -58,7 +58,7 @@ describe HTTP::StaticFileHandler do headers["If-Modified-Since"] = HTTP.format_time(File.info(datapath("static_file_handler", "test.txt")).modification_time - 1.hour) response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end end @@ -70,7 +70,7 @@ describe HTTP::StaticFileHandler do headers = HTTP::Headers.new headers["If-None-Match"] = initial_response.headers["Etag"] response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status_code.should eq(304) + response.status.code.should eq(304) end it "serves file if header does not match etag" do @@ -78,7 +78,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = "some random etag" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end @@ -86,7 +86,7 @@ describe HTTP::StaticFileHandler do headers = HTTP::Headers.new headers["If-None-Match"] = "*" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status_code.should eq(304) + response.status.code.should eq(304) end it "serves file if header is empty" do @@ -94,7 +94,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = "" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end @@ -103,7 +103,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = ", foo" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end end @@ -115,7 +115,7 @@ describe HTTP::StaticFileHandler do headers = HTTP::Headers.new headers["If-None-Match"] = %(,, ,W/"1234567" , , #{initial_response.headers["Etag"]},"12345678",%) response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status_code.should eq(304) + response.status.code.should eq(304) end it "serves file if no header matches etag" do @@ -123,7 +123,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = "some random etag, 1234567" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end end @@ -137,7 +137,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = initial_response.headers["Etag"] response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status_code.should eq(304) + response.status.code.should eq(304) end it "serves a file if header does not match etag even If-Modified-Since is fresh" do @@ -148,48 +148,48 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = "some random etag" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end end it "lists directory's entries" do response = handle HTTP::Request.new("GET", "/") - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should match(/test.txt/) end it "does not list directory's entries when directory_listing is set to false" do response = handle HTTP::Request.new("GET", "/"), directory_listing: false - response.status_code.should eq(404) + response.status.code.should eq(404) end it "does not serve a not found file" do response = handle HTTP::Request.new("GET", "/not_found_file.txt") - response.status_code.should eq(404) + response.status.code.should eq(404) end it "does not serve a not found directory" do response = handle HTTP::Request.new("GET", "/not_found_dir/") - response.status_code.should eq(404) + response.status.code.should eq(404) end it "does not serve a file as directory" do response = handle HTTP::Request.new("GET", "/test.txt/") - response.status_code.should eq(404) + response.status.code.should eq(404) end it "handles only GET and HEAD method" do %w(GET HEAD).each do |method| response = handle HTTP::Request.new(method, "/test.txt") - response.status_code.should eq(200) + response.status.code.should eq(200) end %w(POST PUT DELETE).each do |method| response = handle HTTP::Request.new(method, "/test.txt") - response.status_code.should eq(404) + response.status.code.should eq(404) response = handle HTTP::Request.new(method, "/test.txt"), false - response.status_code.should eq(405) + response.status.code.should eq(405) response.headers["Allow"].should eq("GET, HEAD") end end @@ -197,14 +197,14 @@ describe HTTP::StaticFileHandler do it "expands a request path" do %w(../test.txt ../../test.txt test.txt/../test.txt a/./b/../c/../../test.txt).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status_code.should eq(302) + response.status.code.should eq(302) response.headers["Location"].should eq("/test.txt") end # directory %w(.. ../ ../.. a/.. a/.././b/../).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status_code.should eq(302) + response.status.code.should eq(302) response.headers["Location"].should eq("/") end end @@ -212,13 +212,13 @@ describe HTTP::StaticFileHandler do it "unescapes a request path" do %w(test%2Etxt %74%65%73%74%2E%74%78%74).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq(file_text) end %w(%2E%2E/test.txt found%2F%2E%2E%2Ftest%2Etxt).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status_code.should eq(302) + response.status.code.should eq(302) response.headers["Location"].should eq("/test.txt") end end @@ -226,16 +226,16 @@ describe HTTP::StaticFileHandler do it "returns 400" do %w(%00 test.txt%00).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status_code.should eq(400) + response.status.code.should eq(400) end end it "handles invalid redirect path" do response = handle HTTP::Request.new("GET", "test.txt%0A") - response.status_code.should eq(302) + response.status.code.should eq(302) response.headers["Location"].should eq "/test.txt%0A" response = handle HTTP::Request.new("GET", "/test.txt%0A") - response.status_code.should eq(404) + response.status.code.should eq(404) end end diff --git a/spec/std/http/server/server_spec.cr b/spec/std/http/server/server_spec.cr index 296b2a200343..05dad5b556a9 100644 --- a/spec/std/http/server/server_spec.cr +++ b/spec/std/http/server/server_spec.cr @@ -158,7 +158,7 @@ module HTTP it "changes status and others" do io = IO::Memory.new response = Response.new(io) - response.status_code = 404 + response.status = HTTP::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") @@ -225,7 +225,7 @@ module HTTP io = IO::Memory.new response = Response.new(io) - response.respond_with_error("Bad Request", 400) + response.respond_with_error(HTTP::Status::BAD_REQUEST) io.to_s.should eq("HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\nTransfer-Encoding: chunked\r\n\r\n10\r\n400 Bad Request\n\r\n") end end @@ -307,20 +307,20 @@ module HTTP socket.flush response = Client::Response.from_io(socket) - response.status_code.should eq(100) + response.status.code.should eq(100) socket << "hello" socket.flush response = Client::Response.from_io(socket) - response.status_code.should eq(200) + response.status.code.should eq(200) response.body.should eq("hello") end end it "handles Expect: 100-continue correctly when body isn't read" do server = Server.new do |context| - context.response.respond_with_error("I don't want your body", 400) + context.response.respond_with_error(HTTP::Status.new(400), "I don't want your body") end address = server.bind_unused_port @@ -340,7 +340,7 @@ module HTTP socket.flush response = Client::Response.from_io(socket) - response.status_code.should eq(400) + response.status.code.should eq(400) response.body.should eq("400 I don't want your body\n") end end diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index 48c79b5c6c43..c62a018f0803 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -63,8 +63,8 @@ describe HTTP::Status do HTTP::Status.new(200).description.should eq("OK") end - it "returns empty string on non-existent status" do - HTTP::Status.new(0).description.should eq("") + it "returns nil on non-existent status" do + HTTP::Status.new(0).description.should eq(nil) end end end diff --git a/src/http/client/response.cr b/src/http/client/response.cr index 878e9f2b368f..c7cab000e915 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -3,20 +3,33 @@ 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::Status.new(@status_code).description + 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 initialize(status_code : Int32, @body : String? = nil, @headers : Headers = Headers.new, status_message = nil, @version = "HTTP/1.1", @body_io = nil) + @status = HTTP::Status.new(status_code) + @status_message = status_message || @status.description + + 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") end end end @@ -31,7 +44,7 @@ class HTTP::Client::Response # Returns `true` if the response status code is between 200 and 299. def success? - HTTP::Status.new(status_code).success? + @status.success? end # Returns a convenience wrapper around querying and setting cookie related @@ -61,7 +74,7 @@ class HTTP::Client::Response end def to_io(io) - io << @version << ' ' << @status_code << ' ' << @status_message << "\r\n" + io << @version << ' ' << @status.value << ' ' << @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) @@ -75,8 +88,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 == HTTP::Status::NO_CONTENT || status == HTTP::Status::NOT_MODIFIED) end def self.supports_chunked?(version) : Bool @@ -131,14 +144,15 @@ class HTTP::Client::Response raise "Invalid HTTP status code: #{pieces[1]}" end + status = HTTP::Status.new(status_code) 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) 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 diff --git a/src/http/formdata.cr b/src/http/formdata.cr index 636cd3c5d156..33e30018d130 100644 --- a/src/http/formdata.cr +++ b/src/http/formdata.cr @@ -27,7 +27,7 @@ require "./formdata/**" # end # # unless name && file -# context.response.status_code = HTTP::Status::BAD_REQUEST +# context.response.status = HTTP::Status::BAD_REQUEST # next # end # diff --git a/src/http/server/handler.cr b/src/http/server/handler.cr index 3fffe21af143..8099be03da49 100644 --- a/src/http/server/handler.cr +++ b/src/http/server/handler.cr @@ -23,7 +23,7 @@ module HTTP::Handler if next_handler = @next next_handler.call(context) else - context.response.status_code = HTTP::Status::NOT_FOUND + context.response.status = HTTP::Status::NOT_FOUND context.response.headers["Content-Type"] = "text/plain" context.response.puts "Not Found" end diff --git a/src/http/server/handlers/error_handler.cr b/src/http/server/handlers/error_handler.cr index 7b3327ca895d..67563cbf5444 100644 --- a/src/http/server/handlers/error_handler.cr +++ b/src/http/server/handlers/error_handler.cr @@ -16,7 +16,7 @@ class HTTP::ErrorHandler rescue ex : Exception if @verbose context.response.reset - context.response.status_code = HTTP::Status::INTERNAL_SERVER_ERROR + context.response.status = HTTP::Status::INTERNAL_SERVER_ERROR context.response.content_type = "text/plain" context.response.print("ERROR: ") ex.inspect_with_backtrace(context.response) diff --git a/src/http/server/handlers/log_handler.cr b/src/http/server/handlers/log_handler.cr index 7e76be855f42..ad2fe9a10dd1 100644 --- a/src/http/server/handlers/log_handler.cr +++ b/src/http/server/handlers/log_handler.cr @@ -11,7 +11,7 @@ class HTTP::LogHandler elapsed = Time.measure { call_next(context) } elapsed_text = elapsed_text(elapsed) - @io.puts "#{context.request.method} #{context.request.resource} - #{context.response.status_code} (#{elapsed_text})" + @io.puts "#{context.request.method} #{context.request.resource} - #{context.response.status.code} (#{elapsed_text})" rescue e @io.puts "#{context.request.method} #{context.request.resource} - Unhandled exception:" e.inspect_with_backtrace(@io) diff --git a/src/http/server/handlers/static_file_handler.cr b/src/http/server/handlers/static_file_handler.cr index 30f02b0ccfb5..fde37f9b8f40 100644 --- a/src/http/server/handlers/static_file_handler.cr +++ b/src/http/server/handlers/static_file_handler.cr @@ -29,7 +29,7 @@ class HTTP::StaticFileHandler if @fallthrough call_next(context) else - context.response.status_code = HTTP::Status::METHOD_NOT_ALLOWED + context.response.status = HTTP::Status::METHOD_NOT_ALLOWED context.response.headers.add("Allow", "GET, HEAD") end return @@ -42,7 +42,7 @@ class HTTP::StaticFileHandler # File path cannot contains '\0' (NUL) because all filesystem I know # don't accept '\0' character as file name. if request_path.includes? '\0' - context.response.status_code = HTTP::Status::BAD_REQUEST + context.response.status = HTTP::Status::BAD_REQUEST return end @@ -69,7 +69,7 @@ class HTTP::StaticFileHandler add_cache_headers(context.response.headers, last_modified) if cache_request?(context, last_modified) - context.response.status_code = HTTP::Status::NOT_MODIFIED + context.response.status = HTTP::Status::NOT_MODIFIED return end @@ -90,7 +90,7 @@ class HTTP::StaticFileHandler end private def redirect_to(context, url) - context.response.status_code = HTTP::Status::FOUND + context.response.status = HTTP::Status::FOUND url = URI.escape(url) { |byte| URI.unreserved?(byte) || byte.chr == '/' } context.response.headers.add "Location", url diff --git a/src/http/server/handlers/websocket_handler.cr b/src/http/server/handlers/websocket_handler.cr index c22120b964fb..6030ca2b51fc 100644 --- a/src/http/server/handlers/websocket_handler.cr +++ b/src/http/server/handlers/websocket_handler.cr @@ -19,7 +19,7 @@ class HTTP::WebSocketHandler version = context.request.headers["Sec-WebSocket-Version"]? unless version == WebSocket::Protocol::VERSION - response.status_code = HTTP::Status::UPGRADE_REQUIRED + response.status = HTTP::Status::UPGRADE_REQUIRED response.headers["Sec-WebSocket-Version"] = WebSocket::Protocol::VERSION return end @@ -27,13 +27,13 @@ class HTTP::WebSocketHandler key = context.request.headers["Sec-WebSocket-Key"]? unless key - response.status_code = HTTP::Status::BAD_REQUEST + response.status = HTTP::Status::BAD_REQUEST return end accept_code = WebSocket::Protocol.key_challenge(key) - response.status_code = HTTP::Status::SWITCHING_PROTOCOLS + response.status = HTTP::Status::SWITCHING_PROTOCOLS response.headers["Upgrade"] = "websocket" response.headers["Connection"] = "Upgrade" response.headers["Sec-WebSocket-Accept"] = accept_code diff --git a/src/http/server/request_processor.cr b/src/http/server/request_processor.cr index 5d6d90691991..f8e17e79d26d 100644 --- a/src/http/server/request_processor.cr +++ b/src/http/server/request_processor.cr @@ -25,7 +25,7 @@ class HTTP::Server::RequestProcessor break unless request if request.is_a?(HTTP::Request::BadRequest) - response.respond_with_error("Bad Request", 400) + response.respond_with_error(HTTP::Status::BAD_REQUEST) response.close return end diff --git a/src/http/server/response.cr b/src/http/server/response.cr index 334beb79913d..bd469a93b145 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -1,7 +1,7 @@ class HTTP::Server # The response to configure and write to in an `HTTP::Server` handler. # - # The response `status_code` and `headers` must be configured before writing + # The response `status` and `headers` must be configured before writing # the response body. Once response output is written, changing the `status` # and `headers` properties has no effect. # @@ -28,12 +28,12 @@ class HTTP::Server # The status code of this response, which must be set before writing the response # body. If not set, the default value is 200 (OK). - property status_code : Int32 + property status : HTTP::Status # :nodoc: def initialize(@io : IO, @version = "HTTP/1.1") @headers = Headers.new - @status_code = 200 + @status = HTTP::Status::OK @wrote_headers = false @upgraded = false @output = output = @original_output = Output.new(@io) @@ -45,7 +45,7 @@ class HTTP::Server # This method is called by RequestProcessor to avoid allocating a new instance for each iteration. @headers.clear @cookies = nil - @status_code = 200 + @status = HTTP::Status::OK @wrote_headers = false @upgraded = false @output = @original_output @@ -62,8 +62,8 @@ class HTTP::Server headers["Content-Length"] = content_length.to_s end - def status_code=(status : HTTP::Status) - self.status_code = status.value + def status_code=(status_code : Int32) + self.status = HTTP::Status.new(status_code) end # See `IO#write(slice)`. @@ -117,17 +117,18 @@ class HTTP::Server # Generates an error response using *message* and *code*. # # Calls `reset` and then writes the given message. - def respond_with_error(message = "Internal Server Error", code = 500) + def respond_with_error(status : HTTP::Status = HTTP::Status::INTERNAL_SERVER_ERROR, status_message = nil) reset - @status_code = code + @status = status + message = status_message || @status.description self.content_type = "text/plain" - self << code << ' ' << message << '\n' + self << @status.code << ' ' << message << '\n' flush end protected def write_headers - status_message = HTTP::Status.new(@status_code).description - @io << @version << ' ' << @status_code << ' ' << status_message << "\r\n" + status_message = @status.description + @io << @version << ' ' << @status.code << ' ' << status_message << "\r\n" headers.each do |name, values| values.each do |value| @io << name << ": " << value << "\r\n" diff --git a/src/http/status.cr b/src/http/status.cr index 094abf37433d..0e6a52c20815 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -1,4 +1,4 @@ -# A support class that provides additional around HTTP status codes. +# A support enum that provides additional around HTTP status codes. # # Based on [Hypertext Transfer Protocol (HTTP) Status Code Registry](https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml) # @@ -98,7 +98,7 @@ enum HTTP::Status end # Returns the default status description of the given HTTP status code. - def description : String + def description : String? case value when 100 then "Continue" when 101 then "Switching Protocols" @@ -161,7 +161,7 @@ enum HTTP::Status when 508 then "Loop Detected" when 510 then "Not Extended" when 511 then "Network Authentication Required" - else "" + else nil end end end diff --git a/src/http/web_socket/protocol.cr b/src/http/web_socket/protocol.cr index 798bc24f95ec..cb8b6077e216 100644 --- a/src/http/web_socket/protocol.cr +++ b/src/http/web_socket/protocol.cr @@ -272,8 +272,8 @@ class HTTP::WebSocket::Protocol handshake.to_io(socket) socket.flush handshake_response = HTTP::Client::Response.from_io(socket) - unless handshake_response.status_code == 101 - raise Socket::Error.new("Handshake got denied. Status code was #{handshake_response.status_code}.") + unless handshake_response.status == HTTP::Status::SWITCHING_PROTOCOLS + raise Socket::Error.new("Handshake got denied. Status code was #{handshake_response.status.code}.") end challenge_response = Protocol.key_challenge(random_key) diff --git a/src/oauth2/client.cr b/src/oauth2/client.cr index 0380a6eaa088..d90d4dcdf640 100644 --- a/src/oauth2/client.cr +++ b/src/oauth2/client.cr @@ -145,8 +145,8 @@ class OAuth2::Client } response = HTTP::Client.post(token_uri, form: body, headers: headers) - case response.status_code - when 200, 201 + case response.status + when HTTP::Status::OK, HTTP::Status::CREATED OAuth2::AccessToken.from_json(response.body) else raise OAuth2::Error.from_json(response.body) From d56b3cfa690a80482d295c630dd96e05dfa4d167 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Wed, 2 Jan 2019 12:35:43 +1100 Subject: [PATCH 05/23] Add #status_code convenience method to response --- spec/std/http/client/response_spec.cr | 25 +++++---- .../server/handlers/error_handler_spec.cr | 2 +- spec/std/http/server/handlers/handler_spec.cr | 2 +- .../handlers/static_file_handler_spec.cr | 56 +++++++++---------- spec/std/http/server/server_spec.cr | 20 ++++++- src/http/client/response.cr | 5 ++ src/http/server/response.cr | 6 ++ 7 files changed, 73 insertions(+), 43 deletions(-) diff --git a/spec/std/http/client/response_spec.cr b/spec/std/http/client/response_spec.cr index 6726cad601bc..b8e01626aba3 100644 --- a/spec/std/http/client/response_spec.cr +++ b/spec/std/http/client/response_spec.cr @@ -6,7 +6,7 @@ class HTTP::Client it "parses response with body" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nhelloworld")) response.version.should eq("HTTP/1.1") - response.status.code.should eq(200) + response.status_code.should eq(200) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("5") @@ -16,7 +16,7 @@ class HTTP::Client it "parses response with streamed body" do Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nhelloworld")) do |response| response.version.should eq("HTTP/1.1") - response.status.code.should eq(200) + response.status_code.should eq(200) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("5") @@ -34,7 +34,7 @@ class HTTP::Client it "parses response with body without \\r" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\nContent-Type: text/plain\nContent-Length: 5\n\nhelloworld")) response.version.should eq("HTTP/1.1") - response.status.code.should eq(200) + response.status_code.should eq(200) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("5") @@ -43,7 +43,7 @@ class HTTP::Client it "parses response with body but without content-length" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\r\n\r\nhelloworld")) - response.status.code.should eq(200) + response.status_code.should eq(200) response.status_message.should eq("OK") response.headers.size.should eq(0) response.body.should eq("helloworld") @@ -51,7 +51,7 @@ class HTTP::Client it "parses response with empty body but without content-length" do response = Response.from_io(IO::Memory.new("HTTP/1.1 404 Not Found\r\n\r\n")) - response.status.code.should eq(404) + response.status_code.should eq(404) response.status_message.should eq("Not Found") response.headers.size.should eq(0) response.body.should eq("") @@ -59,7 +59,7 @@ class HTTP::Client it "parses response without body" do response = Response.from_io(IO::Memory.new("HTTP/1.1 100 Continue\r\n\r\n")) - response.status.code.should eq(100) + response.status_code.should eq(100) response.status_message.should eq("Continue") response.headers.size.should eq(0) response.body?.should be_nil @@ -67,7 +67,7 @@ class HTTP::Client it "parses response without status message" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200\r\n\r\n")) - response.status.code.should eq(200) + response.status_code.should eq(200) response.status_message.should eq("") response.headers.size.should eq(0) response.body.should eq("") @@ -106,7 +106,7 @@ class HTTP::Client it "parses response ignoring body" do response = Response.from_io(IO::Memory.new("HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\nhelloworld"), true) response.version.should eq("HTTP/1.1") - response.status.code.should eq(200) + response.status_code.should eq(200) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("5") @@ -116,7 +116,7 @@ class HTTP::Client it "parses 204 response without body but Content-Length == 0 (#2512)" do response = Response.from_io(IO::Memory.new("HTTP/1.1 204 OK\r\nContent-Type: text/plain\r\nContent-Length: 0\r\n\r\n")) response.version.should eq("HTTP/1.1") - response.status.code.should eq(204) + response.status_code.should eq(204) response.status_message.should eq("OK") response.headers["content-type"].should eq("text/plain") response.headers["content-length"].should eq("0") @@ -284,9 +284,14 @@ class HTTP::Client response.charset.should eq("UTF-8") end + it "returns status_code" do + response = Response.new(HTTP::Status::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) + response.status_code.should eq(204) response.body.should eq("") end diff --git a/spec/std/http/server/handlers/error_handler_spec.cr b/spec/std/http/server/handlers/error_handler_spec.cr index ac0b86503fd2..d0f1540d7b07 100644 --- a/spec/std/http/server/handlers/error_handler_spec.cr +++ b/spec/std/http/server/handlers/error_handler_spec.cr @@ -16,7 +16,7 @@ describe HTTP::ErrorHandler do io.rewind response2 = HTTP::Client::Response.from_io(io) - response2.status.code.should eq(500) + response2.status_code.should eq(500) response2.status_message.should eq("Internal Server Error") (response2.body =~ /ERROR: OH NO!/).should be_truthy end diff --git a/spec/std/http/server/handlers/handler_spec.cr b/spec/std/http/server/handlers/handler_spec.cr index f98c773c230b..b38337f03c9b 100644 --- a/spec/std/http/server/handlers/handler_spec.cr +++ b/spec/std/http/server/handlers/handler_spec.cr @@ -22,7 +22,7 @@ describe HTTP::Handler do io.rewind response = HTTP::Client::Response.from_io(io) - response.status.code.should eq(404) + response.status_code.should eq(404) response.body.should eq("Not Found\n") end end diff --git a/spec/std/http/server/handlers/static_file_handler_spec.cr b/spec/std/http/server/handlers/static_file_handler_spec.cr index c2f0d23afed2..b9c5a4c5b9a2 100644 --- a/spec/std/http/server/handlers/static_file_handler_spec.cr +++ b/spec/std/http/server/handlers/static_file_handler_spec.cr @@ -17,7 +17,7 @@ describe HTTP::StaticFileHandler do it "serves a file" do response = handle HTTP::Request.new("GET", "/test.txt"), ignore_body: false - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end @@ -39,7 +39,7 @@ describe HTTP::StaticFileHandler do headers["If-Modified-Since"] = initial_response.headers["Last-Modified"] response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status.code.should eq(304) + response.status_code.should eq(304) response.headers["Last-Modified"].should eq initial_response.headers["Last-Modified"] response.headers["Content-Type"]?.should be_nil @@ -50,7 +50,7 @@ describe HTTP::StaticFileHandler do headers["If-Modified-Since"] = HTTP.format_time(File.info(datapath("static_file_handler", "test.txt")).modification_time + 1.hour) response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status.code.should eq(304) + response.status_code.should eq(304) end it "serves file if file mtime is younger" do @@ -58,7 +58,7 @@ describe HTTP::StaticFileHandler do headers["If-Modified-Since"] = HTTP.format_time(File.info(datapath("static_file_handler", "test.txt")).modification_time - 1.hour) response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end end @@ -70,7 +70,7 @@ describe HTTP::StaticFileHandler do headers = HTTP::Headers.new headers["If-None-Match"] = initial_response.headers["Etag"] response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status.code.should eq(304) + response.status_code.should eq(304) end it "serves file if header does not match etag" do @@ -78,7 +78,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = "some random etag" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end @@ -86,7 +86,7 @@ describe HTTP::StaticFileHandler do headers = HTTP::Headers.new headers["If-None-Match"] = "*" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status.code.should eq(304) + response.status_code.should eq(304) end it "serves file if header is empty" do @@ -94,7 +94,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = "" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end @@ -103,7 +103,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = ", foo" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end end @@ -115,7 +115,7 @@ describe HTTP::StaticFileHandler do headers = HTTP::Headers.new headers["If-None-Match"] = %(,, ,W/"1234567" , , #{initial_response.headers["Etag"]},"12345678",%) response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status.code.should eq(304) + response.status_code.should eq(304) end it "serves file if no header matches etag" do @@ -123,7 +123,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = "some random etag, 1234567" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end end @@ -137,7 +137,7 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = initial_response.headers["Etag"] response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true - response.status.code.should eq(304) + response.status_code.should eq(304) end it "serves a file if header does not match etag even If-Modified-Since is fresh" do @@ -148,48 +148,48 @@ describe HTTP::StaticFileHandler do headers["If-None-Match"] = "some random etag" response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: false - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq(File.read(datapath("static_file_handler", "test.txt"))) end end it "lists directory's entries" do response = handle HTTP::Request.new("GET", "/") - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should match(/test.txt/) end it "does not list directory's entries when directory_listing is set to false" do response = handle HTTP::Request.new("GET", "/"), directory_listing: false - response.status.code.should eq(404) + response.status_code.should eq(404) end it "does not serve a not found file" do response = handle HTTP::Request.new("GET", "/not_found_file.txt") - response.status.code.should eq(404) + response.status_code.should eq(404) end it "does not serve a not found directory" do response = handle HTTP::Request.new("GET", "/not_found_dir/") - response.status.code.should eq(404) + response.status_code.should eq(404) end it "does not serve a file as directory" do response = handle HTTP::Request.new("GET", "/test.txt/") - response.status.code.should eq(404) + response.status_code.should eq(404) end it "handles only GET and HEAD method" do %w(GET HEAD).each do |method| response = handle HTTP::Request.new(method, "/test.txt") - response.status.code.should eq(200) + response.status_code.should eq(200) end %w(POST PUT DELETE).each do |method| response = handle HTTP::Request.new(method, "/test.txt") - response.status.code.should eq(404) + response.status_code.should eq(404) response = handle HTTP::Request.new(method, "/test.txt"), false - response.status.code.should eq(405) + response.status_code.should eq(405) response.headers["Allow"].should eq("GET, HEAD") end end @@ -197,14 +197,14 @@ describe HTTP::StaticFileHandler do it "expands a request path" do %w(../test.txt ../../test.txt test.txt/../test.txt a/./b/../c/../../test.txt).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status.code.should eq(302) + response.status_code.should eq(302) response.headers["Location"].should eq("/test.txt") end # directory %w(.. ../ ../.. a/.. a/.././b/../).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status.code.should eq(302) + response.status_code.should eq(302) response.headers["Location"].should eq("/") end end @@ -212,13 +212,13 @@ describe HTTP::StaticFileHandler do it "unescapes a request path" do %w(test%2Etxt %74%65%73%74%2E%74%78%74).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq(file_text) end %w(%2E%2E/test.txt found%2F%2E%2E%2Ftest%2Etxt).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status.code.should eq(302) + response.status_code.should eq(302) response.headers["Location"].should eq("/test.txt") end end @@ -226,16 +226,16 @@ describe HTTP::StaticFileHandler do it "returns 400" do %w(%00 test.txt%00).each do |path| response = handle HTTP::Request.new("GET", "/#{path}") - response.status.code.should eq(400) + response.status_code.should eq(400) end end it "handles invalid redirect path" do response = handle HTTP::Request.new("GET", "test.txt%0A") - response.status.code.should eq(302) + response.status_code.should eq(302) response.headers["Location"].should eq "/test.txt%0A" response = handle HTTP::Request.new("GET", "/test.txt%0A") - response.status.code.should eq(404) + response.status_code.should eq(404) end end diff --git a/spec/std/http/server/server_spec.cr b/spec/std/http/server/server_spec.cr index 05dad5b556a9..7b0d27da6bf2 100644 --- a/spec/std/http/server/server_spec.cr +++ b/spec/std/http/server/server_spec.cr @@ -155,6 +155,20 @@ 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 = HTTP::Status::CREATED + response.status_code.should eq 201 + end + it "changes status and others" do io = IO::Memory.new response = Response.new(io) @@ -307,13 +321,13 @@ module HTTP socket.flush response = Client::Response.from_io(socket) - response.status.code.should eq(100) + response.status_code.should eq(100) socket << "hello" socket.flush response = Client::Response.from_io(socket) - response.status.code.should eq(200) + response.status_code.should eq(200) response.body.should eq("hello") end end @@ -340,7 +354,7 @@ module HTTP socket.flush response = Client::Response.from_io(socket) - response.status.code.should eq(400) + response.status_code.should eq(400) response.body.should eq("400 I don't want your body\n") end end diff --git a/src/http/client/response.cr b/src/http/client/response.cr index c7cab000e915..f674c8a9ac1e 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -61,6 +61,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 diff --git a/src/http/server/response.cr b/src/http/server/response.cr index bd469a93b145..e16a1e3ce003 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -62,6 +62,12 @@ class HTTP::Server headers["Content-Length"] = content_length.to_s end + # Convenience method to retrieve the HTTP status code. + def status_code + status.code + end + + # Convenience method to set the HTTP status code. def status_code=(status_code : Int32) self.status = HTTP::Status.new(status_code) end From ca20af67a5de1f53c1f9ca13731f4c7e057cdc32 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Wed, 2 Jan 2019 12:57:35 +1100 Subject: [PATCH 06/23] Use free predicate methods --- src/http/client/response.cr | 2 +- src/http/web_socket/protocol.cr | 2 +- src/oauth2/client.cr | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/http/client/response.cr b/src/http/client/response.cr index f674c8a9ac1e..b1108d8d97f6 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -94,7 +94,7 @@ class HTTP::Client::Response end def self.mandatory_body?(status : HTTP::Status) : Bool - !(status.informational? || status == HTTP::Status::NO_CONTENT || status == HTTP::Status::NOT_MODIFIED) + !(status.informational? || status.no_content? || status.not_modified?) end def self.supports_chunked?(version) : Bool diff --git a/src/http/web_socket/protocol.cr b/src/http/web_socket/protocol.cr index cb8b6077e216..883d528be57d 100644 --- a/src/http/web_socket/protocol.cr +++ b/src/http/web_socket/protocol.cr @@ -272,7 +272,7 @@ class HTTP::WebSocket::Protocol handshake.to_io(socket) socket.flush handshake_response = HTTP::Client::Response.from_io(socket) - unless handshake_response.status == HTTP::Status::SWITCHING_PROTOCOLS + unless handshake_response.status.switching_protocols? raise Socket::Error.new("Handshake got denied. Status code was #{handshake_response.status.code}.") end diff --git a/src/oauth2/client.cr b/src/oauth2/client.cr index d90d4dcdf640..4e18c65fdf6b 100644 --- a/src/oauth2/client.cr +++ b/src/oauth2/client.cr @@ -146,7 +146,7 @@ class OAuth2::Client response = HTTP::Client.post(token_uri, form: body, headers: headers) case response.status - when HTTP::Status::OK, HTTP::Status::CREATED + when .ok?, .created? OAuth2::AccessToken.from_json(response.body) else raise OAuth2::Error.from_json(response.body) From 5d3768a81e01be90967bdb5826c77de5c7aba81e Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Wed, 2 Jan 2019 23:58:09 +1100 Subject: [PATCH 07/23] Don't duplicate initializer --- src/http/client/response.cr | 13 ++----------- src/http/server/handlers/log_handler.cr | 2 +- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/http/client/response.cr b/src/http/client/response.cr index b1108d8d97f6..be95d1a8b5ee 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -21,17 +21,8 @@ class HTTP::Client::Response end end - def initialize(status_code : Int32, @body : String? = nil, @headers : Headers = Headers.new, status_message = nil, @version = "HTTP/1.1", @body_io = nil) - @status = HTTP::Status.new(status_code) - @status_message = status_message || @status.description - - 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") - 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.new(status_code), body, headers, status_message, version, body_io) end def body diff --git a/src/http/server/handlers/log_handler.cr b/src/http/server/handlers/log_handler.cr index ad2fe9a10dd1..7e76be855f42 100644 --- a/src/http/server/handlers/log_handler.cr +++ b/src/http/server/handlers/log_handler.cr @@ -11,7 +11,7 @@ class HTTP::LogHandler elapsed = Time.measure { call_next(context) } elapsed_text = elapsed_text(elapsed) - @io.puts "#{context.request.method} #{context.request.resource} - #{context.response.status.code} (#{elapsed_text})" + @io.puts "#{context.request.method} #{context.request.resource} - #{context.response.status_code} (#{elapsed_text})" rescue e @io.puts "#{context.request.method} #{context.request.resource} - Unhandled exception:" e.inspect_with_backtrace(@io) From 1f81d64a10862b3346a11d3556c1775b3827556e Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 00:02:25 +1100 Subject: [PATCH 08/23] Add HTTP::Status.from_code --- spec/std/http/status_spec.cr | 12 ++++++++++++ src/http/status.cr | 5 +++++ 2 files changed, 17 insertions(+) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index c62a018f0803..fe8a0efc4894 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -2,6 +2,18 @@ require "spec" require "http" describe HTTP::Status do + describe "#from_code" do + it "raises when given invalid status code" do + expect_raises(Exception, "Invalid HTTP status code: 1000") do + HTTP::Status.from_code(1000) + end + end + + it "returns a instance when given valid status code" do + HTTP::Status.from_code(201).should eq HTTP::Status::CREATED + end + end + describe "#code" do it "returns the status code" do HTTP::Status::INTERNAL_SERVER_ERROR.value.should eq 500 diff --git a/src/http/status.cr b/src/http/status.cr index 0e6a52c20815..49063421a752 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -68,6 +68,11 @@ enum HTTP::Status NOT_EXTENDED = 510 NETWORK_AUTHENTICATION_REQUIRED = 511 + def self.from_code(status_code : Int32) + raise "Invalid HTTP status code: #{status_code}" unless 100 <= status_code <= 999 + new(status_code) + end + def code value end From 24e04426e0912d0bdc84e670264ef75befa2f1ac Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 00:12:02 +1100 Subject: [PATCH 09/23] Switch to using HTTP::Status.from_code internally --- src/http/client/response.cr | 4 ++-- src/http/server/response.cr | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/http/client/response.cr b/src/http/client/response.cr index be95d1a8b5ee..be5dfe23f534 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -22,7 +22,7 @@ class HTTP::Client::Response 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.new(status_code), body, headers, status_message, version, body_io) + new(HTTP::Status.from_code(status_code), body, headers, status_message, version, body_io) end def body @@ -140,7 +140,7 @@ class HTTP::Client::Response raise "Invalid HTTP status code: #{pieces[1]}" end - status = HTTP::Status.new(status_code) + status = HTTP::Status.from_code(status_code) status_message = pieces[2]? ? pieces[2].chomp : "" body_type = HTTP::BodyType::OnDemand diff --git a/src/http/server/response.cr b/src/http/server/response.cr index e16a1e3ce003..7197f33595d1 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -69,7 +69,7 @@ class HTTP::Server # Convenience method to set the HTTP status code. def status_code=(status_code : Int32) - self.status = HTTP::Status.new(status_code) + self.status = HTTP::Status.from_code(status_code) end # See `IO#write(slice)`. From 78b50de04947933657d572fc732a8bc10d9be07f Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 00:45:36 +1100 Subject: [PATCH 10/23] Additional HTTP::Status specs --- spec/std/http/status_spec.cr | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index fe8a0efc4894..d00e13be948a 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -2,16 +2,20 @@ require "spec" require "http" describe HTTP::Status do - describe "#from_code" do + describe ".from_code" do it "raises when given invalid status code" do expect_raises(Exception, "Invalid HTTP status code: 1000") do HTTP::Status.from_code(1000) end end - it "returns a instance when given valid status code" do + 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 From c0dbecdc68c44319a3fb8a4cfff9abee43b7e70e Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 01:50:19 +1100 Subject: [PATCH 11/23] Revert breaking change to #respond_with_error --- spec/std/http/server/server_spec.cr | 4 ++-- src/http/server/request_processor.cr | 2 +- src/http/server/response.cr | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/std/http/server/server_spec.cr b/spec/std/http/server/server_spec.cr index 7b0d27da6bf2..ee9630bf569b 100644 --- a/spec/std/http/server/server_spec.cr +++ b/spec/std/http/server/server_spec.cr @@ -239,7 +239,7 @@ module HTTP io = IO::Memory.new response = Response.new(io) - response.respond_with_error(HTTP::Status::BAD_REQUEST) + response.respond_with_error("Bad Request", 400) io.to_s.should eq("HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\nTransfer-Encoding: chunked\r\n\r\n10\r\n400 Bad Request\n\r\n") end end @@ -334,7 +334,7 @@ module HTTP it "handles Expect: 100-continue correctly when body isn't read" do server = Server.new do |context| - context.response.respond_with_error(HTTP::Status.new(400), "I don't want your body") + context.response.respond_with_error("I don't want your body", 400) end address = server.bind_unused_port diff --git a/src/http/server/request_processor.cr b/src/http/server/request_processor.cr index f8e17e79d26d..5d6d90691991 100644 --- a/src/http/server/request_processor.cr +++ b/src/http/server/request_processor.cr @@ -25,7 +25,7 @@ class HTTP::Server::RequestProcessor break unless request if request.is_a?(HTTP::Request::BadRequest) - response.respond_with_error(HTTP::Status::BAD_REQUEST) + response.respond_with_error("Bad Request", 400) response.close return end diff --git a/src/http/server/response.cr b/src/http/server/response.cr index 7197f33595d1..fbabe6903512 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -123,10 +123,10 @@ class HTTP::Server # Generates an error response using *message* and *code*. # # Calls `reset` and then writes the given message. - def respond_with_error(status : HTTP::Status = HTTP::Status::INTERNAL_SERVER_ERROR, status_message = nil) + def respond_with_error(message = "Internal Server Error", code = 500) reset - @status = status - message = status_message || @status.description + @status = HTTP::Status.from_code(code) + message = message || @status.description self.content_type = "text/plain" self << @status.code << ' ' << message << '\n' flush From 858a7d79575d527cc2b021c87ec473c518e9d0c3 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 01:52:26 +1100 Subject: [PATCH 12/23] Review changes --- spec/std/http/status_spec.cr | 2 +- src/http/status.cr | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index d00e13be948a..36e31a7d4ccf 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -4,7 +4,7 @@ require "http" describe HTTP::Status do describe ".from_code" do it "raises when given invalid status code" do - expect_raises(Exception, "Invalid HTTP status code: 1000") do + expect_raises(ArgumentError, "Invalid HTTP status code: 1000") do HTTP::Status.from_code(1000) end end diff --git a/src/http/status.cr b/src/http/status.cr index 49063421a752..3e43373f4046 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -1,4 +1,4 @@ -# A support enum that provides additional around HTTP status codes. +# An enum that provides additional support around HTTP status codes. # # Based on [Hypertext Transfer Protocol (HTTP) Status Code Registry](https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml) # @@ -68,8 +68,10 @@ enum HTTP::Status NOT_EXTENDED = 510 NETWORK_AUTHENTICATION_REQUIRED = 511 + # Create a new status instance with the given status code, or raise an + # error if the status code given is not inside 100..999. def self.from_code(status_code : Int32) - raise "Invalid HTTP status code: #{status_code}" unless 100 <= status_code <= 999 + raise ArgumentError.new("Invalid HTTP status code: #{status_code}") unless 100 <= status_code <= 999 new(status_code) end From 0710db7ee35400e710686e8c9fc5f13dc84a3ba7 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 01:58:19 +1100 Subject: [PATCH 13/23] Adjust status code constraints --- src/http/status.cr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/http/status.cr b/src/http/status.cr index 3e43373f4046..8a6caeded360 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -69,9 +69,9 @@ enum HTTP::Status NETWORK_AUTHENTICATION_REQUIRED = 511 # Create a new status instance with the given status code, or raise an - # error if the status code given is not inside 100..999. + # error if the status code given is not inside 100..599. def self.from_code(status_code : Int32) - raise ArgumentError.new("Invalid HTTP status code: #{status_code}") unless 100 <= status_code <= 999 + raise ArgumentError.new("Invalid HTTP status code: #{status_code}") unless 100 <= status_code <= 599 new(status_code) end From 637a58558e72f775e6dc4643c072a351896453be Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 10:03:50 +1100 Subject: [PATCH 14/23] Use #code where possible --- src/http/client/response.cr | 2 +- src/http/status.cr | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/http/client/response.cr b/src/http/client/response.cr index be5dfe23f534..4cce69b9cc18 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -70,7 +70,7 @@ class HTTP::Client::Response end def to_io(io) - io << @version << ' ' << @status.value << ' ' << @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) diff --git a/src/http/status.cr b/src/http/status.cr index 8a6caeded360..290e3a0c9824 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -81,27 +81,27 @@ enum HTTP::Status # Returns `true` if the response status code is between 100 and 199. def informational? - 100 <= value <= 199 + 100 <= code <= 199 end # Returns `true` if the response status code is between 200 and 299. def success? - 200 <= value <= 299 + 200 <= code <= 299 end # Returns `true` if the response status code is between 300 and 399. def redirection? - 300 <= value <= 399 + 300 <= code <= 399 end # Returns `true` if the response status code is between 400 and 499. def client_error? - 400 <= value <= 499 + 400 <= code <= 499 end # Returns `true` if the response status code is between 500 and 599. def server_error? - 500 <= value <= 599 + 500 <= code <= 599 end # Returns the default status description of the given HTTP status code. From cb57c985f2753ccb586aa2906d046103f2174d66 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 10:13:13 +1100 Subject: [PATCH 15/23] Validate status_code as being 3 numbers between 0 and 999 --- spec/std/http/status_spec.cr | 4 ++-- src/http/status.cr | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index 36e31a7d4ccf..e7e4d8435a29 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -4,8 +4,8 @@ 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) + expect_raises(ArgumentError, "Invalid HTTP status code: 50") do + HTTP::Status.from_code(50) end end diff --git a/src/http/status.cr b/src/http/status.cr index 290e3a0c9824..55b3dd258207 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -69,9 +69,11 @@ enum HTTP::Status NETWORK_AUTHENTICATION_REQUIRED = 511 # Create a new status instance with the given status code, or raise an - # error if the status code given is not inside 100..599. + # error if the status code given is not inside 000..999. def self.from_code(status_code : Int32) - raise ArgumentError.new("Invalid HTTP status code: #{status_code}") unless 100 <= status_code <= 599 + unless status_code.to_s.size == 3 && 0 <= status_code <= 999 + raise ArgumentError.new("Invalid HTTP status code: #{status_code}") + end new(status_code) end From 2991c21a93fa97d6060ad2c09eb390615a280b1c Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 10:27:15 +1100 Subject: [PATCH 16/23] =?UTF-8?q?Don=E2=80=99t=20validate=20length=20of=20?= =?UTF-8?q?the=20integer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- spec/std/http/status_spec.cr | 4 ++-- src/http/status.cr | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index e7e4d8435a29..36e31a7d4ccf 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -4,8 +4,8 @@ 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: 50") do - HTTP::Status.from_code(50) + expect_raises(ArgumentError, "Invalid HTTP status code: 1000") do + HTTP::Status.from_code(1000) end end diff --git a/src/http/status.cr b/src/http/status.cr index 55b3dd258207..21a7d83e1c67 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -71,9 +71,7 @@ enum HTTP::Status # Create a new status instance with the given status code, or raise an # error if the status code given is not inside 000..999. def self.from_code(status_code : Int32) - unless status_code.to_s.size == 3 && 0 <= status_code <= 999 - raise ArgumentError.new("Invalid HTTP status code: #{status_code}") - end + raise ArgumentError.new("Invalid HTTP status code: #{status_code}") unless 100 <= status_code <= 999 new(status_code) end From 572bf57cc717625cf8d799769d822b812b57849f Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 10:27:45 +1100 Subject: [PATCH 17/23] Review changes --- src/http/status.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http/status.cr b/src/http/status.cr index 21a7d83e1c67..48bf736e9007 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -106,7 +106,7 @@ enum HTTP::Status # Returns the default status description of the given HTTP status code. def description : String? - case value + case code when 100 then "Continue" when 101 then "Switching Protocols" when 102 then "Processing" From fa35035e52496303978411617a9299466e3be000 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Thu, 3 Jan 2019 10:31:14 +1100 Subject: [PATCH 18/23] Update method description --- src/http/status.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http/status.cr b/src/http/status.cr index 48bf736e9007..e58e0c9860a5 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -69,7 +69,7 @@ enum HTTP::Status NETWORK_AUTHENTICATION_REQUIRED = 511 # Create a new status instance with the given status code, or raise an - # error if the status code given is not inside 000..999. + # error if the status code given is not inside 100..999. def self.from_code(status_code : Int32) raise ArgumentError.new("Invalid HTTP status code: #{status_code}") unless 100 <= status_code <= 999 new(status_code) From 9e92830c9a9a6bdc73b15a6d9a499271452b0692 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Fri, 4 Jan 2019 10:50:52 +1100 Subject: [PATCH 19/23] Use the correct method in the test --- spec/std/http/status_spec.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index 36e31a7d4ccf..cf425e3bd33d 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -20,7 +20,7 @@ describe HTTP::Status do describe "#code" do it "returns the status code" do - HTTP::Status::INTERNAL_SERVER_ERROR.value.should eq 500 + HTTP::Status::INTERNAL_SERVER_ERROR.code.should eq 500 end end From 6025a3285da9bed2b6d87477f905c21a7f41488f Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Fri, 4 Jan 2019 10:51:29 +1100 Subject: [PATCH 20/23] Use status symbol where possible --- spec/std/http/client/response_spec.cr | 28 +++++++++---------- spec/std/http/server/server_spec.cr | 4 +-- src/http/formdata.cr | 2 +- src/http/server/handler.cr | 2 +- src/http/server/handlers/error_handler.cr | 2 +- .../server/handlers/static_file_handler.cr | 8 +++--- src/http/server/handlers/websocket_handler.cr | 6 ++-- src/http/server/response.cr | 4 +-- 8 files changed, 28 insertions(+), 28 deletions(-) diff --git a/spec/std/http/client/response_spec.cr b/spec/std/http/client/response_spec.cr index b8e01626aba3..f0823e79e4a0 100644 --- a/spec/std/http/client/response_spec.cr +++ b/spec/std/http/client/response_spec.cr @@ -188,7 +188,7 @@ class HTTP::Client headers["Content-Type"] = "text/plain" headers["Content-Length"] = "5" - response = Response.new(HTTP::Status::OK, "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") @@ -200,7 +200,7 @@ class HTTP::Client headers["Content-Length"] = "5" headers["Set-Cookie"] = "foo=bar; path=/" - response = Response.new(HTTP::Status::OK, "hello", headers) + response = Response.new(:ok, "hello", headers) io = IO::Memory.new response.to_io(io) @@ -221,71 +221,71 @@ class HTTP::Client end it "sets content length from body" do - response = Response.new(HTTP::Status::OK, "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(HTTP::Status::OK) + 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(HTTP::Status::OK, 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(HTTP::Status::OK, 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(HTTP::Status::OK, "") + 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(HTTP::Status::OK, "", 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(HTTP::Status::OK, "", 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(HTTP::Status::OK, "", 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(HTTP::Status::OK, "", 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(HTTP::Status::OK, "", 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(HTTP::Status::CREATED) + response = Response.new(:created) response.status_code.should eq 201 end @@ -297,7 +297,7 @@ class HTTP::Client describe "success?" do it "returns true for the 2xx" do - response = Response.new(HTTP::Status::OK) + response = Response.new(:ok) response.success?.should eq(true) end diff --git a/spec/std/http/server/server_spec.cr b/spec/std/http/server/server_spec.cr index ee9630bf569b..2a5119d3a530 100644 --- a/spec/std/http/server/server_spec.cr +++ b/spec/std/http/server/server_spec.cr @@ -165,14 +165,14 @@ module HTTP it "retrieves status code" do io = IO::Memory.new response = Response.new(io) - response.status = HTTP::Status::CREATED + 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 = HTTP::Status::NOT_FOUND + 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") diff --git a/src/http/formdata.cr b/src/http/formdata.cr index 33e30018d130..a027a56d0912 100644 --- a/src/http/formdata.cr +++ b/src/http/formdata.cr @@ -27,7 +27,7 @@ require "./formdata/**" # end # # unless name && file -# context.response.status = HTTP::Status::BAD_REQUEST +# context.response.status = :bad_request # next # end # diff --git a/src/http/server/handler.cr b/src/http/server/handler.cr index 8099be03da49..2edbb1307cf0 100644 --- a/src/http/server/handler.cr +++ b/src/http/server/handler.cr @@ -23,7 +23,7 @@ module HTTP::Handler if next_handler = @next next_handler.call(context) else - context.response.status = HTTP::Status::NOT_FOUND + context.response.status = :not_found context.response.headers["Content-Type"] = "text/plain" context.response.puts "Not Found" end diff --git a/src/http/server/handlers/error_handler.cr b/src/http/server/handlers/error_handler.cr index 67563cbf5444..e128db0d9d0f 100644 --- a/src/http/server/handlers/error_handler.cr +++ b/src/http/server/handlers/error_handler.cr @@ -16,7 +16,7 @@ class HTTP::ErrorHandler rescue ex : Exception if @verbose context.response.reset - context.response.status = HTTP::Status::INTERNAL_SERVER_ERROR + context.response.status = :internal_server_error context.response.content_type = "text/plain" context.response.print("ERROR: ") ex.inspect_with_backtrace(context.response) diff --git a/src/http/server/handlers/static_file_handler.cr b/src/http/server/handlers/static_file_handler.cr index fde37f9b8f40..78e4ebe09daf 100644 --- a/src/http/server/handlers/static_file_handler.cr +++ b/src/http/server/handlers/static_file_handler.cr @@ -29,7 +29,7 @@ class HTTP::StaticFileHandler if @fallthrough call_next(context) else - context.response.status = HTTP::Status::METHOD_NOT_ALLOWED + context.response.status = :method_not_allowed context.response.headers.add("Allow", "GET, HEAD") end return @@ -42,7 +42,7 @@ class HTTP::StaticFileHandler # File path cannot contains '\0' (NUL) because all filesystem I know # don't accept '\0' character as file name. if request_path.includes? '\0' - context.response.status = HTTP::Status::BAD_REQUEST + context.response.status = :bad_request return end @@ -69,7 +69,7 @@ class HTTP::StaticFileHandler add_cache_headers(context.response.headers, last_modified) if cache_request?(context, last_modified) - context.response.status = HTTP::Status::NOT_MODIFIED + context.response.status = :not_modified return end @@ -90,7 +90,7 @@ class HTTP::StaticFileHandler end private def redirect_to(context, url) - context.response.status = HTTP::Status::FOUND + context.response.status = :found url = URI.escape(url) { |byte| URI.unreserved?(byte) || byte.chr == '/' } context.response.headers.add "Location", url diff --git a/src/http/server/handlers/websocket_handler.cr b/src/http/server/handlers/websocket_handler.cr index 6030ca2b51fc..9d2a85930a90 100644 --- a/src/http/server/handlers/websocket_handler.cr +++ b/src/http/server/handlers/websocket_handler.cr @@ -19,7 +19,7 @@ class HTTP::WebSocketHandler version = context.request.headers["Sec-WebSocket-Version"]? unless version == WebSocket::Protocol::VERSION - response.status = HTTP::Status::UPGRADE_REQUIRED + response.status = :upgrade_required response.headers["Sec-WebSocket-Version"] = WebSocket::Protocol::VERSION return end @@ -27,13 +27,13 @@ class HTTP::WebSocketHandler key = context.request.headers["Sec-WebSocket-Key"]? unless key - response.status = HTTP::Status::BAD_REQUEST + response.status = :bad_request return end accept_code = WebSocket::Protocol.key_challenge(key) - response.status = HTTP::Status::SWITCHING_PROTOCOLS + response.status = :switching_protocols response.headers["Upgrade"] = "websocket" response.headers["Connection"] = "Upgrade" response.headers["Sec-WebSocket-Accept"] = accept_code diff --git a/src/http/server/response.cr b/src/http/server/response.cr index fbabe6903512..f3b8b092faf7 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -33,7 +33,7 @@ class HTTP::Server # :nodoc: def initialize(@io : IO, @version = "HTTP/1.1") @headers = Headers.new - @status = HTTP::Status::OK + @status = :ok @wrote_headers = false @upgraded = false @output = output = @original_output = Output.new(@io) @@ -45,7 +45,7 @@ class HTTP::Server # This method is called by RequestProcessor to avoid allocating a new instance for each iteration. @headers.clear @cookies = nil - @status = HTTP::Status::OK + @status = :ok @wrote_headers = false @upgraded = false @output = @original_output From 4ade22aab79173d9bf4df98a83439d2ae9b827a3 Mon Sep 17 00:00:00 2001 From: Chris Hobbs Date: Sat, 5 Jan 2019 10:48:11 +1100 Subject: [PATCH 21/23] Update src/http/server/response.cr Co-Authored-By: dwightwatson --- src/http/server/response.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http/server/response.cr b/src/http/server/response.cr index f3b8b092faf7..1fbf9d1d3e4d 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -126,7 +126,7 @@ class HTTP::Server def respond_with_error(message = "Internal Server Error", code = 500) reset @status = HTTP::Status.from_code(code) - message = message || @status.description + message ||= @status.description self.content_type = "text/plain" self << @status.code << ' ' << message << '\n' flush From 3a7b199b2da010f2b769582645c7306ba70c019f Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Sat, 5 Jan 2019 10:50:26 +1100 Subject: [PATCH 22/23] Inline variable --- src/http/server/response.cr | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/http/server/response.cr b/src/http/server/response.cr index f3b8b092faf7..4f55c3f82374 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -133,8 +133,7 @@ class HTTP::Server end protected def write_headers - status_message = @status.description - @io << @version << ' ' << @status.code << ' ' << status_message << "\r\n" + @io << @version << ' ' << @status.code << ' ' << @status.description << "\r\n" headers.each do |name, values| values.each do |value| @io << name << ": " << value << "\r\n" From 456d3d13416a229c84837818638749559c90cee3 Mon Sep 17 00:00:00 2001 From: Dwight Watson Date: Sat, 5 Jan 2019 12:52:51 +1100 Subject: [PATCH 23/23] Replace .from_code with .new --- spec/std/http/status_spec.cr | 10 +++++----- src/http/client/response.cr | 4 ++-- src/http/server/response.cr | 4 ++-- src/http/status.cr | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/std/http/status_spec.cr b/spec/std/http/status_spec.cr index cf425e3bd33d..152de22ebfc9 100644 --- a/spec/std/http/status_spec.cr +++ b/spec/std/http/status_spec.cr @@ -2,19 +2,19 @@ require "spec" require "http" describe HTTP::Status do - describe ".from_code" do + describe ".new" do it "raises when given invalid status code" do expect_raises(ArgumentError, "Invalid HTTP status code: 1000") do - HTTP::Status.from_code(1000) + HTTP::Status.new(1000) end end it "returns an instance when given defined status code" do - HTTP::Status.from_code(201).should eq HTTP::Status::CREATED + HTTP::Status.new(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) + HTTP::Status.new(418).should eq HTTP::Status.new(418) end end @@ -80,7 +80,7 @@ describe HTTP::Status do end it "returns nil on non-existent status" do - HTTP::Status.new(0).description.should eq(nil) + HTTP::Status.new(999).description.should eq(nil) end end end diff --git a/src/http/client/response.cr b/src/http/client/response.cr index 4cce69b9cc18..0e8687c98f08 100644 --- a/src/http/client/response.cr +++ b/src/http/client/response.cr @@ -22,7 +22,7 @@ class HTTP::Client::Response 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) + new(HTTP::Status.new(status_code), body, headers, status_message, version, body_io) end def body @@ -140,7 +140,7 @@ class HTTP::Client::Response raise "Invalid HTTP status code: #{pieces[1]}" end - status = HTTP::Status.from_code(status_code) + status = HTTP::Status.new(status_code) status_message = pieces[2]? ? pieces[2].chomp : "" body_type = HTTP::BodyType::OnDemand diff --git a/src/http/server/response.cr b/src/http/server/response.cr index 5b07f794e443..cd1865e8772d 100644 --- a/src/http/server/response.cr +++ b/src/http/server/response.cr @@ -69,7 +69,7 @@ class HTTP::Server # Convenience method to set the HTTP status code. def status_code=(status_code : Int32) - self.status = HTTP::Status.from_code(status_code) + self.status = HTTP::Status.new(status_code) end # See `IO#write(slice)`. @@ -125,7 +125,7 @@ class HTTP::Server # Calls `reset` and then writes the given message. def respond_with_error(message = "Internal Server Error", code = 500) reset - @status = HTTP::Status.from_code(code) + @status = HTTP::Status.new(code) message ||= @status.description self.content_type = "text/plain" self << @status.code << ' ' << message << '\n' diff --git a/src/http/status.cr b/src/http/status.cr index e58e0c9860a5..380bffd0cfda 100644 --- a/src/http/status.cr +++ b/src/http/status.cr @@ -70,9 +70,9 @@ enum HTTP::Status # Create a new status instance with the given status code, or raise an # error if the status code given is not inside 100..999. - def self.from_code(status_code : Int32) + def self.new(status_code : Int32) raise ArgumentError.new("Invalid HTTP status code: #{status_code}") unless 100 <= status_code <= 999 - new(status_code) + previous_def(status_code) end def code