From 012596a74712769bc85f7436f318acb8145c3969 Mon Sep 17 00:00:00 2001 From: Todd Mazierski Date: Tue, 17 Feb 2015 16:45:08 -0500 Subject: [PATCH 1/2] Add test to reproduce bug with Warden test helper. When using the `login_as` Warden test helper, it depends whether or not the API class under test is evaluated *before* or *after* the tests. Given that this bug first appeared in v0.10.0, my current theory is the use of `deep_dup` is responsible. Warden persists registered hooks in instance variables (ex. http://git.io/NhKG). When an API class is evaluated *before* the tests, the hooks are registered *after* duplication takes place, so the original object is left with an empty array of hooks. --- Gemfile | 1 + spec/grape/warden_test_helpers_spec.rb | 49 ++++++++++++++++++++++++++ spec/spec_helper.rb | 4 +++ 3 files changed, 54 insertions(+) create mode 100644 spec/grape/warden_test_helpers_spec.rb diff --git a/Gemfile b/Gemfile index 7809c5ecb..fb319d945 100644 --- a/Gemfile +++ b/Gemfile @@ -7,4 +7,5 @@ group :development, :test do gem 'guard' gem 'guard-rspec' gem 'guard-rubocop' + gem 'warden', '~> 1.2.3' end diff --git a/spec/grape/warden_test_helpers_spec.rb b/spec/grape/warden_test_helpers_spec.rb new file mode 100644 index 000000000..80af1aa9f --- /dev/null +++ b/spec/grape/warden_test_helpers_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +FailureApp = ->(env) { [401, {}, ['']] } + +class APIEvaluatedBeforeTests < Grape::API + use(Warden::Manager) { |manager| manager.failure_app = FailureApp } + + before { env.fetch('warden').authenticate! } + + get '/' +end + +describe Warden::Test::Helpers do + def app + subject + end + + describe '#login_as' do + before { login_as(:user) } + + context 'API evaluated before tests' do + subject { APIEvaluatedBeforeTests } + + it 'logs in' do + get '/' + expect(last_response).to be_ok + end + end + + context 'API evaluated after tests' do + let(:api_evaluated_after_tests) do + Class.new(Grape::API) do + use(Warden::Manager) { |manager| manager.failure_app = FailureApp } + + before { env.fetch('warden').authenticate! } + + get '/' + end + end + + subject { api_evaluated_after_tests } + + it 'logs in' do + get '/' + expect(last_response).to be_ok + end + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index c6cd2d063..0cecc9db4 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -14,6 +14,7 @@ require 'cookiejar' require 'json' require 'mime/types' +require 'warden' Dir["#{File.dirname(__FILE__)}/support/*.rb"].each do |file| require file @@ -23,7 +24,10 @@ RSpec.configure do |config| config.include Rack::Test::Methods + config.include Warden::Test::Helpers config.raise_errors_for_deprecations! + config.after(:each) { Warden.test_reset! } + config.before(:each) { Grape::Util::InheritableSetting.reset_global! } end From 009be00e4f6d149072ef065abeea6c3bc845ea43 Mon Sep 17 00:00:00 2001 From: Todd Mazierski Date: Sun, 12 Apr 2015 17:42:54 -0400 Subject: [PATCH 2/2] A possible fix/conversation starter for #930. @dblock's suggestion (http://git.io/vvmJj) was instead of `deep_dup`ing _all_ settings by default, to only `deep_dup` settings on a whitelist. However, after implementing this, at least according to the tests, there doesn't appear to be **any** settings need to be `deep_dup`ed. Instead, a shallow clone (a `dup`) looks to be sufficient. I've confirmed that with this change, the test `warden_test_helpers_spec.rb` passes in the suite and in isolation (i.e. the bug described is fixed). --- lib/grape/util/inheritable_values.rb | 2 +- lib/grape/util/stackable_values.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/grape/util/inheritable_values.rb b/lib/grape/util/inheritable_values.rb index 21a55d84b..2546075db 100644 --- a/lib/grape/util/inheritable_values.rb +++ b/lib/grape/util/inheritable_values.rb @@ -36,7 +36,7 @@ def to_hash def initialize_copy(other) super self.inherited_values = other.inherited_values - self.new_values = other.new_values.deep_dup + self.new_values = other.new_values.dup end protected diff --git a/lib/grape/util/stackable_values.rb b/lib/grape/util/stackable_values.rb index 4937445c1..8ce736272 100644 --- a/lib/grape/util/stackable_values.rb +++ b/lib/grape/util/stackable_values.rb @@ -45,7 +45,7 @@ def freeze_value(key) def initialize_copy(other) super self.inherited_values = other.inherited_values - self.new_values = other.new_values.deep_dup + self.new_values = other.new_values.dup end end end