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

#1160)

* Fix bug where session cookie was only written on POST, PUT and DELETE.

* Write session cookie and cookies in separate methods.

* Remove FlashHandler, add write_flash method to TextResponse.

* Fix in error message for Session#destroy.

* Consistent indentation for multi-line arguments in Renderable.

spec/lucky/text_response_spec.cr

@@ -4,60 +4,194 @@ 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 "flash" do
+      it "writes the flash to the session" do
+        context = build_context
+        context.flash.success = "Yay!"
+        flash_json = {success: "Yay!"}.to_json
 
-    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)
+
+        context.session.get(Lucky::FlashStore::SESSION_KEY).should eq(flash_json)
+      end
+
+      it "only keeps the flash for one request" do
+        context1 = build_context
+        now_json = {success: "Yay!"}.to_json
+        context1.session.set(Lucky::FlashStore::SESSION_KEY, now_json)
+        next_json = context1.flash.to_json
+
+        context1.flash.success.should eq("Yay!")
+
+        print_response_with_body(context1)
+
+        context2 = build_context
+        context2.session.set(Lucky::FlashStore::SESSION_KEY, next_json)
+
+        context2.flash.success.should eq("")
+      end
+
+      it "keeps the flash for the next request" do
+        context1 = build_context
+        context1.flash.success = "Yay!"
+        next_json = context1.flash.to_json
+
+        print_response_with_body(context1)
 
-    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
+        context2 = build_context
+        context2.session.set(Lucky::FlashStore::SESSION_KEY, next_json)
+
+        context2.flash.success.should eq("Yay!")
+      end
     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
+    context "cookies" do
+      it "sets a cookie" do
+        context = build_context
+        context.cookies.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("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 "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
 
-        print_response_with_body(context, status: 200, body: "some body")
-        context.response.close
+      it "uses the passed in status" do
+        context = build_context
+        print_response(context, status: 300)
+        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 "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
+
+      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
@@ -67,6 +201,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,
+  body = "",
+  status = 200,
+  content_type = "text/html"
+)
   Lucky::TextResponse.new(context, content_type, body, status: status).print
 end

src/lucky/cookies/session.cr

@@ -19,7 +19,7 @@ class Lucky::Session
   end
 
   def destroy
-    {% raise "CookieJar#destroy has been renamed to CookieJar#clear to match Hash#clear" %}
+    {% raise "Session#destroy has been renamed to Session#clear to match Hash#clear" %}
   end
 
   def reset