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 undefined method `DelegateClass' for Rack::Session::Cookie:Class #1610

Merged
merged 1 commit into from Feb 24, 2020

Conversation

@onigra
Copy link
Contributor

@onigra onigra commented Feb 24, 2020

Summary

Rack::Session::Cookie requires require 'delegate'

Test Repository: https://github.com/onigra/rack-undefined-method-delegate-class

Environment

  • rack 2.2.2
  • Ruby 2.7.0

Before

# require "delegate"
require "rack/session/cookie"

run ->(env) { [200, { 'Content-Type' => 'text/plain' }, ['ok']] }
$ rackup

Traceback (most recent call last):
        20: from /Users/onigra/.asdf/installs/ruby/2.7.0/bin/rackup:23:in `<main>'
        19: from /Users/onigra/.asdf/installs/ruby/2.7.0/bin/rackup:23:in `load'
        18: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/bin/rackup:5:in `<top (required)>'
        17: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/server.rb:168:in `start'
        16: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/server.rb:311:in `start'
        15: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/server.rb:379:in `handle_profiling'
        14: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/server.rb:312:in `block in start'
        13: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/server.rb:422:in `wrapped_app'
        12: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/server.rb:249:in `app'
        11: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/server.rb:349:in `build_app_and_options_from_config'
        10: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/builder.rb:66:in `parse_file'
         9: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/builder.rb:105:in `load_file'
         8: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/builder.rb:116:in `new_from_string'
         7: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/builder.rb:116:in `eval'
         6: from /Users/onigra/src/github.com/onigra/rack-undefined-method-delegate-class/config.ru:2:in `block in <main>'
         5: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72:in `require'
         4: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72:in `require'
         3: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/session/cookie.rb:9:in `<top (required)>'
         2: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/session/cookie.rb:11:in `<module:Rack>'
         1: from /Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/session/cookie.rb:50:in `<module:Session>'
/Users/onigra/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/session/cookie.rb:155:in `<class:Cookie>': undefined method `DelegateClass' for Rack::Session::Cookie:Class (NoMethodError)

After

require "delegate"
require "rack/session/cookie"

run ->(env) { [200, { 'Content-Type' => 'text/plain' }, ['ok']] }
$ rackup

[2020-02-24 14:13:29] INFO  WEBrick 1.6.0
[2020-02-24 14:13:29] INFO  ruby 2.7.0 (2019-12-25) [x86_64-darwin18]
[2020-02-24 14:13:29] INFO  WEBrick::HTTPServer#start: pid=5735 port=9292
@jeremyevans
Copy link
Collaborator

@jeremyevans jeremyevans commented Feb 24, 2020

While adding the delegate require does fix the problem, accepting this will lead to the slippery slope of adding such internal requires for all deep requires of rack. That is something we are trying to avoid starting in Rack 2.2. As stated in the release notes for 2.2.0, under Changed:

  • Rely on autoload to load constants instead of requiring internal files, make sure to require 'rack' and not just 'rack/...'.

If you change your require to be "rack" instead of "rack/session/cookie", that will solve the problem. That is the recommended way going forward. While this particular issue was not expected, it was expected there would some fallout from the change to rely more heavily on autoload.

I'm not going to close this immediately, I would like to get some feedback from other members of the core team first.

@ioquatix
Copy link
Member

@ioquatix ioquatix commented Feb 24, 2020

I think individual files should pull in relevant dependencies, and as such I would accept a PR to fix this. Personally, I think autoload is tangential to this issue, as that is about loading files relative to rack/lib, not stdlib or other gems.

@jeremyevans
Copy link
Collaborator

@jeremyevans jeremyevans commented Feb 24, 2020

@ioquatix Just to confirm, you are OK with this PR because the require is for a stdlib library, but you wouldn't be OK with adding a require for rack/*, even if you got the same result (direct require of rack/session/cookie not working)? Note that I'm not opposed to that approach, I'm just trying to understand the reasoning.

@tenderlove
Copy link
Member

@tenderlove tenderlove commented Feb 24, 2020

If you change your require to be "rack" instead of "rack/session/cookie", that will solve the problem.

How would this fix it? delegate needs to be required at some point. I don't see it required at all:

[aaron@tc-lan-adapter ~/g/rack (master)]$ git grep delegate | grep require
[aaron@tc-lan-adapter ~/g/rack (master)]$

Since rack/session/cookie is the one using DelegateClass, it seems to be the right place to put the require.

@tenderlove
Copy link
Member

@tenderlove tenderlove commented Feb 24, 2020

