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

endpoint#present makes needless sql query #632

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 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 @@ -13,6 +13,7 @@ Next Release
* [#614](https://github.com/intridea/grape/pull/614): Params with `nil` value are now refused by `RegexpValidator` - [@dm1try](https://github.com/dm1try).
* [#494](https://github.com/intridea/grape/issues/494): Fixed performance issue with requests carrying a large payload - [@dblock](https://github.com/dblock).
* [#619](https://github.com/intridea/grape/pull/619): Convert specs to RSpec 3 syntax with Transpec - [@danielspector](https://github.com/danielspector).
* [#632](https://github.com/intridea/grape/pull/632): Usage of Grape::Endpoint#present for ActiveRecord relations doesn't make an additional query during entity's detection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fixme , please mention yourself :)
... during entity's detection - [@fixme](https://github.com/fixme).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've forgot :)


0.7.0 (4/2/2013)
=================
Expand Down
12 changes: 8 additions & 4 deletions lib/grape/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -336,14 +336,18 @@ def present(*args)
entity_class = options.delete(:with)

if entity_class.nil?
# entity class not explicitely defined, auto-detect from first object in the collection
object_instance = object.respond_to?(:first) ? object.first : object
# entity class not explicitely defined, auto-detect from relation#klass or first object in the collection
object_class = if object.respond_to?(:klass)
object.klass
else
object.respond_to?(:first) ? object.first.class : object.class
end

object_instance.class.ancestors.each do |potential|
object_class.ancestors.each do |potential|
entity_class ||= (settings[:representations] || {})[potential]
end

entity_class ||= object_instance.class.const_get(:Entity) if object_instance.class.const_defined?(:Entity) && object_instance.class.const_get(:Entity).respond_to?(:represent)
entity_class ||= object_class.const_get(:Entity) if object_class.const_defined?(:Entity) && object_class.const_get(:Entity).respond_to?(:represent)
end

root = options.delete(:root)
Expand Down
20 changes: 20 additions & 0 deletions spec/grape/entity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,26 @@ def first
get '/example'
end

it 'does not use #first method on ActiveRecord::Relation to prevent needless sql query' do
entity = Class.new(Grape::Entity)
some_relation = Class.new
some_model = Class.new

allow(entity).to receive(:represent).and_return("Auto-detect!")
allow(some_relation).to receive(:first)
allow(some_relation).to receive(:klass).and_return(some_model)

some_model.const_set :Entity, entity

subject.get '/example' do
present some_relation
end

expect(some_relation).not_to receive(:first)
get '/example'
expect(last_response.body).to eq('Auto-detect!')
end

it 'autodetection does not use Entity if it is not a presenter' do
some_model = Class.new
entity = Class.new
Expand Down