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

Raise an error if `scope` or `enum` is about to add a conflicting method to the class #13450

Merged
merged 3 commits into from Jan 29, 2014

Conversation

@chancancode
Copy link
Member

@chancancode chancancode commented Dec 22, 2013

See #13389 for details. I left out case 4, because that does not particularly concerns me, and I don't think checking klass.new.respond_to?(...) is a good idea.

cc @senny @dhh

@senny
Copy link
Member

@senny senny commented Dec 22, 2013

@chancancode did you consider 813c8c0 ? What can be the sign of a mistake is a feature in other situations.

As an illustration:

module Publishable
  extend ActiveSupport::Concern

  included do
    enum status: [:proposed, :written, :published]
  end

  def published!
    super
    "do publish work..."
  end
end
@schneems
Copy link
Member

@schneems schneems commented Dec 22, 2013

@senny we could do some metaprogramming to avoid that scenario using singleton_method_added like this:

class ActiveRecord; end
class ActiveRecord::Base
  class << self
    def singleton_method_added(method_name)
      puts "We could check '#{method_name}' as it is added to #{self}"
      super
    end
  end
end

class Barz < ActiveRecord::Base
  def self.any_method_you_want
  end
end

# => "We could check 'any_method_you_want' as it is added to Barz"

Though this only works if the singleton_method_added is already inherited or included, if you put the include at the bottom of the class, it wouldn't check anything.

@chancancode
Copy link
Member Author

@chancancode chancancode commented Dec 22, 2013

@senny, good catch, will update to make this order independent

@chancancode
Copy link
Member Author

@chancancode chancancode commented Dec 24, 2013

@senny updated!

@schneems
Copy link
Member

@schneems schneems commented Dec 24, 2013

Tangent: 

It would have been easier if there was an accessor method/object.

Model.enum.shipped?

Instead of the method going straight on the class:

Model.shipped?


Sent from Mailbox for iPhone

On Tue, Dec 24, 2013 at 6:22 AM, Godfrey Chan notifications@github.com
wrote:

@senny updated!

Reply to this email directly or view it on GitHub:
#13450 (comment)

@chancancode
Copy link
Member Author

@chancancode chancancode commented Dec 25, 2013

@schneems check out the discussion in #13389 :)

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 1, 2014

Seems good to me :shipit:

@rafaelfranca
rafaelfranca reviewed Jan 1, 2014
View changes
activerecord/lib/active_record/scoping/named.rb Outdated
@@ -139,6 +139,12 @@ def scope_attributes? # :nodoc:
# Article.published.featured.latest_article
# Article.featured.titles
def scope(name, body, &block)
if respond_to?(name)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 1, 2014
Member

Will this handle private methods too?

This comment has been minimized.

@chancancode

chancancode Jan 1, 2014
Author Member

Fixed via in chancancode@f350402, thanks 😁

@chancancode
Copy link
Member Author

@chancancode chancancode commented Jan 2, 2014

