Fixed that records returned from has_and_belongs_to_many associations with additional attributes should be marked as read only (fixes #4512) [DHH] DEPRECATED: Using additional attributes on has_and_belongs_to_many associations. Instead upgrade your association to be a real join model [DHH]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4123 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

3 files changed, 15 insertions, 10 deletions

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 7c60ab8fd0..3ad0eeecc7 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -698,11 +698,10 @@ module ActiveRecord
       # an option, it is guessed using the lexical order of the class names. So a join between Developer and Project
       # will give the default join table name of "developers_projects" because "D" outranks "P".
       #
-      # Any additional fields added to the join table will be placed as attributes when pulling records out through
-      # has_and_belongs_to_many associations. This is helpful when have information about the association itself
-      # that you want available on retrieval. Note that any fields in the join table will override matching field names
-      # in the two joined tables. As a consequence, having an "id" field in the join table usually has the undesirable
-      # result of clobbering the "id" fields in either of the other two tables.
+      # Deprecated: Any additional fields added to the join table will be placed as attributes when pulling records out through
+      # has_and_belongs_to_many associations. Records returned from join tables with additional attributes will be marked as
+      # ReadOnly (because we can't save changes to the additional attrbutes). It's strongly recommended that you upgrade any
+      # associations with attributes to a real join model (see introduction).
       #
       # Adds the following methods for retrieval and query.
       # +collection+ is replaced with the symbol passed as the first argument, so 
@@ -714,7 +713,7 @@ module ActiveRecord
       # * <tt>collection.push_with_attributes(object, join_attributes)</tt> - adds one to the collection by creating an association in the join table that
       #   also holds the attributes from <tt>join_attributes</tt> (should be a hash with the column names as keys). This can be used to have additional
       #   attributes on the join, which will be injected into the associated objects when they are retrieved through the collection.
-      #   (collection.concat_with_attributes is an alias to this method).
+      #   (collection.concat_with_attributes is an alias to this method). This method is now deprecated.
       # * <tt>collection.delete(object, ...)</tt> - removes one or more objects from the collection by removing their associations from the join table.  
       #   This does not destroy the objects.
       # * <tt>collection=objects</tt> - replaces the collections content by deleting and adding objects as appropriate.
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index 3f6fdb6bac..cd866d2cdd 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -40,8 +40,8 @@ module ActiveRecord
           end
 
           options[:conditions] = conditions
-          options[:joins] = @join_sql
-          options[:readonly] ||= !options[:joins].nil?
+          options[:joins]      = @join_sql
+          options[:readonly]   = finding_with_ambigious_select?(options[:select])
 
           if options[:order] && @reflection.options[:order]
             options[:order] = "#{options[:order]}, #{@reflection.options[:order]}"
@@ -157,7 +157,13 @@ module ActiveRecord
 
           @join_sql = "INNER JOIN #{@reflection.options[:join_table]} ON #{@reflection.klass.table_name}.#{@reflection.klass.primary_key} = #{@reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
         end
-        
+
+        # Join tables with additional columns on top of the two foreign keys must be considered ambigious unless a select
+        # clause has been explicitly defined. Otherwise you can get broken records back, if, say, the join column also has
+        # and id column, which will then overwrite the id column of the records coming back.
+        def finding_with_ambigious_select?(select_clause)
+          !select_clause && @owner.connection.columns(@reflection.options[:join_table], "Join Table Columns").size != 2
+        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 1013251b0a..8227cbe89d 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -850,7 +850,7 @@ module ActiveRecord #:nodoc:
                   (hash[method].keys + params.keys).uniq.each do |key|
                     merge = hash[method][key] && params[key] # merge if both scopes have the same key
                     if key == :conditions && merge
-                      hash[method][key] = [params[key], hash[method][key]].collect{|sql| "( %s )" % sanitize_sql(sql)}.join(" AND ")
+                      hash[method][key] = [params[key], hash[method][key]].collect{ |sql| "( %s )" % sanitize_sql(sql) }.join(" AND ")
                     elsif key == :include && merge
                       hash[method][key] = merge_includes(hash[method][key], params[key]).uniq
                     else