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 backward compatibility issue introduced by #126 #143

Conversation

Edouard-chin
Copy link
Member

@Edouard-chin Edouard-chin commented Mar 12, 2019

This change had the negative effect of breakin apps that were
setting configuration on the Session before the hook was
getting triggered, thus ending up with a "uninitialized constant"
error.

This patches move the session_class assignment when loading then
session.rb file as well as autoloading the
AR::SessionStore::Session constant so that application referencing
it too early will still work.

Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

While this fixes the issue, it also seems complex.

Would it make more sense to extend the existing accessor seen in #142? Like this:

Rails.application.config.session_store :active_record_store, serializer: :json

require 'active_record/session_store/session'
ActiveSupport.on_load(:active_record, yield: true) do
next unless ActiveRecord::SessionStore.autoload?(:Session)

ActionDispatch::Session::ActiveRecordStore.session_class = ActiveRecord::SessionStore::Session
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this duplicated in the session class?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should se the session_class when loading session. Do we need to do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't remove the session_class assignment when loading the session.rb file and just have the assignment in the lazy hook. This is because of the edge case I mentioned in the PR description.

TL;DR If the host application reference Session, "session.rb" gets loaded, the active_record hook is dispatched, at this point the Session constant is not yet defined since the AS hook takes precedence, calling Session one more time in the hook will just raise a undefined constant error. Chicken and egg situation really

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so we should just require 'active_record/session_store/session'in this hook them. If the file was already required it will just returnfalse`.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a gift for finding overcomplicated solutions 🤦‍♂️ . Thanks!

@Edouard-chin
Copy link
Member Author

I agree it seem over complicated but couldn't find a better alternative.

Would it make more sense to extend the existing accessor seen in

I'm not sure if I understand what you mean, the problem will persist as soon as ActiveRecord::SessionStore::Session is referenced

@kaspth
Copy link
Contributor

kaspth commented Mar 12, 2019

From #142 (comment):

# config/initializers/session_store.rb
Rails.application.config.session_store(:active_record_store)

ActiveSupport.on_load(:active_record) do
  ActiveRecord::SessionStore::Session.serializer = :json
end

It's only referenced to set the serializer (unless I'm misunderstanding how the first line works).

@Edouard-chin
Copy link
Member Author

Yeah that code is actually the right way to configure the session.

What we need to fix is the case where ActiveRecord::SessionStore::Session is called outside the on_load(:active_record) hook (like the doc advises to do https://github.com/rails/activerecord-session_store#configuration)

@Edouard-chin Edouard-chin force-pushed the ec-fix-lazy-hook-backward-compatibility branch from a32740e to 33231d8 Compare March 12, 2019 23:04
- The goal of rails#126 was to lazily require the 'session.rb' file as
  otherwise, we'd load `Active::Record` base too prematurely during
  the boot process.

  This change had the negative effect of breakin apps that were
  setting configuration on the `Session` **before** the hook was
  getting triggered, thus ending up with a "uninitialized constant"
  error.

  This patches move the `session_class` assignment when loading then
  `session.rb` file as well as autoloading the
  `AR::SessionStore::Session` constant so that application referencing
  it too early will still work.
@Edouard-chin Edouard-chin force-pushed the ec-fix-lazy-hook-backward-compatibility branch from 33231d8 to 588d83d Compare March 12, 2019 23:05
@Edouard-chin
Copy link
Member Author

This should be good to go Rafael/Kasper

@rafaelfranca rafaelfranca merged commit 831b0c9 into rails:master Mar 22, 2019
@Edouard-chin Edouard-chin deleted the ec-fix-lazy-hook-backward-compatibility branch March 22, 2019 20:42
@sikachu
Copy link
Member

sikachu commented Mar 23, 2019

Thank you! I'm going to push out 1.1.3

suhiltzaile pushed a commit to bebanjo/activerecord-session_store that referenced this pull request Apr 12, 2021
…ard-compatibility

Fix backward compatibility issue introduced by rails#126
suhiltzaile pushed a commit to bebanjo/activerecord-session_store that referenced this pull request Apr 12, 2021
…ard-compatibility

Fix backward compatibility issue introduced by rails#126
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.

4 participants