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

Add rubocop - # frozen_string_literal #74

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
5 changes: 3 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ jobs:
- name: Install dependencies
run: bundle install
- name: Run tests
run:
bundle exec rspec
run: bundle exec rspec
- name: Rubocop
run: bundle exec rubocop --parallel
16 changes: 16 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
inherit_gem:
rubocop-rails_config:
- config/rails.yml

AllCops:
TargetRubyVersion: 3.1

Style/ClassAndModuleChildren:
EnforcedStyle: nested

Lint/Debugger:
Enabled: true

Style/StringLiterals:
Enabled: true
EnforcedStyle: single_quotes
7 changes: 5 additions & 2 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# frozen_string_literal: true

source 'https://rubygems.org'

gem "activerecord", ">=7"
gem "sqlite3", "~> 1.4"
gem 'activerecord', '>=7'
gem 'sqlite3', '~> 1.4'
gem 'rubocop-rails_config', '~> 1.16'
# Specify your gem's dependencies in jit_preloader.gemspec
gemspec
30 changes: 15 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,16 @@ This gem will publish an `n_plus_one_query` event via ActiveSupport::Notificatio

You could implement some basic tracking. This will let you measure the extent of the N+1 query problems in your app:
```ruby
ActiveSupport::Notifications.subscribe("n_plus_one_query") do |event, data|
ActiveSupport::Notifications.subscribe('n_plus_one_query') do |event, data|
statsd.increment "web.#{Rails.env}.n_plus_one_queries.global"
end
```

You could log the N+1 queries. In your development environment, you could throw N+1 queries into the logs along with a stack trace:
```ruby
ActiveSupport::Notifications.subscribe("n_plus_one_query") do |event, data|
ActiveSupport::Notifications.subscribe('n_plus_one_query') do |event, data|
message = "N+1 Query detected: #{data[:association]} on #{data[:source].class}"
backtrace = caller.select{|r| r.starts_with?(Rails.root.to_s) }
backtrace = caller.select { |r| r.starts_with?(Rails.root.to_s) }
Rails.logger.debug("\n\n#{message}\n#{backtrace.join("\n")}\n".red)
end
```
Expand All @@ -113,7 +113,7 @@ config.around(:each) do |example|
raise QueryError.new(message)
end
end
ActiveSupport::Notifications.subscribed(callback, "n_plus_one_query") do
ActiveSupport::Notifications.subscribed(callback, 'n_plus_one_query') do
example.run
end
end
Expand Down Expand Up @@ -144,7 +144,7 @@ There is now a `has_many_aggregate` method available for ActiveRecord::Base. Thi
```ruby
# old
Contact.all.each do |contact|
contact.addresses.maximum("LENGTH(street)")
contact.addresses.maximum('LENGTH(street)')
contact.addresses.count
end
# SELECT * FROM contacts
Expand All @@ -159,8 +159,8 @@ end
#new
class Contact < ActiveRecord::Base
has_many :addresses
has_many_aggregate :addresses, :max_street_length, :maximum, "LENGTH(street)", default: nil
has_many_aggregate :addresses, :count_all, :count, "*"
has_many_aggregate :addresses, :max_street_length, :maximum, 'LENGTH(street)', default: nil
has_many_aggregate :addresses, :count_all, :count, '*'
end

Contact.jit_preload.each do |contact|
Expand All @@ -177,7 +177,7 @@ Furthermore, there is an argument `max_ids_per_query` setting max ids per query.
```ruby
class Contact < ActiveRecord::Base
has_many :addresses
has_many_aggregate :addresses, :count_all, :count, "*", max_ids_per_query: 10
has_many_aggregate :addresses, :count_all, :count, '*', max_ids_per_query: 10
end

Contact.jit_preload.each do |contact|
Expand All @@ -197,26 +197,26 @@ This is a method `preload_scoped_relation` that is available that can handle thi
#old
class Contact < ActiveRecord::Base
has_many :addresses
has_many :usa_addresses, ->{ where(country: Country.find_by_name("USA")) }
has_many :usa_addresses, -> { where(country: Country.find_by_name('USA')) }
end

Contact.jit_preload.all.each do |contact|
# This will preload the association as expected, but it must be defined as an association in advance
contact.usa_addresses

# This will preload as the entire addresses association, and filters it in memory
contact.addresses.select{|address| address.country == Country.find_by_name("USA") }
contact.addresses.select { |address| address.country == Country.find_by_name('USA') }

# This is an N+1 query
contact.addresses.where(country: Country.find_by_name("USA"))
contact.addresses.where(country: Country.find_by_name('USA'))
end

# New
Contact.jit_preload.all.each do |contact|
contact.preload_scoped_relation(
name: "USA Addresses",
name: 'USA Addresses',
base_association: :addresses,
preload_scope: Address.where(country: Country.find_by_name("USA"))
preload_scope: Address.where(country: Country.find_by_name('USA'))
)
end
# SELECT * FROM contacts
Expand All @@ -236,14 +236,14 @@ JitPreloader.globally_enabled = true
# Can also be given anything that responds to `call`.
# You could build a kill switch with Redis (or whatever you'd like)
# so that you can turn it on or off dynamically.
JitPreloader.globally_enabled = ->{ $redis.get('always_jit_preload') == 'on' }
JitPreloader.globally_enabled = -> { $redis.get('always_jit_preload') == 'on' }

# Setting global max ids constraint on all aggregation methods.
JitPreloader.max_ids_per_query = 10

class Contact < ActiveRecord::Base
has_many :emails
has_many_aggregate :emails, :count_all, :count, "*"
has_many_aggregate :emails, :count_all, :count, '*'
end

# When enabled globally, this would not generate an N+1 query.
Expand Down
3 changes: 2 additions & 1 deletion Rakefile
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
require "bundler/gem_tasks"
# frozen_string_literal: true

require 'bundler/gem_tasks'
42 changes: 23 additions & 19 deletions jit_preloader.gemspec
Original file line number Diff line number Diff line change
@@ -1,34 +1,38 @@
# coding: utf-8
# frozen_string_literal: true

lib = File.expand_path('../lib', __FILE__)
$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
require 'jit_preloader/version'

Gem::Specification.new do |spec|
spec.name = "jit_preloader"
spec.name = 'jit_preloader'
spec.version = JitPreloader::VERSION
spec.authors = ["Kyle d'Oliveira"]
spec.email = ["[email protected]"]
spec.summary = %q{Tool to understand N+1 queries and to remove them}
spec.description = %q{The JitPreloader has the ability to send notifications when N+1 queries occur to help guage how problematic they are for your code base and a way to remove all of the commons explicitly or automatically}
spec.homepage = "https://github.com/clio/jit_preloader"
spec.metadata["homepage_uri"] = spec.homepage
spec.metadata["source_code_uri"] = spec.homepage
spec.email = ['[email protected]']
spec.summary = 'Tool to understand N+1 queries and to remove them'
spec.description = 'The JitPreloader has the ability to send notifications when N+1 queries occur to help guage how problematic they are for your code base and a way to remove all of the commons explicitly or automatically'
spec.homepage = 'https://github.com/clio/jit_preloader'
spec.metadata['homepage_uri'] = spec.homepage
spec.metadata['source_code_uri'] = spec.homepage
spec.required_ruby_version = '>= 3.0.0'

spec.license = "MIT"
spec.license = 'MIT'

spec.files = `git ls-files -z`.split("\x0")
spec.files = Dir.glob('lib/**/*.rb') + [File.basename(__FILE__)]
spec.executables = spec.files.grep(%r{^bin/}) { |f| File.basename(f) }
spec.test_files = spec.files.grep(%r{^(test|spec|features)/})
spec.require_paths = ["lib"]
spec.require_paths = ['lib']

spec.add_dependency "activerecord", "< 8"
spec.add_dependency "activesupport"
spec.add_dependency 'activerecord', '< 8'
spec.add_dependency 'activesupport'

spec.add_development_dependency "bundler"
spec.add_development_dependency "rake", "~> 13.0"
spec.add_development_dependency "rspec"
spec.add_development_dependency "database_cleaner"
spec.add_development_dependency "sqlite3"
spec.add_development_dependency "byebug"
spec.add_development_dependency "db-query-matchers"
spec.add_development_dependency 'bundler'
spec.add_development_dependency 'rake', '~> 13.0'
spec.add_development_dependency 'rspec'
spec.add_development_dependency 'database_cleaner'
spec.add_development_dependency 'sqlite3'
spec.add_development_dependency 'rubocop-rails_config'
spec.add_development_dependency 'byebug'
spec.add_development_dependency 'db-query-matchers'
end
9 changes: 5 additions & 4 deletions lib/jit_preloader.rb
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
# frozen_string_literal: true

require 'active_support/concern'
require 'active_support/core_ext/module/delegation'
require 'active_support/notifications'
require 'active_record'

require "jit_preloader/version"
require 'jit_preloader/version'
require 'jit_preloader/active_record/base'
require 'jit_preloader/active_record/relation'
require 'jit_preloader/active_record/associations/collection_association'
require 'jit_preloader/active_record/associations/singular_association'
if Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new("7.0.0")
if Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new('7.0.0')
require 'jit_preloader/active_record/associations/preloader/ar7_association'
require 'jit_preloader/active_record/associations/preloader/ar7_branch'
elsif Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new("6.1.0")
elsif Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new('6.1.0')
require 'jit_preloader/active_record/associations/preloader/ar6_association'
else
require 'jit_preloader/active_record/associations/preloader/collection_association'
Expand Down Expand Up @@ -41,5 +43,4 @@ def self.globally_enabled?
@enabled
end
end

end
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

module JitPreloader
module ActiveRecordAssociationsCollectionAssociation

def load_target
was_loaded = loaded?

Expand All @@ -17,10 +18,10 @@ def load_target
JitPreloader::Preloader.attach(records) if records.any? && !jit_loaded && JitPreloader.globally_enabled?

# If the records were not pre_loaded
records.each{ |record| record.jit_n_plus_one_tracking = true }
records.each { |record| record.jit_n_plus_one_tracking = true }

if !jit_loaded && owner.jit_n_plus_one_tracking
ActiveSupport::Notifications.publish("n_plus_one_query",
ActiveSupport::Notifications.publish('n_plus_one_query',
source: owner, association: reflection.name)
end
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

module JitPreloader
module PreloaderAssociation

# A monkey patch to ActiveRecord. The old method looked like the snippet
# below. Our changes here are that we remove records that are already
# part of the target, then attach all of the records to a new jit preloader.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

module JitPreloader
module PreloaderAssociation

# A monkey patch to ActiveRecord. The old method looked like the snippet
# below. Our changes here are that we remove records that are already
# part of the target, then attach all of the records to a new jit preloader.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# frozen_string_literal: true

module JitPreloader
module PreloaderBranch
"""
''"
ActiveRecord version >= 7.x.x introduced an improvement for preloading associations in batches:
https://github.com/rails/rails/blob/main/activerecord/lib/active_record/associations/preloader.rb#L121

Expand All @@ -9,7 +11,7 @@ module PreloaderBranch
But this change breaks that behaviour because now Batch is calling `klass.base_class` (a method defined by ActiveRecord::Base)
before we have a chance to filter out the non-AR classes.
This patch for AR 7.x makes the Branch class ignore any association loaders that aren't for ActiveRecord::Base subclasses.
"""
"''

def loaders
@loaders = super.find_all do |loader|
Expand Down
Original file line number Diff line number Diff line change
@@ -1,34 +1,42 @@
class ActiveRecord::Associations::Preloader::CollectionAssociation
private
# A monkey patch to ActiveRecord. The old method looked like the snippet
# below. Our changes here are that we remove records that are already
# part of the target, then attach all of the records to a new jit preloader.
#
# def preload(preloader)
# associated_records_by_owner(preloader).each do |owner, records|
# association = owner.association(reflection.name)
# association.loaded!
# association.target.concat(records)
# records.each { |record| association.set_inverse_instance(record) }
# end
# end
# frozen_string_literal: true

def preload(preloader)
return unless (reflection.scope.nil? || reflection.scope.arity == 0) && klass.ancestors.include?(ActiveRecord::Base)
all_records = []
associated_records_by_owner(preloader).each do |owner, records|
association = owner.association(reflection.name)
association.loaded!
# It is possible that some of the records are loaded already.
# We don't want to duplicate them, but we also want to preserve
# the original copy so that we don't blow away in-memory changes.
new_records = association.target.any? ? records - association.target : records
module ActiveRecord
module Associations
module Preloader
class CollectionAssociation
private
# A monkey patch to ActiveRecord. The old method looked like the snippet
# below. Our changes here are that we remove records that are already
# part of the target, then attach all of the records to a new jit preloader.
#
# def preload(preloader)
# associated_records_by_owner(preloader).each do |owner, records|
# association = owner.association(reflection.name)
# association.loaded!
# association.target.concat(records)
# records.each { |record| association.set_inverse_instance(record) }
# end
# end

association.target.concat(new_records)
new_records.each { |record| association.set_inverse_instance(record) }
def preload(preloader)
return unless (reflection.scope.nil? || reflection.scope.arity == 0) && klass.ancestors.include?(ActiveRecord::Base)
all_records = []
associated_records_by_owner(preloader).each do |owner, records|
association = owner.association(reflection.name)
association.loaded!
# It is possible that some of the records are loaded already.
# We don't want to duplicate them, but we also want to preserve
# the original copy so that we don't blow away in-memory changes.
new_records = association.target.any? ? records - association.target : records

all_records.concat(records) if owner.jit_preloader || JitPreloader.globally_enabled?
association.target.concat(new_records)
new_records.each { |record| association.set_inverse_instance(record) }

all_records.concat(records) if owner.jit_preloader || JitPreloader.globally_enabled?
end
JitPreloader::Preloader.attach(all_records) if all_records.any?
end
end
end
JitPreloader::Preloader.attach(all_records) if all_records.any?
end
end
Loading