Skip to content

Do not coerce session keys to strings for getters/setters#523

Open
radar wants to merge 1 commit into
mainfrom
session-without-strings
Open

Do not coerce session keys to strings for getters/setters#523
radar wants to merge 1 commit into
mainfrom
session-without-strings

Conversation

@radar

@radar radar commented Jul 4, 2026

Copy link
Copy Markdown

I came across a bug today where I had this code in my action's test:

  let(:params) do
    {
      book_permalink: "book-permalink",
      chapter_permalink: "chapter-permalink",
      note_id: 1,
      text: "Updated note text",
      "rack.session" => { user_id: 1 },
    }
  end

But then when attempting to access the session's value in the action's code:

request.session[:user_id] => nil

This is because the previous implementation was stringifying keys. There wasn't a session key stored as "user_id" in the session, so it returns nil.

This PR removes the cooercing-to-string of these values. So whatever is put in there is now maintained in the data type it was insertedin.

@radar radar changed the title Do not cooerce session keys to strings for getters/setters Do not coerce session keys to strings for getters/setters Jul 4, 2026
@radar radar force-pushed the session-without-strings branch from 1c932ee to 2095179 Compare July 4, 2026 22:51
@parndt

parndt commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Nice; I've definitely had this issue in the past.

@timriley

timriley commented Jul 5, 2026

Copy link
Copy Markdown
Member

Thanks for this, @radar! I think this is a good change. One thing I want to do with this is have a closer poke at the ramifications for our Rack 2/3 compatibility. This string-key-ifying session wrapper was introduced in #477 as part of our Rack 3 compatibility effort, and I just want to be doubly sure that we're not introducing any regressions (the fact that our tests are happy is a good sign!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants