[PATCH] Prevent use of ActiveRecord method names as association or column names

Patch to prevent reusing existing method name in association declaration

François, the patch looks good! Special thanks for the unit tests :)

Refactored for post-[3213]

I just updated the patch. I tested on MySQL, and all 672 tests pass.

Updated to reflect mail thread

After discussion on rails-core: http://thread.gmane.org/gmane.comp.lang.ruby.rails.core/41

Updated the patch to reflect the last part of the discussion.

A much better solution would be to simply rename "quote" to "quote_string" or simliar... or another name less likely to conflict with a top-level modem (Quote thru an association was my issue - why I'm here).

This won't solve every problem but it seems Rails should make an attempt to avoid short function names that would conflict with obvious real life models...

I can make this patch, is someone will apply it...

I read the thread... but I'm not sure if the following is dealt with.

If we just can't rename a few things then ./script/generate model or add_column, etc... all of those various aspects of the Rails code need to throw the appropriate errors... but again, I think you'd want to avoid this where possible.

This doesn't allow subclasses to override reflections in the parent class.

Hi! Could someone help me understand the resolution of this ticket? It says it's "untested", but that seems more like an "unresolution" to me than a resolution! I don't know what it means*.

(*So is there a page on the Rails Trac explaining what the different resolutions mean? Especially "untested" and "undocumented" — I have a hard time understanding how they could be considered resolutions... The best docs I could find on this were http://trac.edgewall.org/wiki/TracTicketTriage and http://www.mozilla.org/quality/help/bugzilla-privilege-guide.html , but neither mentions anything about an "untested" resolution... Feel free to point me to a newbie Rails Trac FAQ if these are newbie questions that have already been answered... Thanks.)

Perhaps this ticket was closed prematurely?

It doesn't look like the patch ever got applied, so I just wanted to know if there were any plans regarding that...

Is this patch also supposed to also solve the problem described in #3628, where "quote"* cannot be used as the name of a column? It looks like this patch only deals with associations/reflections, not with "plain old" column names!

(*Yes, I know quote was recently renamed to quote_value, but there's still a problem if anyone tries to use quote_value or some other AR method name as a column name...)

Some comments from http://thread.gmane.org/gmane.comp.lang.ruby.rails.core/41 make it look like solving this for column names might have been considered but somehow didn't make it into the patch (??)...

François Beausoleil:

./script/../config/../vendor/rails/railties/lib/commands/runner.rb:27:
Columns 'name', 'transaction', 'quote' have already been taken in
models of class Game.  You will have to rename your columns to not
conflict with pre-existing method names.
(ActiveRecord::ColumnNameAlreadyTaken)

When I used name in my models, I never expected that to conflict. It's such a common one. Guess what it conflicts with: Class::name. Here's the implementation of the guard method:

# Raise ActiveRecord::ColumnNameAlreadyTaken if one of the columns defined
# in objects of this class conflict with pre-existing method names.
def guard_against_duplicate_column_names
  # We cast a very wide net here, maybe we should not cast such as wide one ?
  methods = [ ActiveRecord::Base.methods,
              ActiveRecord::Base.instance_methods].flatten.compact.uniq
  duplicates = self.class.columns.select do |column|
    next false if column.name == self.class.primary_key
    methods.include?(column.name)
  end

  raise ActiveRecord::ColumnNameAlreadyTaken.new(duplicates,
self.class) unless duplicates.empty?
end

Sounds good to me!

Francois Beausoleil writes:

This patch does not handle the case where a method declaration shadows a column in the model. This is left as an exercise to the reader.

http://article.gmane.org/gmane.comp.lang.ruby.rails.core/68

So, what do we do ? We could also take the status quo, and just add the patch as-is, meaning relationships would prevent reusing existing method names, but columns are not.

Why not for columns too?

http://article.gmane.org/gmane.comp.lang.ruby.rails.core/60

Dave did say:

We sidestep the issue. Reserved words are the reason why you can do attribute access with brackets, ala: model[:attr]

Of course that can be done. In that case, should auto read/write methods be forbidden for such columns ? In the end, we're trying to keep the principle of least surprise active. What's less suprising: ...

I think François has a good idea there — "should auto read/write methods be forbidden for such columns?". Couldn't we just change define_read_methods to throw an error or something if you try to create a column with the same name as a reserved ActiveRecord method ?

http://article.gmane.org/gmane.comp.lang.ruby.rails.core/115

What if the patch were the 'full bonanza', combining both of the earlier ones, with a special exception for the 'type' column, since that one would be a hassle, and is going away soon via a Ruby update anyway? Even better, these checks could also be added to the generator script. Is that too draconian? Personally, I think this can be a source of very frustrating bugs to Rails newcomers, and it would be better to err on the side of caution.

I agree, personally (What am I missing?)

Q regarding "generator script": Doesn't that only generate models? How would it check if you were using an invalid column/association name?

Replying to dreamer3@gmail.com:

I read the thread... but I'm not sure if the following is dealt with. If we just can't rename a few things then ./script/generate model or add_column, etc... all of those various aspects of the Rails code need to throw the appropriate errors... but again, I think you'd want to avoid this where possible.

Why would we want to avoid that? I think that's a great idea (If I'm understanding correctly...)! If we can preemptively prevent people from creating columns like "quote" then there is less of a need for run-time checks/errors to handle the results of having these "bad" column names...