This project is archived and is in readonly mode.
named_scope can bash critical methods
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.
-
named_scope_spec.rb
365 Bytes
-
Matt Lyonlet 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 DeNataleCalm 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 KemperCould add a defined?(...) check.
-
nkallenDefinitely 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 JanowskiMaybe 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
- 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.
-
restrict_named_scope.diff
2.01 KB
-
CourtenayFix the bad test, then :) Just because it fails doesn't mean it was a good test originally.
-
Rick DeNatale
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
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 HumeI 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_methodsBut 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.
-
