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

The receiver in a scope should be a `relation` #29301

Merged
merged 1 commit into from Jun 28, 2017

Conversation

Projects
None yet
3 participants
@kamipo
Member

kamipo commented May 31, 2017

Currently the receiver in a scope is klass, not relation.

https://github.com/rails/rails/blob/v5.1.1/activerecord/lib/active_record/scoping/named.rb#L159

I think it is a strange because the receiver in default_scope and a
scope on association is relation.

https://github.com/rails/rails/blob/v5.1.1/activerecord/lib/active_record/scoping/default.rb#L119
https://github.com/rails/rails/blob/v5.1.1/activerecord/lib/active_record/associations/association_scope.rb#L164

I fixed to the receiver is to be a relation properly for consistency.

The receiver in a scope should be a `relation`
Currently the receiver in a scope is `klass`, not `relation`.
I think it is a strange because the receiver in `default_scope` and a
scope on association is `relation`.
I fixed to the receiver is to be a `relation` properly for consistency.
@matthewd

This comment has been minimized.

Member

matthewd commented Jun 1, 2017

Dropping the scoping seems like a breaking change, but I can't think of a way it actually would be: if you're calling the scope on a relation instance, it will already be applying a scope; if you're calling it on the class directly, there's no scope to apply.

Changing the receiver is definitely noticeable if someone's looking for it, but doesn't feel problematic: the relation forwards unknown methods to the class anyway.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jun 28, 2017

I'm going to merge this PR so we get feedback if it is a breaking change in the beta. Like you, I can't think of a way that this would cause a breaking change.

@rafaelfranca rafaelfranca merged commit 65b02ea into rails:master Jun 28, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kamipo kamipo deleted the kamipo:receiver_in_scope_should_be_relation branch Jun 28, 2017

kamipo added a commit that referenced this pull request Sep 27, 2017

Add test case for `arel_attribute` with a custom table
Since #29301, `arel_attribute` respects a custom table name.
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Dec 20, 2017

Just found a case where it breaks:

def class Foo < ApplicationRecord
  scope(:bar), ->() do
    x = sanitize_sql("SOME_SQL")
    select(x)
  end
end

Now sanitze_sql will not be found.

@kamipo

This comment has been minimized.

Member

kamipo commented Dec 21, 2017

This should be addressed by #31392.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Dec 21, 2017

That is true 👍

stevenharman added a commit to stevenharman/geocoder that referenced this pull request Feb 27, 2018

Make #near_scope_options public for Rails 5.2
Rails 5.2 changed the receiver within a scope from `klass` to
`relation`. With that, we no longer have private access to
Geocoder::Store::ActiveRecord#near_scope_options, and so must make it
public.

see: rails/rails#29301

kamipo added a commit to kamipo/rails that referenced this pull request Mar 30, 2018

Chaining named scope is no longer leaking to querying class methods
Previously `scoping` is used to delegate to named scope methods from
relations for propagating the chaining source scope. But it was caused
undesired behavior that pollute all querying class methods.

Example:

```ruby
class Topic < ActiveRecord::Base
  scope :toplevel, -> { where(parent_id: nil) }
  scope :children, -> { where.not(parent_id: nil) }
  scope :has_children, -> { where(id: Topic.children.select(:parent_id)) }
end

# Works as expected.
Topic.toplevel.where(id: Topic.children.select(:parent_id))

# Doesn't work due to leaking `toplevel` to `Topic.children`.
Topic.toplevel.has_children
```

Since rails#29301, the receiver in named scopes has changed from model class
to the chaining source scope, so the polluting querying class methods is
no longer required with that.

Fixes rails#14003.

kamipo added a commit to kamipo/rails that referenced this pull request Jul 19, 2018

Avoid extra scoping in delegating to klass methods in the `scope` block
Since rails#29301, delegating to klass methods in the `scope` block would
cause extra scoping by the receiver itself. The extra scoping would
always override intermediate scoping like `unscoped` and caused the
regression rails#33387. To keep the original scoping behavior, should avoid
the extra scoping in the `scope` block.

Fixes rails#33387.

kamipo added a commit to kamipo/rails that referenced this pull request Oct 8, 2018

Generate delegation methods to named scope in the definition time
The delegation methods to named scope are defined when `method_missing`
is invoked on the relation.

Since rails#29301, the receiver in the named scope is changed to the relation
like others (e.g. `default_scope`, etc) for consistency.

Most named scopes would be delegated from relation by `method_missing`,
since we don't allow scopes to be defined which conflict with instance
methods on `Relation` (rails#31179). But if a named scope is defined with the
same name as any method on the `superclass` (e.g. `Kernel.open`), the
`method_missing` on the relation is not invoked.

To address the issue, make the delegation methods to named scope is
generated in the definition time.

Fixes rails#34098.

kamipo added a commit to kamipo/rails that referenced this pull request Oct 8, 2018

Generate delegation methods to named scope in the definition time
The delegation methods to named scope are defined when `method_missing`
is invoked on the relation.

Since rails#29301, the receiver in the named scope is changed to the relation
like others (e.g. `default_scope`, etc) for consistency.

Most named scopes would be delegated from relation by `method_missing`,
since we don't allow scopes to be defined which conflict with instance
methods on `Relation` (rails#31179). But if a named scope is defined with the
same name as any method on the `superclass` (e.g. `Kernel.open`), the
`method_missing` on the relation is not invoked.

To address the issue, make the delegation methods to named scope is
generated in the definition time.

Fixes rails#34098.

kamipo added a commit to kamipo/rails that referenced this pull request Oct 9, 2018

Generate delegation methods to named scope in the definition time
The delegation methods to named scope are defined when `method_missing`
is invoked on the relation.

Since rails#29301, the receiver in the named scope is changed to the relation
like others (e.g. `default_scope`, etc) for consistency.

Most named scopes would be delegated from relation by `method_missing`,
since we don't allow scopes to be defined which conflict with instance
methods on `Relation` (rails#31179). But if a named scope is defined with the
same name as any method on the `superclass` (e.g. `Kernel.open`), the
`method_missing` on the relation is not invoked.

To address the issue, make the delegation methods to named scope is
generated in the definition time.

Fixes rails#34098.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment