This project is archived and is in readonly mode.

#404 ✓ wontfix
Rick DeNatale

named_scope can bash critical methods

Reported by Rick DeNatale | June 12th, 2008 @ 09:36 PM

ActiveRecord::NamedScope::ClassMethods#named_scope when called, defines a new class method when it is called, which has the same name as the first argument. This is exactly the same code as is in the has_finder method in the has_finder gem with the appropriate method and variable name changes.

Unfortunately the method makes no check on the name. Our app uses has_finder with the call

has_finder :public, ...

We ran into a problem with RSpec, when I stubbed a method on one of these active record instances, the stub! method failed because call was being sent to nil. I finally tracked the point of failure to the line equivalent to line 95 in activerecord/lib/active_record/named_scope.rb

84: def named_scope(name, options = {}, &block)

85: scopes[name] = lambda do |parent_scope, *args|

86: Scope.new(parent_scope, case options

87: when Hash

88: options

89: when Proc

90: options.call(*args)

91: end, &block)

92: end

93: (class << self; self end).instance_eval do

95: define_method name do |*args|

96: scopes[name].call(self, *args)

97: end

98: end

99: end

What appears to be happening is that RSpec is using class_eval to define a method on the model class in processing a stub! call, and the method public is being called internally by ruby to make the method public, but the public method defined by has_finder/named_scope is called instead and scopes[:public] is nil.

I've proved that this is what is happening with a simple attached spec file, which doesn't use stub but simply shows that after using named_scope the public method in class is ineffective in making newly defined methods public.

I haven't written a patch because I'm not sure what the fix should be.

