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

wout · 2020-05-29T18:21:51Z

Purpose

This fixes #1064.

Description

Lucky::SessionHandler has been removed
Lucky::FlashHandler has been removed
flash messages, session cookie and cookies are now written in Lucky::TextResponse#print

I tested this on our app as well and everything works as expected now. 😌️

Checklist

- An issue already exists detailing the issue/or feature request that this PR fixes
- All specs are formatted with crystal tool format spec src
- Inline documentation has been added and/or updated
- Lucky builds on docker with ./script/setup
- All builds and specs pass on docker with ./script/test

wout · 2020-05-29T18:30:27Z

We'll also need to update Lucky CLI to remove Lucky::SessionHandler from the generator. I can take care of that as well if you like.

jwoertink

So glad you found the issue! This looks great to me. I just left a few small comments, but then I think it's good to go.

src/lucky/text_response.cr

jwoertink · 2020-05-29T18:31:02Z

src/lucky/renderable.cr

@@ -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")]


jwoertink · 2020-05-29T18:32:37Z

spec/lucky/text_response_spec.cr

@@ -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,


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?

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. ☺️

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.

I think this look good 👍

I'm ok with doing this. Should we do the same on https://github.com/luckyframework/lucky/pull/1160/files#diff-a949f27bf5c58356d03de361bcf6cba2R182 as well?

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

paulcsmith

Looks good! Just need to handle flash and I think it is good to go. Thanks for investigating + fixing.

Do you know why it wasn't working before, but it worked this time? Was it that it changed to the TextResponse class? Just trying to make sure I understand why this works but it didn't work when done in Renderable

paulcsmith · 2020-05-29T19:19:02Z

src/lucky/text_response.cr

@@ -20,6 +20,8 @@ class Lucky::TextResponse < Lucky::Response
  end

  def print : Nil
+    write_session
+    write_cookies


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.

Great, I'll do that as well.

Voila, that's done!

paulcsmith · 2020-05-29T19:19:29Z

spec/lucky/text_response_spec.cr

@@ -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,


I think this look good 👍

wout · 2020-05-29T19:44:59Z

Do you know why it wasn't working before, but it worked this time? Was it that it changed to the TextResponse class? Just trying to make sure I understand why this works but it didn't work when done in Renderable

Well, yeah, that's what threw me off. I didn't realise Lucky::TextResponse is initialized in two places:

So I added the write_session and write_cookies methods to the latter, not the former. It makes sense to move those methods to a more central place, especially since it's also used in other places:

https://github.com/wout/lucky/blob/1064-write-cookies-right-before-text-response/src/lucky/redirectable.cr#L62-L81

paulcsmith · 2020-05-29T20:05:49Z

Ohhh that makes total sense. And I definitely agree on adding it to TextResponse so all the responses get it. Have you tested this new update with your flash messages and can confirm those are still working as expected? If so I think this LGTM!

wout · 2020-05-29T20:33:39Z

I'll have to look closer at it tomorrow. Specs run green, but not sure about my apps.

paulcsmith · 2020-05-29T20:35:24Z

Sounds good. The flash/session stuff can be a bit gnarly with edge cases so better safe than sorry 😂

Thanks for double checking and for fixing this. Can't wait to get this out

wout · 2020-05-29T20:37:57Z

Yeah, flash messages are a breed of their own. :) I'll have to install it into an app that uses them. More tomorrow!

wout · 2020-05-30T10:52:47Z

@paulcsmith I've tested flash messages throughout one app and they all work as expected, except for the following two scenarios.

a flash message set in the SignIns::Delete action is lost
But I recall noticing this before. In one of my apps I even removed the default flash message there.
flash messages are not maintained with multiple redirects
I'm not sure if this was working before either.

paulcsmith · 2020-05-31T02:10:38Z

I think this is ok to merge. Could you open issues for those 2 redirect issues? I think they are likely bugs from before that we need to fix.

Thanks for the thorough investigation, and for the fix!

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

1b01dd0

jwoertink approved these changes May 29, 2020

View reviewed changes

Write session cookie and cookies in separate methods.

0d94590

paulcsmith reviewed May 29, 2020

View reviewed changes

Remove FlashHandler, add write_flash method to TextResponse.

6f2b4b9

Fix in error message for Session#destroy.

73b1b27

Consistent indentation for multi-line arguments in Renderable.

4e5958b

paulcsmith merged commit 02c3de6 into luckyframework:master May 31, 2020

wout deleted the 1064-write-cookies-right-before-text-response branch May 31, 2020 06:50

This was referenced May 31, 2020

Flash messages set in the SignIns::Delete action are lost #1161

Closed

Flash messages are not maintained with multiple redirects. #1162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wout commented May 29, 2020 •

edited

Loading

wout commented May 29, 2020

jwoertink left a comment

jwoertink May 29, 2020

jwoertink May 29, 2020

wout May 29, 2020 •

edited

Loading

wout May 29, 2020

paulcsmith May 29, 2020

jwoertink May 29, 2020

wout May 30, 2020

paulcsmith left a comment

paulcsmith May 29, 2020

wout May 29, 2020

wout May 29, 2020

paulcsmith May 29, 2020

wout commented May 29, 2020

paulcsmith commented May 29, 2020

wout commented May 29, 2020

paulcsmith commented May 29, 2020

wout commented May 29, 2020 •

edited

Loading

wout commented May 30, 2020 •

edited

Loading

paulcsmith commented May 31, 2020

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

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

Conversation

wout commented May 29, 2020 • edited Loading

Purpose

Description

Checklist

wout commented May 29, 2020

jwoertink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wout May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wout commented May 29, 2020

paulcsmith commented May 29, 2020

wout commented May 29, 2020

paulcsmith commented May 29, 2020

wout commented May 29, 2020 • edited Loading

wout commented May 30, 2020 • edited Loading

paulcsmith commented May 31, 2020

wout commented May 29, 2020 •

edited

Loading

wout May 29, 2020 •

edited

Loading

wout commented May 29, 2020 •

edited

Loading

wout commented May 30, 2020 •

edited

Loading