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

Remove order in has_many_aggregate queries #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
3 changes: 2 additions & 1 deletion lib/jit_preloader/active_record/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def has_many_aggregate(assoc, name, aggregate, field, table_alias_name: nil, def
end

association_scope = klass.all.merge(association(assoc).scope).unscope(where: aggregate_association.foreign_key)
association_scope = association_scope.instance_exec(&reflection.scope).reorder(nil) if reflection.scope
association_scope = association_scope.instance_exec(&reflection.scope) if reflection.scope

# If the query uses an alias for the association, use that instead of the table name
table_reference = table_alias_name
Expand All @@ -134,6 +134,7 @@ def has_many_aggregate(assoc, name, aggregate, field, table_alias_name: nil, def
preloaded_data = Hash[association_scope
.where(conditions)
.group(group_by)
.reorder(nil)
.send(aggregate, field)
]

Expand Down
7 changes: 7 additions & 0 deletions spec/lib/jit_preloader/preloader_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,13 @@
expect(source_map).to eql({})
end

it "does NOT include ORDER BY in query" do
Contact.jit_preload.each do |c|
expect { c.addresses_count }.to_not make_database_queries(matching: 'ORDER BY')
expect { c.addresses_with_scope_count }.to_not make_database_queries(matching: 'ORDER BY')
end
end

it "can handle dynamic queries" do
Contact.jit_preload.each_with_index do |c, i|
expect(c.addresses_count(country: usa)).to eql usa_addresses_counts[i]
Expand Down
6 changes: 6 additions & 0 deletions spec/support/models.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ class Contact < ActiveRecord::Base
belongs_to :contact_owner, polymorphic: true

has_many :addresses
has_many :addresses_with_scope, -> { desc }, class_name: "Address"
has_many :phone_numbers
has_one :email_address
has_many :employees

has_many_aggregate :addresses, :max_street_length, :maximum, "LENGTH(street)"
has_many_aggregate :phone_numbers, :count, :count, "id"
has_many_aggregate :addresses, :count, :count, "*"
has_many_aggregate :addresses_with_scope, :count, :count, "*"

scope :desc, ->{ order(id: :desc) }
end
Expand Down Expand Up @@ -57,8 +59,12 @@ class Child < Contact
end

class Address < ActiveRecord::Base
default_scope { order(id: :asc) }

belongs_to :contact
belongs_to :country

scope :desc, ->{ reorder(id: :desc) }
end

class EmailAddress < ActiveRecord::Base
Expand Down