Rails 5.0 Redux #66

mvastola · 2016-01-04T04:18:57Z

Building and expanding on the great work @gregmolnar in PR #59 (resolving issue #55), this PR offers an improved implementation of Rails 5.0 compatibility that does not sacrifice backward compatibility with Rails versions in the 4.x series, resolves all deprecations/warnings related to the edge versions of Ruby and Rails ~~(with the exception of the deprecation addressed by #64)~~, and is eminently ready to merge.

Advice/ideas/questions/suggestions/etc welcome as always.

rails-bot · 2016-01-04T04:18:58Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sikachu (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

mvastola · 2016-01-04T04:34:22Z

Update: I've actually decided to include @masonjm's fine work in PR #64 (fixing issue #63) into this PR, making this PR a one-stop, all-inclusive solution bringing this repository up-to-date with the latest, bleeding-edge versions of ruby, rails, and all this repo's dependent gems.

mvastola · 2016-01-04T04:39:48Z

Summary of Previous Updates

This PR:

Includes verbatim PRs: ~~Use the update Rack constant for RACK_SESSION_OPTIONS #57 (@jshirley)~~, Fix Thread.exclusive deprecation warning in Ruby 2.3 #64 (@masonjm)
Adapts Code from PR: ~~Rails 5 support #59 (@gregmolnar)~~
Supersedes PRs: ~~Remove alias_method_chain #62 (@kddeisz)~~ and setup_sessid_compatibility! after app initialized instead. #67 (@godfat)
Resolves the following issues: Known issue: Does not currently working on edge Rails (5.x.x) #55, Deprecation on Thread.exclusive #63

(PRs with strikethroughs have been closed by their creators in deference to this one.)

sikachu · 2016-01-04T17:41:13Z

I don't have time until this evening or later on next week to review and catch up on these PRs. I've pinged other core team members to take a look at this if they can do it sooner. Thanks for your patience!

mvastola · 2016-01-04T19:03:59Z

@sikachu Thanks! Fortunately, with all the closures, there aren't many PRs to look at! :-)

- Closes rails#63 - Closes rails#64 - Closes rails#67

kddnewton · 2016-01-05T14:11:54Z

lib/action_dispatch/session/active_record_store.rb

+          logger.silence_logger do
+            model = @@session_class.find_by_session_id(id)
+            if !model
+              id = generate_sid unless ActiveRecord.version.to_s.start_with?('4.')


Seems dangerous to compare this to the to_s in case that implementation changes. Not sure of best practice here but it might be better to go with ActiveRecord::VERSION::MAJOR == 4

What do you mean by "in case the implementation changes"? I added the #to_s because Rails.version returns a String and at the moment I was getting a Gem::Version from AR.version (Rails.version was undefined in this scope).

Also, the new Gemspeconly indicates compatibility until prior to 5.1. I doubt they'd change the implementation of this function barring a newly discovered bug or security issues that somehow forced a specific return type that couldn't be cast to a string..

The fact that the start_with? arg is suffixed with a dot should avoid ambiguity, I would think, even if it makes the solution hacky. That is, unless you're thinking of some case that I'm not (in which case please tell me).

Regardless though, ActiveRecord::VERSION::MAJOR == 4 is much cleaner, so assuming it works in all relevant Rails versions, I'll switch the code over to that tonight. Thanks for the suggestion!

gregmolnar · 2016-01-05T14:56:42Z

I also reviewed this PR and in my opinion it doesn't worth to keep this gem backward compatible if the price is to have these if/else statements in the code. The session is loaded at each request and it will have some performance impact. But that might be just me.

mvastola · 2016-01-05T17:18:29Z

Hm. Most of the added if/thens are to the tests. There's 1 or 2 max extra comparisons run on each page load, I think, and these (will) compare two constant integers. That said, if the owners want the extra operations removed, I'm happy to oblige.

gregmolnar · 2016-01-05T18:12:29Z

get_session has one, get_session_model has two conditions which could be removed if backward compatibility is dropped.

mvastola · 2016-01-05T19:04:59Z

Ok, so three from those two methods. Gems routinely have code that runs differently to ensure compatibility with various ruby/rails versions.

In the end, these statements can't add up to more than a dozen or so CPU cycles per page load. I think we're starting to descend into really picayune territory at that point, given the speeds of contemporary processors.

gregmolnar · 2016-01-05T20:15:48Z

It is not easy to benchmark this in a real world scenario, but you can easily check how much slower something if you do an is_a?(). So I think if you can avoid that easily, why not. It is micro-optimisation but at the end they small things are adding up.
Also, dropping backward compatibility with a major release was never an issue in the ruby world.

mvastola · 2016-01-07T22:30:46Z

@gregmolnar, so I'm just getting a chance to look at this again now. Idea/potential compromise: I think the only (non-test) file where we have this backward-compatibility issue is lib/action_dispatch/session/active_record_store.rb (the fact that it's just one file being part of the reason why I'm hesitant to break compatibility).

Would you have any objections/concerns if there was a module that extended ActiveRecordStore conditionally (that is, if we were in Rails 4.x) at file load-time (perhaps named LegacyRailsSupport, for instance) that modified the methods to add in Rails 4 support and therefore removed the need to check versions/argument types/etc during each method invocation?

I think this might be the better solution all-around, especially if it's mutually agreeable. I don't think it's necessary to change the way the tests work, since speed optimization really isn't critical there and they're hardly slow to begin with. (I will still take @kddeisz's suggestion for how the versions are compared though.)

