Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug where session cookie was not always written at the right time. #1160

Merged
merged 5 commits into from
May 31, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 0 additions & 90 deletions spec/lucky/session_handler_spec.cr

This file was deleted.

183 changes: 140 additions & 43 deletions spec/lucky/text_response_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -4,60 +4,152 @@ include ContextHelper

describe Lucky::TextResponse do
describe "#print" do
it "uses the default status if none is set" do
context = build_context
print_response(context, status: nil)
context.response.status_code.should eq Lucky::TextResponse::DEFAULT_STATUS
end
context "cookies" do
it "sets a cookie" do
context = build_context
context.cookies.set(:email, "test@example.com")

it "uses the passed in status" do
context = build_context
print_response(context, status: 300)
context.response.status_code.should eq 300
end
print_response_with_body(context)

it "uses the response status if it's set, and Lucky::TextResponse status is nil" do
context = build_context
context.response.status_code = 300
print_response(context, status: nil)
context.response.status_code.should eq 300
end
context.response.headers.has_key?("Set-Cookie").should be_true
context.response.headers["Set-Cookie"].should contain("email=")
end

it "persist cookies across multiple requests using response headers from Lucky and request headers from the browser" do
context_1 = build_context
context_1.cookies.set(:email, "test@example.com")
print_response_with_body(context_1)

browser_request = build_request
cookie_header = context_1.response.cookies.map do |cookie|
cookie.to_cookie_header
end.join(", ")
browser_request.headers.add("Cookie", cookie_header)
context_2 = build_context("/", request: browser_request)

context_2.cookies.get(:email).should eq "test@example.com"
end

it "only writes updated cookies to the response" do
request = build_request
# set initial cookies via header
request.headers.add("Cookie", "cookie1=value1; cookie2=value2")
context = build_context("/", request: request)
context.cookies.set_raw(:cookie2, "updated2")

print_response_with_body(context)

context.response.headers["Set-Cookie"].should contain("cookie2=updated2")
context.response.headers["Set-Cookie"].should_not contain("cookie1")
end

it "sets a session" do
context = build_context
context.session.set(:email, "test@example.com")

print_response_with_body(context)

context.response.headers.has_key?("Set-Cookie").should be_true
context.response.headers["Set-Cookie"].should contain("_app_session")
end

it "prints no body with a head call" do
context = build_context("HEAD")
print_response_with_body(context, "Body", status: nil)
context.request.method.should eq "HEAD"
context.request.body.to_s.should eq ""
context.response.status_code.should eq 200
it "persists the session across multiple requests" do
context_1 = build_context
context_1.session.set(:email, "test@example.com")

print_response_with_body(context_1)

request = build_request
cookie_header = context_1.response.cookies.map do |cookie|
cookie.to_cookie_header
end.join("; ")
request.headers.add("Cookie", cookie_header)
context_2 = build_context("/", request: request)
print_response_with_body(context_2)

context_2.session.get(:email).should eq("test@example.com")
end

it "writes all the proper headers when a cookie is set" do
context = build_context
context
.cookies
.set(:yo, "lo")
.path("/awesome")
.expires(Time.utc(2000, 1, 1))
.domain("luckyframework.org")
.secure(true)
.http_only(true)

print_response_with_body(context)

header = context.response.headers["Set-Cookie"]
header.should contain("path=/awesome")
header.should contain("expires=Sat, 01 Jan 2000")
header.should contain("domain=luckyframework.org")
header.should contain("Secure")
header.should contain("HttpOnly")
end
end

it "gzips if enabled" do
Lucky::Server.temp_config(gzip_enabled: true) do
output = IO::Memory.new
context = build_context_with_io(output)
context.request.headers["Accept-Encoding"] = "gzip"
context "status" do
it "uses the default status if none is set" do
context = build_context
print_response(context, status: nil)
context.response.status_code.should eq Lucky::TextResponse::DEFAULT_STATUS
end

it "uses the passed in status" do
context = build_context
print_response(context, status: 300)
context.response.status_code.should eq 300
end

print_response_with_body(context, status: 200, body: "some body")
context.response.close
it "uses the response status if it's set, and Lucky::TextResponse status is nil" do
context = build_context
context.response.status_code = 300
print_response(context, status: nil)
context.response.status_code.should eq 300
end

context.response.headers["Content-Encoding"].should eq "gzip"
expected_io = IO::Memory.new
Gzip::Writer.open(expected_io) { |gzw| gzw.print "some body" }
output.to_s.ends_with?(expected_io.to_s).should be_true
it "prints no body with a head call" do
context = build_context("HEAD")
print_response_with_body(context, "Body", status: nil)
context.request.method.should eq "HEAD"
context.request.body.to_s.should eq ""
context.response.status_code.should eq 200
end
end

