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

Avoiding ActiveRecord default_scope #266

Closed
gadimbaylisahil opened this issue Jul 4, 2020 · 10 comments
Closed

Avoiding ActiveRecord default_scope #266

gadimbaylisahil opened this issue Jul 4, 2020 · 10 comments

Comments

@gadimbaylisahil
Copy link

gadimbaylisahil commented Jul 4, 2020

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: #267
Associated PR in rubocop-hq/rubocop-rails: rubocop/rubocop-rails#267

@flanger001
Copy link

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
Copy link
Author

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 🙏

@andyw8
Copy link
Contributor

andyw8 commented Jul 5, 2020

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

@sdglhm
Copy link

sdglhm commented Jul 5, 2020

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Author

@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 #267. Thanks for the work!

@pirj
Copy link
Member

pirj commented Jul 6, 2020

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
  1. 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
Copy link
Member

pirj commented Sep 3, 2020

Closing as stale. Thank you all for participating.

@pirj pirj closed this as completed Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants