rubocop / rails-style-guide

A community-driven Ruby on Rails style guide
http://rails.rubystyle.guide
6.48k stars 1.06k forks source link

Avoiding ActiveRecord `default_scope` #266

Closed gadimbaylisahil closed 4 years ago

gadimbaylisahil commented 4 years ago

Hi, Rubocop community!

I have myself used default_scope in Active Record and have always regretted it. In general the community opinion on default_scope is overall negative (correct me if I may be wrong, please).

Main problem is its implicitness in associations for me. Let's say we have:

class Transaction < ActiveRecord::Base
  default_scope { where(reversed: false) }
end

class CapturedPayment < ActiveRecord::Base
  belongs_to :transaction
end

captured_payment = CapturedPayment.find(n)

# If the transaction of the captured_payment has been reversed,
# when calling

captured_payment.transaction 

# The result will return nil as it will apply default scope to it

Googling default_scope already returns quiet a few results:

link1 link2 link3

It being usually problematic, how about to recommend to avoid it in style guides?

Associated PR: https://github.com/rubocop-hq/rails-style-guide/pull/267 Associated PR in rubocop-hq/rubocop-rails: https://github.com/rubocop-hq/rubocop-rails/pull/267

flanger001 commented 4 years ago

default_scope's one good use is with soft deletion libraries.

default_scope { where(deleted_at: nil) }

Most of the time you want only non-deleted records, so this is valid. I reject the argument that it's better to spray .not_deleted everywhere on your scopes; this (or forgetting to do this, more importantly) has bitten me far more often than the default scope option has.

But otherwise I think it is problematic or "evil" as we are so accustomed to saying. I think the thing to recommend is an explicit scope.

gadimbaylisahil commented 4 years ago

Thanks for input!

Yes, that is the one use case. With every tool we get conveniences and some drawbacks alongside. Question would be is it worth it? Should we compare these drawbacks and come to a conclusion?

For soft deleting itself I am mostly against for user data, but that's whole another topic and use case is not only limited to that.

Perhaps there are better solutions? I won't try to repeat what's already been documented quiet nicely so attaching an article here:

Soft deletion is hard

Curious about more opinions :pray:

andyw8 commented 4 years ago

There's a PR open to add this to rubocop-rails: https://github.com/rubocop-hq/rubocop-rails/pull/267

sdglhm commented 4 years ago

I also feel like there's a certain pressure when using default_scope but as @flanger001 has mentioned, I also use it when I need to reference something like either a deleted_at or an archived value.

default_scope {
  where(archived: false)
}

It's sane for me to define the default scope to return non-archived values rather than explicitly saying I need non-archived only everywhere.

gadimbaylisahil commented 4 years ago

There's a PR open to add this to rubocop-rails: rubocop-hq/rubocop-rails#267

Thank you! I would've gone ahead and open a PR in this project for the style guide but I see that the description for good and bad is already included here so, I will leave it to @fatkodima.

fatkodima commented 4 years ago

@gadimbaylisahil Please, go ahead and create a PR. I will update my linked PR with your examples and link to the style guide then 😄

gadimbaylisahil commented 4 years ago

@gadimbaylisahil Please, go ahead and create a PR. I will update my linked PR with your examples and link to the style guide then smile

Right on

gadimbaylisahil commented 4 years ago

@gadimbaylisahil Please, go ahead and create a PR. I will update my linked PR with your examples and link to the style guide then smile

Done, I kept your examples and added 2 more references in https://github.com/rubocop-hq/rails-style-guide/pull/267. Thanks for the work!

pirj commented 4 years ago

Two problems with default_scope that were not mentioned:

  1. Newly created records are affected by it:
    
    class User < ActiveRecord::Base
    default_scope { where(approved: true) }
    end

Transition.new.approved # => true


2. Depending on how `default_scope`'s conditions are defined, 1) may not work:

class User < ActiveRecord::Base default_scope { where('approved = ?', true) } end

Transition.new.approved # => false


Otherwise, I don't really see it as being problematic. The example with soft-deletions @flanger001 mentions is pretty legit, and it's not about soft-deletions only, it's also for some default `active` status of the model, which is used as a default across the application.
Can't tell off the top of my head, but I believe the following should work just fine:

class BaseUser < ActiveRecord::Base self.table_name = 'users' end class User < BaseUser default_scope { where(approved: true) } has_many :orders end

ok!

BaseUser.new.approved # => false

ok!

Order.today.merge(User.vip) # only "approved" and "vip"


So, even though there are those two culprits, the benefits of properly using `default_scope` overweigh its potential downsides. As @flanger001 said, it's a major burden to spread `approved`/`active`/not deleted all around the code, especially where the model is being used implicitly, e.g.

class Order belongs_to :user, -> { active } # only consider orders from active users by default end

pirj commented 4 years ago

Closing as stale. Thank you all for participating.