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

activerecord/CHANGELOG

@@ -1,5 +1,9 @@
 *SVN*
 
+* DEPRECATED: Using additional attributes on has_and_belongs_to_many associations. Instead upgrade your association to be a real join model [DHH]
+
+* Fixed that records returned from has_and_belongs_to_many associations with additional attributes should be marked as read only (fixes #4512) [DHH]
+
 * Do not implicitly mark recordss of has_many :through as readonly but do mark habtm records as readonly (eventually only on join tables without rich attributes). [Marcel Mollina Jr.]
 
 * Fixed broken OCIAdapter #4457 [schoenm@earthlink.net]

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.

activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb

@@ -41,7 +41,7 @@ module ActiveRecord
 
           options[:conditions] = conditions
           options[:joins]      = @join_sql
-          options[:readonly] ||= !options[:joins].nil?
+          options[:readonly]   = finding_with_ambigious_select?(options[:select])
 
           if options[:order] && @reflection.options[:order]
             options[:order] = "#{options[:order]}, #{@reflection.options[:order]}"
@@ -158,6 +158,12 @@ 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

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

activerecord/test/associations_test.rb

@@ -7,6 +7,7 @@ require 'fixtures/reply'
 require 'fixtures/computer'
 require 'fixtures/customer'
 require 'fixtures/order'
+require 'fixtures/category'
 require 'fixtures/post'
 require 'fixtures/author'
 
@@ -1080,7 +1081,7 @@ end
 
 
 class HasAndBelongsToManyAssociationsTest < Test::Unit::TestCase
-  fixtures :accounts, :companies, :developers, :projects, :developers_projects
+  fixtures :accounts, :companies, :categories, :posts, :categories_posts, :developers, :projects, :developers_projects
   
   def test_has_and_belongs_to_many
     david = Developer.find(1)
@@ -1470,6 +1471,25 @@ class HasAndBelongsToManyAssociationsTest < Test::Unit::TestCase
     end_sql
   end
   
+  def test_updating_attributes_on_non_rich_associations
+    welcome = categories(:technology).posts.first
+    welcome.title = "Something else"
+    assert welcome.save!
+  end
+  
+  def test_updating_attributes_on_rich_associations
+    david = projects(:action_controller).developers.first
+    david.name = "DHH"
+    assert_raises(ActiveRecord::ReadOnlyRecord) { david.save! }
+  end
+
+  
+  def test_updating_attributes_on_rich_associations_with_limited_find
+    david = projects(:action_controller).developers.find(:all, :select => "developers.*").first
+    david.name = "DHH"
+    assert david.save!
+  end
+
   def test_join_table_alias
     assert_equal 3, Developer.find(:all, :include => {:projects => :developers}, :conditions => 'developers_projects_join.joined_on IS NOT NULL').size
   end