Fix for multiple default_scope all_queries options
In our use case - we have a base model that has a default scope that we want enabled for all queries, ex: ```ruby class Developer < ApplicationRecord default_scope -> { where(firm_id: Current.firm_id) }, all_queries: true end ``` We're also leveraging a module that will add a default scope to only find soft-deleted records. ```ruby module SoftDeletable extend ActiveSupport::Concern included do default_scope { where(deleted_at: nil) } end ``` Through this, we've found that when using default scopes in combination, *specifically in the use case where the _non_ all queries scope is declared first*, that we would get an error when calling `.update`: ```ruby class Developer < ApplicationRecord include SoftDeletable default_scope -> { where(firm_id: Current.firm_id) }, all_queries: true ``` ```ruby Current.firm_id = 5 developer = Developer.new(name: "Steve") developer.update(name: "Stephen") NoMethodError: undefined method `where' for nil:NilClass ``` In digging into the code, this was due to there not being an `else` case for the `inject` used to combine `default_scopes` together (`inject` uses the return value of the block as the memoizer). Without the `else` case, if the block returned `nil`, `nil` was passed to the evaluation of the next `default_scope`. This commit adds the `else`, and also makes a minor adjustment in variable naming (`default_scope` to `combined_scope`) in an effort to add a little more readability, as we're iterating over an array of default scopes, but what we're building is _the_ default scope to be used in the query, etc. Co-authored-by: Will Cosgrove <will@cosgrove.email>
This commit is contained in:
parent
d94b65aad6
commit
da05fa3381
@ -146,11 +146,13 @@ def build_default_scope(relation = relation(), all_queries: nil)
|
||||
end
|
||||
elsif default_scopes.any?
|
||||
evaluate_default_scope do
|
||||
default_scopes.inject(relation) do |default_scope, scope_obj|
|
||||
default_scopes.inject(relation) do |combined_scope, scope_obj|
|
||||
if execute_scope?(all_queries, scope_obj)
|
||||
scope = scope_obj.scope.respond_to?(:to_proc) ? scope_obj.scope : scope_obj.scope.method(:call)
|
||||
|
||||
default_scope.instance_exec(&scope) || default_scope
|
||||
combined_scope.instance_exec(&scope) || combined_scope
|
||||
else
|
||||
combined_scope
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -80,6 +80,23 @@ def test_default_scope_with_multiple_calls
|
||||
assert_equal 50000, wheres["salary"]
|
||||
end
|
||||
|
||||
def test_combined_default_scope_without_and_with_all_queries_works
|
||||
Mentor.create!
|
||||
klass = DeveloperWithIncludedMentorDefaultScopeNotAllQueriesAndDefaultScopeFirmWithAllQueries
|
||||
|
||||
create_sql = capture_sql { klass.create!(name: "Steve") }.first
|
||||
|
||||
assert_match(/mentor_id/, create_sql)
|
||||
assert_match(/firm_id/, create_sql)
|
||||
|
||||
developer = klass.find_by!(name: "Steve")
|
||||
|
||||
update_sql = capture_sql { developer.update(name: "Stephen") }.first
|
||||
|
||||
assert_no_match(/mentor_id/, update_sql)
|
||||
assert_match(/firm_id/, update_sql)
|
||||
end
|
||||
|
||||
def test_default_scope_runs_on_create
|
||||
Mentor.create!
|
||||
create_sql = capture_sql { DeveloperwithDefaultMentorScopeNot.create!(name: "Eileen") }.first
|
||||
|
@ -164,7 +164,20 @@ class DeveloperWithDefaultMentorScopeAllQueries < ActiveRecord::Base
|
||||
|
||||
class DeveloperWithDefaultNilableMentorScopeAllQueries < ActiveRecord::Base
|
||||
self.table_name = "developers"
|
||||
firm_id = nil # Could be something like Current.mentor_id
|
||||
firm_id = nil # Could be something like Current.firm_id
|
||||
default_scope -> { where(firm_id: firm_id) if firm_id }, all_queries: true
|
||||
end
|
||||
|
||||
module MentorDefaultScopeNotAllQueries
|
||||
extend ActiveSupport::Concern
|
||||
|
||||
included { default_scope { where(mentor_id: 1) } }
|
||||
end
|
||||
|
||||
class DeveloperWithIncludedMentorDefaultScopeNotAllQueriesAndDefaultScopeFirmWithAllQueries < ActiveRecord::Base
|
||||
include MentorDefaultScopeNotAllQueries
|
||||
self.table_name = "developers"
|
||||
firm_id = 10 # Could be something like Current.firm_id
|
||||
default_scope -> { where(firm_id: firm_id) if firm_id }, all_queries: true
|
||||
end
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user