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 retaining setup blocks when remounting APIs #2102

Merged
merged 1 commit into from
Sep 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ end
group :development do
gem 'appraisal'
gem 'benchmark-ips'
gem 'benchmark-memory'
gem 'guard'
gem 'guard-rspec'
gem 'guard-rubocop'
Expand Down
47 changes: 47 additions & 0 deletions benchmark/remounting.rb
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion lib/grape/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions spec/grape/api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down