Comments and changes to this ticket

  • Matt Lyon

    Matt Lyon June 12th, 2008 @ 10:20 PM

    let me get this straight.... it's a bug because you told a meta-method to overwrite 'public'? So are you going to bitch about how "named_scope :find" is screwing you up too?

    if anything, the real bug here is that it's not bitching about you overwriting something you shouldn't be overwriting.

  • Rick DeNatale

    Rick DeNatale June 12th, 2008 @ 10:33 PM

    Calm down. I said that I didn't know what the proper disposition of this should be.

    But.

    It probably should warn or prevent giving a named_scope a name which interferes with Ruby or Rails. It's unfortunate that our app used public, and that at this time, the name is pretty engrained in the app. I didn't make that decision or write the code, I just was the one who discovered the problem late in the game.

  • Jeremy Kemper

    Jeremy Kemper June 13th, 2008 @ 12:24 AM

    Could add a defined?(...) check.

  • nkallen

    nkallen June 13th, 2008 @ 01:16 AM

    Definitely not a bug, but a warning or documentation may be helpful. I personally wouldn't add the warning, but that's because I have a certain minimalist aesthetic toward code. Elsewhere in ActiveRecord there are lots of rich warnings/exceptions for when people do silly things, so it is consistent with the AR to add warnings here. It sort of depends how defensive you want to be; there are an infinite number of ways to use library code in a way that is counter-productive. Have to make a judgement call about how much time we'll save end users vs. extra entropy in the codebase. In other words, I support the decision whatever it is ;)

  • Damian Janowski

    Damian Janowski June 13th, 2008 @ 03:57 PM

    Maybe protected method names should be available with an indirection?

    For example:

    Foo.find(:public)

    This should apply to some Ruby methods and all class methods already defined in AR::Base.

    (BTW, isn't the approach above worth adding? It's good to be able to define a named_scope :all and get, for example, default sorting on a model. Then Foo.find(:all) just works)

  • Rick DeNatale

    Rick DeNatale June 13th, 2008 @ 06:07 PM

    • Title changed from “named_scope bashes critical methods” to “named_scope can bash critical methods”

    First, I've edited the title of this ticket to more accurately reflect the issue.

    I'm for adding an exception if someone tries to use named_scope with a name that matches an existing class method.

    I our case the code was written by a very experienced ruby/rails developer, and he missed the subtlety that it was defining a class method which conflicts with the public directive. We only discovered the issue when I started converting over from RSpec 1.1.3 to 1.1.4 which exposed the issue, but I then proved that it is an issue with the file I attached.

    I suspect that others might also miss this subtlety and other plugins or tools which use similar metaprogramming techniques will produce other hard to debug problems.

    I'm attaching a patch, which restricts any methods of Class. I originally tried to restrict any methods already defined in the receiving class of named_scope, but the Active records failed because at least one named scope used in the test cases is :base, and that seems to be a method.

    The methods restricted are:

    "<", "<=", "<=>", "==", "===", "=~", ">", ">=", "Array", "BigDecimal", "DelegateClass", "Float", "Integer", "Pathname", "Rational", "String", "__id__", "__send__", "`", "abort", "active_gem_with_options", "acts_like?", "alias_attribute", "alias_method", "alias_method_chain", "allocate", "ancestors", "as_load_path", "at_exit", "attr", "attr_accessor", "attr_accessor_with_default", "attr_internal", "attr_internal_accessor", "attr_internal_ivar_name", "attr_internal_naming_format", "attr_internal_naming_format=", "attr_internal_reader", "attr_internal_writer", "attr_reader", "attr_writer", "autoload", "autoload?", "b64encode", "binding", "blank?", "blank_slate_method_added", "blankslate_original_append_features", "block_given?", "breakpoint", "callcc", "caller", "catch", "cattr_accessor", "cattr_reader", "cattr_writer", "chomp", "chomp!", "chop", "chop!", "class", "class_eval", "class_inheritable_accessor", "class_inheritable_array", "class_inheritable_array_writer", "class_inheritable_hash", "class_inheritable_hash_writer", "class_inheritable_reader", "class_inheritable_writer", "class_variable_defined?", "class_variable_get", "class_variable_set", "class_variables", "clone", "const_defined?", "const_get", "const_missing", "const_set", "constants", "copy_instance_variables_from", "daemonize", "dclone", "debugger", "decode64", "decode_b", "define_method", "delegate", "deprecate", "deprecated_method_warning", "deprecation_horizon", "display", "dup", "duplicable?", "enable_warnings", "encode64", "enum_for", "eql?", "equal?", "eval", "exec", "exit", "exit!", "extend", "extend_with_included_modules_from", "extended", "extended_by", "fail", "find_hidden_method", "fork", "format", "freeze", "frozen?", "gem", "gem_original_require", "getc", "gets", "global_variables", "gsub", "gsub!", "hash", "id", "include", "include?", "included", "included_in_classes", "included_modules", "inheritable_attributes", "inherited", "inherited_with_inheritable_attributes", "inherited_without_inheritable_attributes", "initialize", "initialize_copy", "inspect", "instance_eval", "instance_exec", "instance_method", "instance_methods", "instance_of?", "instance_values", "instance_variable_defined?", "instance_variable_get", "instance_variable_names", "instance_variable_set", "instance_variables", "is_a?", "iterator?", "kind_of?", "lambda", "load", "load_without_new_constant_marking", "local_constant_names", "local_constants", "local_variables", "location_of_caller", "loop", "mattr_accessor", "mattr_reader", "mattr_writer", "method", "method_added", "method_defined?", "method_missing", "method_removed", "method_undefined", "methods", "model_name", "module_eval", "name", "nesting", "new", "nil?", "object_id", "open", "p", "parent", "parents", "present?", "print", "printf", "private", "private_class_method", "private_instance_methods", "private_method_defined?", "private_methods", "proc", "protected", "protected_instance_methods", "protected_method_defined?", "protected_methods", "public", "public_class_method", "public_instance_methods", "public_method_defined?", "public_methods", "putc", "puts", "rails_original_const_missing", "raise", "rand", "read_inheritable_attribute", "readline", "readlines", "remove_class", "remove_class_variable", "remove_const", "remove_instance_variable", "remove_method", "remove_subclasses", "remove_subclasses_of", "require", "require_association", "require_dependency", "require_gem", "require_library_or_gem", "require_or_load", "reset_inheritable_attributes", "respond_to?", "returning", "scan", "select", "send", "send!", "set_trace_func", "silence_stderr", "silence_stream", "silence_warnings", "singleton_method_added", "singleton_method_removed", "singleton_method_undefined", "singleton_methods", "sleep", "split", "sprintf", "srand", "sub", "sub!", "subclasses", "subclasses_of", "superclass", "superclass_delegating_accessor", "superclass_delegating_reader", "superclass_delegating_writer", "suppress", "syscall", "system", "taguri", "taguri=", "taint", "tainted?", "test", "throw", "timeout", "to_a", "to_enum", "to_json", "to_param", "to_query", "to_s", "to_yaml", "to_yaml_properties", "to_yaml_style", "trace_var", "trap", "type", "undef_method", "unloadable", "untaint", "untrace_var", "warn", "with_options", "write_inheritable_array", "write_inheritable_attribute", "write_inheritable_hash", "y", "yaml_as", "yaml_tag_class_name", "yaml_tag_read_class", and "yaml_tag_subclasses?"

    So the patch is a staw man.

  • Courtenay

    Courtenay June 13th, 2008 @ 07:03 PM

    Fix the bad test, then :) Just because it fails doesn't mean it was a good test originally.

  • Rick DeNatale

    Rick DeNatale June 13th, 2008 @ 08:16 PM

    Courtenay,

    I'm not sure to which test you are referring.

    The problem I had was not that the test was failing, but that misuse of named_scope had so borked Ruby so as to make class_eval defining a new method fail.

    The spec just exposed a rather critical 'timebomb'.

  • Courtenay

    Courtenay June 13th, 2008 @ 09:11 PM

    ha, no i mean... if you patch it to check for defined? before allowing a named scope, and one of the core rails tests breaks, then fix that test..

  • John Hume

    John Hume June 14th, 2008 @ 04:24 AM

    I don't think that patch is doing quite the right thing. To get the list of methods to warn about, it does this.

    RestrictedMethods = Class.methods + Class.private_methods + Class.protected_methods
    

    But that's going to give you all the methods that the class Class responds to, as opposed to all the methods that an instance of Class that inherits from Object would have. (It turns out the only difference is that it includes the method "nesting," but it's still in principle just not the list ActiveRecord ought to be warning about.)

    It seems like the restricted methods should be pulled from ActiveRecord::Base.

  • Pratik

    Pratik July 23rd, 2008 @ 02:36 PM

    • State changed from “new” to “wontfix”
    • Tag set to activerecord

    What nkallen said.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Attachments

Pages