[aaron@tc-lan-adapter ~/g/rack (master)]$ ruby --disable-gems -I lib -r rack -e'p Rack::Session::Cookie'
Traceback (most recent call last):
	5: from -e:1:in `<main>'
	4: from -e:1:in `require'
	3: from /Users/aaron/git/rack/lib/rack/session/cookie.rb:9:in `<top (required)>'
	2: from /Users/aaron/git/rack/lib/rack/session/cookie.rb:11:in `<module:Rack>'
	1: from /Users/aaron/git/rack/lib/rack/session/cookie.rb:50:in `<module:Session>'
/Users/aaron/git/rack/lib/rack/session/cookie.rb:155:in `<class:Cookie>': undefined method `DelegateClass' for Rack::Session::Cookie:Class (NoMethodError)
@jeremyevans jeremyevans merged commit 9cad48e into rack:master Feb 24, 2020
18 checks passed
18 checks passed
test (ubuntu-latest, 2.3)
Details
test (ubuntu-latest, 2.4)
Details
test (ubuntu-latest, 2.5)
Details
test (ubuntu-latest, 2.6)
Details
test (ubuntu-latest, 2.7)
Details
test (ubuntu-latest, jruby)
Details
test (ubuntu-latest, truffleruby-head)
Details
test (macos-latest, 2.3)
Details
test (macos-latest, 2.4)
Details
test (macos-latest, 2.5)
Details
test (macos-latest, 2.6)
Details
test (macos-latest, 2.7)
Details
test (macos-latest, jruby)
Details
test (macos-latest, truffleruby-head)
Details
external (ubuntu-latest, 2.6)
Details
external (ubuntu-latest, 2.7)
Details
external (macos-latest, 2.6)
Details
external (macos-latest, 2.7)
Details
@jeremyevans
Copy link
Collaborator

@jeremyevans jeremyevans commented Feb 24, 2020

Yep, @tenderlove is correct. When I originally tested this, it appeared to work with just requiring rack, but my testing was broken. I'll merge this now.

@onigra
Copy link
Contributor Author

@onigra onigra commented Feb 25, 2020

Thank you!! 🙌

@onigra onigra deleted the onigra:fix-undefined-method-delegate-class-on-rack-session-cookie branch Feb 25, 2020
@ioquatix
Copy link
Member

@ioquatix ioquatix commented Feb 25, 2020

Do we want to back port this or does it not matter?

@jeremyevans
Copy link
Collaborator

@jeremyevans jeremyevans commented Feb 25, 2020

I think it is a candidate for 2.2.3. However, the workaround is fairly simple, so I don't feel strongly about it.

@ioquatix ioquatix added this to In progress in Development via automation Feb 25, 2020
@ioquatix ioquatix moved this from In progress to Backport in Development Feb 25, 2020
mhenrixon added a commit to mhenrixon/sidekiq-unique-jobs that referenced this pull request Mar 21, 2020
mhenrixon added a commit to mhenrixon/sidekiq-unique-jobs that referenced this pull request Mar 21, 2020
mhenrixon added a commit to mhenrixon/sidekiq-unique-jobs that referenced this pull request Mar 21, 2020
mhenrixon added a commit to mhenrixon/sidekiq-unique-jobs that referenced this pull request Mar 21, 2020
* Bump gems

* Bump ruby version for coverage

## To make a commit, type your commit message and press SUPER-ENTER.
## To cancel the commit, close the window. To sign off on the commit,
## press SUPER-S.
##
## You may also reference or close a GitHub issue with this commit.
## To do so, type `#` followed by the `tab` key.  You will be shown a
## list of issues related to the current repo.  You may also type
## `owner/repo#` plus the `tab` key to reference an issue in a
## different GitHub repo.

 spec/spec_helper.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 15d8d26..6a7140b 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -2,7 +2,7 @@

 require "bundler/setup"

-if RUBY_ENGINE == "ruby" && RUBY_VERSION >= "2.5" && RUBY_VERSION < "2.6"
+if RUBY_ENGINE == "ruby" && RUBY_VERSION >= "2.6" && RUBY_VERSION < "2.7"
   require "simplecov" unless %w[false 0].include?(ENV["COV"])

   begin

* Fallback to UNIQUE_KEY

* Fix console spec

* Mandatory rubocop commit

* Due to bug in rack

rack/rack#1610

* Fix broken spec

* Da fork jRuby...?

* Skip spec for jruby

* sodsohdoasdhifa;sodb ;oqwuefg2398yr	2t89f	ouwefqo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development
  
Backport
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.