mvastola · 2016-01-08T00:53:35Z

@gregmolnar, please peruse the new changes at your leisure. They are in keeping with my most recent comment and your concerns (I hope/believe) about added CPU cycles.
@kddeisz, I have implemented your suggestion for using a different version comparison technique as well.
The only thing I'm not sure of (and perhaps either @sikachu or @masonjm could shed some light on this), is if I need to wrap the include of the legacy compatibility module in an exclusive thread context with a Mutex like was done in the code @masonjm patched. (If needed I can do it, I just don't know if we do.)

mvastola · 2016-01-16T05:54:38Z

@sikachu, just a gentle reminder to glance at this if you get a chance (no one else on the core team seems to have).

mvastola · 2016-02-03T03:01:36Z

@sikachu, just another gentle reminder to look at this when you can. The second beta of Rails 5 was just released and the RC(s) as well as the initial release of Rails 5 are all tentatively scheduled within the month of February, so I'm anxious to get this reviewed, merged (after making any changes you may request) and released so that if any bugs crop up they can be addressed before people begin to use Rails 5 in production environments.

sikachu · 2016-02-03T15:07:34Z

Appraisals

+  gem 'actionpack',   '>= 5.0.0.alpha', '< 5.1'
+  gem 'activerecord', '>= 5.0.0.alpha', '< 5.1'
+  gem 'railties',     '>= 5.0.0.alpha', '< 5.1'
+  gem 'rack',         '>= 2.0.0.alpha', '< 3'


I think this can be ~> 2,0 now per RubyGems dependency list.

Ruby Gems has ~> 2.x. Perhaps it's equivalent to what I have now? Current release of rack is still 2.0.0.alpha..

If ~> 2.x isn't equivalent I would have to guess we need to point to the rack github repo for now? I can check later on the equivalence, but ultimately your call as to what to do.

If we change the version requirement and don't point to github and it doesn't match the alpha release, we'll obviously get an error..

I mistyped; Yeah, I wonder how/why it's picking up that dependency given it's a prerelease version. Let me experimenting with it a bit.

Ok. Otherwise I can do that later. Just on a phone now. I doubt there will be any breaking changes between the alpha and the final 2.0.0 release (and we're running separate appraisals against both the master branch and the alpha release regardless) so I'm more concerned the rest of the code passes muster with you since I know your time is ultra limited.

It works because 2.x is a prerelease version, which comes before 2.0.*. So yes, they are equivalent.

Yep, I think I just learned this today that passing any string to the version requirement turns on the prerelease flag. So this is good.

@gregmolnar

- Closes rails#55 - Excises support for Rails 4 into new (conditionally included) mod. (This commit adapted in part from code by @gregmolnar.)

mvastola · 2016-02-04T01:07:27Z

@sikachu So I went to implement your suggested change, but the best I could do (while keeping the legacy code in a separate file) was rename ActionDispatch::Session::ActiveRecordStoreLegacy to ActionDispatch::Session::LegacySupport. The problem being ActionDispatch::Session::ActiveRecordStore is a class rather than a module, and it is a subclass at that. If, given this, you have an alternate preference, let me know.

mvastola · 2016-02-04T01:10:22Z

For the sake of documenting the change, this newest commit also contains a tiny tweak wherein I moved the require 'action_dispatch/session/legacy_support' line into the same if block that decides whether or not to include the legacy code.

mvastola · 2016-02-04T14:53:17Z

@sikachu can this be merged/released then?

sikachu · 2016-02-04T15:11:39Z

Yep, I think this is good. Thank you for your patch.

I'm going to merge your change, but I'm not going to cut the gem until the RC drops, as things could still change between beta2 and RC. I believe you can use Bundler to point the gem to this repository in the meantime to test the gem out.

Fix compatibility issue with Rails 5.0

mvastola · 2016-02-04T15:27:21Z

Sounds fair. (Though it should be noted that some of the commits in the PR have nothing to do with Rails 5.)

In any case, it's only a couple of weeks, so I'm optimistic of my survival chances. :-p

mvastola · 2016-02-04T15:31:47Z

Also, merging this PR incorporated or made irrelevant 5 PRS and fully addressed 2 issues in this repo, so you may notice the queue is a little lighter now. :-)

masonjm · 2016-02-04T17:31:57Z

Thank you @sikachu and @mvastola for getting this moved along :). Kudos!

mvastola · 2016-02-04T22:50:54Z

Thank you too, @sikachu!

Close rails#70

Fix regression caused by #66. Close #71, Fix #70

rails/activerecord-session_store#66 added rails 5 compatibility to the activerecord-session_store gem Without this, we were not able to support UI load balancing which requires session information be stored in the database. The restriction on the logging gem also had to be removed as it was causing a dependency issue versioned as it was. https://bugzilla.redhat.com/show_bug.cgi?id=1300018

rails-bot assigned sikachu Jan 4, 2016

mvastola force-pushed the master branch from c7c9617 to 60d94b0 Compare January 4, 2016 04:29

mvastola force-pushed the master branch 4 times, most recently from 6098fee to b2768c9 Compare January 4, 2016 04:45

jshirley mentioned this pull request Jan 4, 2016

Use the update Rack constant for RACK_SESSION_OPTIONS #57

Closed

mvastola mentioned this pull request Jan 4, 2016

setup_sessid_compatibility! after app initialized instead. #67

Closed

jshirley and others added 2 commits January 4, 2016 21:21

Use the update Rack constant for RACK_SESSION_OPTIONS

e858ecd

Fix Thread.exclusive deprecation warning in Ruby 2.3

82a7540

- Closes rails#63 - Closes rails#64 - Closes rails#67

mvastola force-pushed the master branch from b2768c9 to 55d27e4 Compare January 5, 2016 02:24

kddnewton reviewed Jan 5, 2016
View reviewed changes

mvastola force-pushed the master branch 6 times, most recently from c533c62 to a1f8a56 Compare January 8, 2016 00:44

mvastola force-pushed the master branch from a1f8a56 to 6118ae9 Compare January 9, 2016 18:41

sikachu reviewed Feb 3, 2016
View reviewed changes

mvastola force-pushed the master branch from 6118ae9 to d3bdbd3 Compare February 4, 2016 00:58

Adding Rails 5 compatibility while maintaining backward-compatibility.

523a0c8

- Closes rails#55 - Excises support for Rails 4 into new (conditionally included) mod. (This commit adapted in part from code by @gregmolnar.)

mvastola force-pushed the master branch from d3bdbd3 to 523a0c8 Compare February 4, 2016 01:03

sikachu added a commit that referenced this pull request Feb 4, 2016

Merge pull request #66 from Partyista/master

1f1999c

Fix compatibility issue with Rails 5.0

sikachu merged commit 1f1999c into rails:master Feb 4, 2016

sikachu mentioned this pull request Feb 8, 2016

Rails 5/rack 2 support #68

Closed

mvastola added a commit to Partyista/activerecord-session_store that referenced this pull request Feb 11, 2016

Fixed regression in PR rails#66.

50769fa

mvastola added a commit to Partyista/activerecord-session_store that referenced this pull request Feb 11, 2016

Fixed regression in PR rails#66.

b5ac26b

Close rails#70

sikachu mentioned this pull request Feb 22, 2016

Fix Thread.exclusive deprecation warning in Ruby 2.3 #64

Closed

sikachu added a commit that referenced this pull request May 13, 2016

Merge pull request #77 from codeodor/master

c8f3a71

Fix regression caused by #66. Close #71, Fix #70

carbonin mentioned this pull request Jun 27, 2016

Update activerecord-session_store gem ManageIQ/manageiq#9473

Merged

Rails 5.0 Redux #66

Rails 5.0 Redux #66

Uh oh!

Conversation

mvastola commented Jan 4, 2016

Uh oh!

rails-bot commented Jan 4, 2016

Uh oh!

mvastola commented Jan 4, 2016

Uh oh!

mvastola commented Jan 4, 2016

Uh oh!

sikachu commented Jan 4, 2016

Uh oh!

mvastola commented Jan 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregmolnar commented Jan 5, 2016

Uh oh!

mvastola commented Jan 5, 2016

Uh oh!

gregmolnar commented Jan 5, 2016

Uh oh!

mvastola commented Jan 5, 2016

Uh oh!

gregmolnar commented Jan 5, 2016

Uh oh!

mvastola commented Jan 7, 2016

Uh oh!

mvastola commented Jan 8, 2016

Uh oh!

mvastola commented Jan 16, 2016

Uh oh!

mvastola commented Feb 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvastola commented Feb 4, 2016

Uh oh!

mvastola commented Feb 4, 2016

Uh oh!

mvastola commented Feb 4, 2016

Uh oh!

sikachu commented Feb 4, 2016

Uh oh!

mvastola commented Feb 4, 2016

Uh oh!

mvastola commented Feb 4, 2016

Uh oh!

masonjm commented Feb 4, 2016

Uh oh!

mvastola commented Feb 4, 2016

Uh oh!

Uh oh!