Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 June 13th, 2008 @ 12:24 AM

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

• You flagged this item as spam.
  

  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 ;)

• You flagged this item as spam.
  

  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)

• You flagged this item as spam.
  

  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.

  • restrict_named_scope.diff 2 KB

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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'.

• You flagged this item as spam.
  

  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..

• You flagged this item as spam.
  

  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 July 23rd, 2008 @ 02:36 PM

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

  What nkallen said.