Update: some open points...

  1. Base.method_defined?("#{value}?") is no good (doesn't check for private methods, etc). Should use the same logic as dangerous_attribute?. Resolution: I'm going to do a small refactor to make those methods available here and reuse them.
  2. if respond_to?(value) doesn't check for private class methods etc, but by doing so it'll raise on many not-too-dangerous cases (e.g. you cannot have a scope called open because it conflicts with Kernel.open which was mixed into Class. Resolution: use a mixed blacklist approach.
@chancancode
Copy link
Member Author

@chancancode chancancode commented Jan 27, 2014

@rafaelfranca @senny Sorry for the hold up, I re-implemented this just now. The 'dangerous' logic is basically the same as attribute method (defined on Base but not on Base.superclass) for both instance and class method. The only exception is that enum instance methods conflicts with stuff added from another enum from within the same class.

This catches the most "dangerous" scenarios (conflict with AR internals) but still allows the programmer to do some potentially dangerous stuff such as re-defining a scope/enum in a subclass or override an existing user-defined class method with a scope.

I also thought about extracting some of these into Active Support, but the logic we use here is actually quite specific to AR, so I don't know how useful that would be. If you still think that's helpful I can give it a shot.

@chancancode
Copy link
Member Author

@chancancode chancancode commented Jan 27, 2014

The extraction would look roughly like this:

class Module
  def method_defined_within?(name, sup = Module)
    if method_defined?(name) || private_method_defined?(name)
      if sup.method_defined?(name) || sup.private_method_defined?(name)
        instance_method(name).owner != sup.instance_method(name).owner
      else
        true
      end
    else
      false
    end
  end

  def class_method_defined_within?(name, sup = Module)
    if respond_to?(name, true)
      if sup.respond_to?(name, true)
        method(name).owner != sup.method(name).owner
      else
        true
      end
    else
      false
    end
  end

  private
    def safe_define_method(name, base = self, sup = Module, &block)
      unless base.method_defined_within?(name, sup)
        define_method(name, &block)
      end
    end

    def safe_define_method!(name, base = self, sup = Module, &block)
      safe_define_method(name, base, sup, &block) ||
        raise ArgumentError, "Attempted to redefine the method #{name} " \
          "when it is already defined by #{base.name}."
    end

    def safe_define_class_method(name, base = self, sup = Module, &block)
      unless base.class_method_defined_within?(name, sup)
        base.singleton_class.send(:define_method, name, &block)
      end
    end

    def safe_define_class_method!(name, base = self, sup = Module, &block)
      safe_define_class_method(name, base, sup, block) ||
        raise ArgumentError, "Attempted to redefine the class method #{name} " \
          "when it is already defined by #{base.name}."
    end
end

class Class
  def method_defined_within?(name, sup = self.superclass)
    super(name, sup)
  end

  def class_method_defined_within?(name, sup = self.superclass)
    super(name, sup)
  end

  private
    def safe_define_method(name, base = self, sup = base.superclass, &block)
      super(name, base, sup, &block)
    end

    def safe_define_method!(name, base = self, sup = base.superclass, &block)
      super(name, base, sup, &block)
    end

    def safe_define_class_method(name, base = self, sup = base.superclass, &block)
      super(name, base, sup, &block)
    end

    def safe_define_class_method!(name, base = self, sup = base.superclass, &block)
      super(name, base, sup, &block)
    end
end
chancancode added 3 commits Jan 25, 2014
Before:

  >> ActiveRecord::Base.respond_to?(:find_by_something)
  NoMethodError: undefined method `abstract_class?' for Object:Class

After:

  >> ActiveRecord::Base.respond_to?(:find_by_something)
  => false
Similar to dangerous attribute methods, a scope name conflict is
dangerous if it conflicts with an existing class method defined within
`ActiveRecord::Base` but not its ancestors.

See also #13389.

*Godfrey Chan*, *Philippe Creux*
Dangerous name conflicts includes instance or class method conflicts
with methods defined within `ActiveRecord::Base` but not its ancestors,
as well as conflicts with methods generated by other enums on the same
class.

Fixes #13389.
chancancode added a commit that referenced this pull request Jan 29, 2014
Raise an error if `scope` or `enum` is about to add a conflicting method to the class

Fixed #13389
@chancancode chancancode merged commit 9653a65 into rails:master Jan 29, 2014
@seuros
Copy link
Member

@seuros seuros commented Jan 30, 2014

With this change, tables that use uuid as id and redefine :first and :last are broken now.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 30, 2014

Redefine how?

@seuros
Copy link
Member

@seuros seuros commented Jan 30, 2014

Like this
scope :first, -> { order('created_at').first }
scope :last, -> { order('created_at DESC').first }

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 30, 2014

why would you do this?

But anyway, you still can have the same behavior.

def self.first
  order('created_at').first
end

def self.last
  order('created_at DESC').first
end
@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Jan 30, 2014

In general you should not use scopes to return AR objects, scopes should return other relations. In any case you can still use class methods for that, as @rafaelfranca just pointed out.

@seuros
Copy link
Member

@seuros seuros commented Jan 30, 2014

Thanks you @rafaelfranca

@phallstrom

This comment has been minimized.

Copy link
Contributor

@phallstrom phallstrom commented on 7e8e91c May 8, 2014

Might be worth noting that dangerous methods also include any that other libraries inject into AR as well. In my case, the ransack gem aliases 'search' to one of it's own methods which caused my 'search' scope to fail.

This comment has been minimized.

Copy link
Member Author

@chancancode chancancode replied May 8, 2014

That's an interesting scenario =/ However, I think the same logic would apply regardless of the source. For instance, if ransack calls that #search method internally, it'd stop working correctly if you have a "search" scope on the same class.

This comment has been minimized.

Copy link
Contributor

@phallstrom phallstrom replied May 8, 2014

@chancancode Absolutely it would. From looking at Ransack's source though I think they do it mostly as a convenience to their users since they only do it if it's not already defined.

https://github.com/activerecord-hackery/ransack/blob/master/lib/ransack/adapters/active_record/base.rb#L7

I don't think Rails needs to change anything other than perhaps mention "hey if it's breaking and you don't see it in AR, check your gems"

This comment has been minimized.

Copy link
Member Author

@chancancode chancancode replied May 8, 2014

@phallstrom makes sense, if you have suggestions for the wordings of the message, feel free to suggest it here, or better yet open a PR 😄

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.