Skip to content

Commit

Permalink
Fix for multiple default_scope all_queries options
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
2 people authored and eileencodes committed May 9, 2022
1 parent d94b65a commit da05fa3
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 3 deletions.
6 changes: 4 additions & 2 deletions activerecord/lib/active_record/scoping/default.rb
Expand Up @@ -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
Expand Down
17 changes: 17 additions & 0 deletions activerecord/test/cases/scoping/default_scoping_test.rb
Expand Up @@ -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
Expand Down
15 changes: 14 additions & 1 deletion activerecord/test/models/developer.rb
Expand Up @@ -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

Expand Down

0 comments on commit da05fa3

Please sign in to comment.