it "doesn't gzip when content type isn't in Lucky::Server.gzip_content_types" do
Lucky::Server.temp_config(gzip_enabled: true) do
output = IO::Memory.new
context = build_context_with_io(output)
context.request.headers["Accept-Encoding"] = "gzip"
context "compression" do
it "gzips if enabled" do
Lucky::Server.temp_config(gzip_enabled: true) do
output = IO::Memory.new
context = build_context_with_io(output)
context.request.headers["Accept-Encoding"] = "gzip"

print_response_with_body(context, status: 200, body: "some body")
context.response.close

context.response.headers["Content-Encoding"].should eq "gzip"
expected_io = IO::Memory.new
Gzip::Writer.open(expected_io) { |gzw| gzw.print "some body" }
output.to_s.ends_with?(expected_io.to_s).should be_true
end
end

it "doesn't gzip when content type isn't in Lucky::Server.gzip_content_types" do
Lucky::Server.temp_config(gzip_enabled: true) do
output = IO::Memory.new
context = build_context_with_io(output)
context.request.headers["Accept-Encoding"] = "gzip"

print_response_with_body(context, status: 200, body: "some body", content_type: "foo/bar")
context.response.close
print_response_with_body(context, status: 200, body: "some body", content_type: "foo/bar")
context.response.close

context.response.headers["Content-Encoding"]?.should_not eq "gzip"
output.to_s.ends_with?("some body").should be_true
context.response.headers["Content-Encoding"]?.should_not eq "gzip"
output.to_s.ends_with?("some body").should be_true
end
end
end
end
Expand All @@ -67,6 +159,11 @@ private def print_response(context : HTTP::Server::Context, status : Int32?)
print_response_with_body(context, "", status)
end

private def print_response_with_body(context : HTTP::Server::Context, body = "", status = 200, content_type = "text/html")
private def print_response_with_body(
context : HTTP::Server::Context,
Copy link
Member

Choose a reason for hiding this comment

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

Did the formatter do this? I ask because the one below has the first arg start on the same line as the signature. I'm thinking maybe we make them the same (one way or the other). I was curious if the formatter allowed both, or if it prefers one vs the other way. Does that make sense?

Copy link
Contributor Author

@wout wout May 29, 2020

Choose a reason for hiding this comment

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

I tend to put the first argument on a new line if everything does not fit on the same line. But the formatter does respect that and will put all following arguments on the same indentation level.

I like it because that will consistently put all multi-line arguments on the same indentation throughout the file. Otherwise, they are hanging at different places, sometimes with odd-numbered indentation levels, depending on the length of the name of the method.

Here again, if you prefer the first argument at the same line as the method, I'll change that. ☺️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, I have been moving a lot of code around there, because that's where I initially added the changes. But then I realised it should be somewhere else. That's why it has been changed.

Copy link
Member

Choose a reason for hiding this comment

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

I think this look good 👍

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be cleaner indeed, so I added those changes as well.

body = "",
status = 200,
content_type = "text/html"
)
Lucky::TextResponse.new(context, content_type, body, status: status).print
end
6 changes: 4 additions & 2 deletions src/lucky/renderable.cr
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,9 @@ module Lucky::Renderable
file(path, content_type, disposition, filename, status.value)
end

def send_text_response(body : String, content_type : String, status : Int32? = nil) : Lucky::TextResponse
def send_text_response(body : String,
content_type : String,
status : Int32? = 100) : Lucky::TextResponse
Lucky::TextResponse.new(context, content_type, body, status: status)
end

Expand All @@ -196,7 +198,7 @@ module Lucky::Renderable
{% raise "'text' in actions has been renamed to 'plain_text'" %}
end

@[Deprecated("`render_text deprecated. Use `plain_text` instead")]
@[Deprecated("`render_text` deprecated. Use `plain_text` instead")]
Copy link
Member

Choose a reason for hiding this comment

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

👍

private def render_text(*args, **named_args) : Lucky::TextResponse
plain_text(*args, **named_args)
end
Expand Down
27 changes: 0 additions & 27 deletions src/lucky/session_handler.cr

This file was deleted.

19 changes: 19 additions & 0 deletions src/lucky/text_response.cr
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ class Lucky::TextResponse < Lucky::Response
end

def print : Nil
write_session
write_cookies
Copy link
Member

Choose a reason for hiding this comment

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

Now that cookies/session are set here, I think the flash handler should also be removed and the flash set before write_session. That way it gets written to the session cookie when it is printed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I'll do that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Voila, that's done!

context.response.content_type = content_type
context.response.status_code = status
gzip if should_gzip?
Expand All @@ -42,4 +44,21 @@ class Lucky::TextResponse < Lucky::Response
Lucky::Server.settings.gzip_content_types.includes?(content_type)
{% end %}
end

private def write_session : Void
context.cookies.set(
paulcsmith marked this conversation as resolved.
Show resolved Hide resolved
Lucky::Session.settings.key,
context.session.to_json
)
end

private def write_cookies : Void
response = context.response

context.cookies.updated.each do |cookie|
response.cookies[cookie.name] = cookie
end

response.cookies.add_response_headers(response.headers)
end
end