From 416a7e15bdfda29cd4f9b335a911bb59a416be60 Mon Sep 17 00:00:00 2001 From: James Lamont Date: Mon, 14 Sep 2020 10:46:28 +1000 Subject: [PATCH] Fix retaining setup blocks when remounting APIs --- CHANGELOG.md | 1 + Gemfile | 1 + benchmark/remounting.rb | 47 +++++++++++++++++++++++++++++++++++++++++ lib/grape/api.rb | 2 +- spec/grape/api_spec.rb | 5 +++++ 5 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 benchmark/remounting.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ec4232b79..08995a1a47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ * [#2097](https://github.com/ruby-grape/grape/pull/2097): Skip to set default value unless `meets_dependency?` - [@wanabe](https://github.com/wanabe). * [#2096](https://github.com/ruby-grape/grape/pull/2096): Fix redundant dependency check - [@braktar](https://github.com/braktar). * [#2096](https://github.com/ruby-grape/grape/pull/2098): Fix nested coercion - [@braktar](https://github.com/braktar). +* [#2102](https://github.com/ruby-grape/grape/pull/2102): Fix retaining setup blocks when remounting APIs - [@jylamont](https://github.com/jylamont). ### 1.4.0 (2020/07/10) diff --git a/Gemfile b/Gemfile index 48a26eb179..7e70dbcfee 100644 --- a/Gemfile +++ b/Gemfile @@ -17,6 +17,7 @@ end group :development do gem 'appraisal' gem 'benchmark-ips' + gem 'benchmark-memory' gem 'guard' gem 'guard-rspec' gem 'guard-rubocop' diff --git a/benchmark/remounting.rb b/benchmark/remounting.rb new file mode 100644 index 0000000000..a585c1d2e8 --- /dev/null +++ b/benchmark/remounting.rb @@ -0,0 +1,47 @@ +$LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib')) +require 'grape' +require 'benchmark/memory' + +class VotingApi < Grape::API + logger Logger.new(STDOUT) + + helpers do + def logger + VotingApi.logger + end + end + + namespace 'votes' do + get do + logger + end + end +end + +class PostApi < Grape::API + mount VotingApi +end + +class CommentAPI < Grape::API + mount VotingApi +end + +env = Rack::MockRequest.env_for('/votes', method: 'GET') + +Benchmark.memory do |api| + calls = 1000 + + api.report('using Array') do + VotingApi.instance_variable_set(:@setup, []) + calls.times { PostApi.call(env) } + puts " setup size: #{VotingApi.instance_variable_get(:@setup).size}" + end + + api.report('using Set') do + VotingApi.instance_variable_set(:@setup, Set.new) + calls.times { PostApi.call(env) } + puts " setup size: #{VotingApi.instance_variable_get(:@setup).size}" + end + + api.compare! +end diff --git a/lib/grape/api.rb b/lib/grape/api.rb index 45dd76c74d..dd8ae8d895 100644 --- a/lib/grape/api.rb +++ b/lib/grape/api.rb @@ -30,7 +30,7 @@ def inherited(api, base_instance_parent = Grape::API::Instance) # an instance that will be used to create the set up but will not be mounted def initial_setup(base_instance_parent) @instances = [] - @setup = [] + @setup = Set.new @base_parent = base_instance_parent @base_instance = mount_instance end diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 4d93256714..0d722a4b12 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -1600,6 +1600,11 @@ def self.io expect(subject.io).to receive(:write).with(message) subject.logger.info 'this will be logged' end + + it 'does not unnecessarily retain duplicate setup blocks' do + subject.logger + expect { subject.logger }.to_not change(subject.instance_variable_get(:@setup), :size) + end end describe '.helpers' do