@chancancode sorry I was doing some testing and forgot to use parse instead of load in the end. Realized that before your comment and push forced already.

@huoxito 👍 I have another comment here about nested hash that got lost after the force-push. Since you mentioned problems with flash I thought that might be why.

oops replied to that one here #13910 (comment)

@huoxito a test-case for this in the test suite would be nice :)

I am testing this case and actually getting same results with both Marshal and JSON. Not sure if I am missing something because I've just got up, or problem lays somewhere else.

@huoxito you won't be able to test flash that way. What you are doing is just assigning flash and then getting it in your view without even touching session so JsonSerializer is not used at all. I haven't looked into flash and session tests, but probably these are better places to try it (this is guess only though). I will let you know if I find anything.

yeah @lukesarnacki thanks I was trying to modify this test here https://github.com/rails/rails/blob/9b2a017aa82f95911280ed597e4bf3193c9399e9/actionpack/test/dispatch/cookies_test.rb#L398-407 to create a new MessageEncryptor instance and verify the result of the decrypt_and_verify but no luck yet. Also tried to set something like request.flash. Will give it another try again later tonight.

@guilleiguaran just added a test case \o/ please let me know if that doesn't make sense

🍻

Thanks for your work on this @huoxito :). I am wondering about design of this solution though.

Main difference between Marshal and JSON is that generating latter doesn't persist data types. So consider such code:

h  = { hodor: "HODOR"}.with_indifferent_access

JSON.parse(JSON.generate(h, quirks_mode: true), quirks_mode: true).class
# => Hash

Marshal.load(Marshal.dump(h)).class
# => ActiveSupport::HashWithIndifferentAccess

Marshal (or ActionDispatch::Session::MarshalSerializer) itself can't make flash HWIA, what it does is keeping information if flash was passed as HWIA. Since JsonSerializer can't do the same, you suggested adding HWIA on top of load method. So in that case simple JsonSerializer unit test would be enough imho. Testing FlashHash etc. would be needed if we wanted to solve this on higher level and i.e. making FlashHash with indifferent access. And for me this is actually good question - where do we want to handle this?

Since session should be simple, maybe json as default is not a problem (if at least HWIA behaviour would be there), but maybe it would be good to rethink this a little bit.

@chancancode @guilleiguaran

EDIT: Same test using Marshal also wouldn't pass, which is my concern as well (cause it seems like focusing on wrong thing). @huoxito but maybe wait for more experienced guys opinions as I am kind of rails code newbie ;).

interesting @lukesarnacki thanks for pointing that out I hadn't realized it. Either way I suppose this is just a matter of where should we call with_indifferent_access? Or are we talking about drop JsonSerializer as default? First time I touched that cookie / sessions code so I'm all ears here too.

I would like to hear more opinions about this before of take a decision.

/cc @chancancode @rafaelfranca

Update: there are some additional work to be done for the serialization, we are tacking those in #13945, this will probably be fixed as part of that work