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

Bring back private class methods accessibility in named scope #32355

Merged
merged 2 commits into from
Apr 6, 2018

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Mar 27, 2018

The receiver in a scope was changed from klass to relation itself
for all scopes (named scope, default_scope, and association scope)
behaves consistently in #29301.

In addition. Before 5.2, if both an AR model class and a Relation
instance have same named methods (e.g. arel_attribute,
predicate_builder, etc), named scope doesn't respect relation instance
information.

For example:

class Post < ActiveRecord::Base
  has_many :comments1, class_name: "RecentComment1"
  has_many :comments2, class_name: "RecentComment2"
end

class RecentComment1 < ActiveRecord::Base
  self.table_name = "comments"
  default_scope { where(arel_attribute(:created_at).gteq(2.weeks.ago)) }
end

class RecentComment2 < ActiveRecord::Base
  self.table_name = "comments"
  default_scope { recent_updated }
  scope :recent_updated, -> { where(arel_attribute(:updated_at).gteq(2.weeks.ago)) }
end

If eager loading Post.eager_load(:comments1, :comments2).to_a,
:comments1 (default_scope) respects aliased table name, but
:comments2 (using named scope) may not work correctly since named
scope doesn't respect relation instance information. See also 801ccab.

But this is a breaking change between releases without deprecation.
I decided to bring back private class methods accessibility in named
scope.

Fixes #31740.
Fixes #32331.

The receiver in a scope was changed from `klass` to `relation` itself
for all scopes (named scope, default_scope, and association scope)
behaves consistently.

In addition. Before 5.2, if both an AR model class and a Relation
instance have same named methods (e.g. `arel_attribute`,
`predicate_builder`, etc), named scope doesn't respect relation instance
information.

For example:

```ruby
class Post < ActiveRecord::Base
  has_many :comments1, class_name: "RecentComment1"
  has_many :comments2, class_name: "RecentComment2"
end

class RecentComment1 < ActiveRecord::Base
  self.table_name = "comments"
  default_scope { where(arel_attribute(:created_at).gteq(2.weeks.ago)) }
end

class RecentComment2 < ActiveRecord::Base
  self.table_name = "comments"
  default_scope { recent_updated }
  scope :recent_updated, -> { where(arel_attribute(:updated_at).gteq(2.weeks.ago)) }
end
```

If eager loading `Post.eager_load(:comments1, :comments2).to_a`,
`:comments1` (default_scope) respects aliased table name, but
`:comments2` (using named scope) may not work correctly since named
scope doesn't respect relation instance information. See also 801ccab.

But this is a breaking change between releases without deprecation.
I decided to bring back private class methods accessibility in named
scope.

Fixes rails#31740.
Fixes rails#32331.
@matthewd
Copy link
Member

Doesn't this [sometimes] make private class methods accessible as public methods on relations? Was that true before? 🤔

@matthewd
Copy link
Member

If a deprecation-driven change is impractical, we could also consider just introducing a detailed custom exception, or reverting to the (yes, differently-broken) 5.1 behaviour and then making the change in 6.0.

@rafaelfranca
Copy link
Member

I like @matthewd's plan

@rafaelfranca rafaelfranca added this to the 5.2.0 milestone Mar 27, 2018
@kamipo kamipo force-pushed the delegate_to_klass_in_a_scope branch from 2c4d5aa to 0bfeb48 Compare March 30, 2018 00:57
@kamipo
Copy link
Member Author

kamipo commented Mar 30, 2018

How about the deprecation 0bfeb48?
I would like to keep the change #29301 if possible, since #32380 (fix for #14003) is based on the change, and more improving/fixing alias tracking for joins/eager_loading in 6.0.

@rafaelfranca rafaelfranca merged commit 7fac2b4 into rails:master Apr 6, 2018
rafaelfranca added a commit that referenced this pull request Apr 6, 2018
Bring back private class methods accessibility in named scope
rafaelfranca added a commit that referenced this pull request Apr 6, 2018
Bring back private class methods accessibility in named scope
@kamipo kamipo deleted the delegate_to_klass_in_a_scope branch April 7, 2018 01:11
@speckins
Copy link
Contributor

speckins commented Jul 5, 2018

I hit this deprecation notice while upgrading. I have a few models where I had extracted some repeated ARel logic, like a complex condition or correlated subquery, into private class methods for re-use across multiple scopes, which at the time I understood to be equivalent to class methods (as stated by the documentation, "...[scope] is simply 'syntactic sugar' for defining an actual class method").

For example:

  class << self
    private
    def _open
      today = Date.today
      end_date = arel_attribute(:submission_end_date)

      arel_attribute(:submission_begin_date).lteq(today).and(end_date.gteq(today).or(end_date.eq(nil)))
    end
  end

  scope :currently_open, -> { where _open }

  scope :currently_open_first, -> {
    order Arel::Nodes::Descending.new(_open), arel_attribute(:submission_begin_date).desc
  }

After reading through the source code for named scopes and relation delegation, it seems that the proc passed to ::scope is instance_exec'd in the context of an instance of a class-specific subclass of Relation, AssociationRelation, or Associations::CollectionProxy.

Do you have a suggestion on how to retain this pattern of sharing code between scopes -- preferably with private visibility -- with this change of receiver?

I thought of redefining my scopes as actual class methods or singleton_class methods, so they would have access to private class methods, but it seems like that might break scopes in cases where table aliases(?) are involved.

What about wrapping an explicit call to a private class method in scoping?

  scope :currently_open, -> {  where scoping { @klass.send(:_open) } }

EDIT: Ended up doing the above in places where I wanted to retain shared code with private visibility between scopes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants