has_many through inserts null primary_key with accepts_nested_attributes_for

[griffithchaffee]

A change between v4.1.1 and v4.1.2 causes a has_many through association to be inserted with a null primary_key when using the "association_ids=" setter. This only happens when accepts_nested_attributes_for is used (might have something to do with autosave callbacks).

This is a problem when the database uses "null false" restrictions on the primary_key. Is this expected behavior and if so is there a fix?

Models

class Parent < ActiveRecord::Base
  has_many :student_parents
  has_many :students, through: :student_parents
end
class StudentParent < ActiveRecord::Base
  belongs_to :parent
  belongs_to :student
end
class Student < ActiveRecord::Base
  has_many :student_parents
  has_many :parents, through: :student_parents
  accepts_nested_attributes_for :student_parents, allow_destroy: true
end

Queries

# v4.1.1
Student.create(parent_ids [1])
  SQL (0.6ms)  INSERT INTO "students" ("created_at", "updated_at") VALUES (?, ?)  [["created_at", "2014-09-22 20:28:08.063974"], ["updated_at", "2014-09-22 20:28:08.063974"]]
  SQL (0.2ms)  INSERT INTO "student_parents" ("created_at", "parent_id", "student_id", "updated_at") VALUES (?, ?, ?, ?)  [["created_at", "2014-09-22 20:28:08.068030"], ["parent_id", 1], ["student_id", 11], ["updated_at", "2014-09-22 20:28:08.068030"]]

# v4.1.2
Student.create(parent_ids [1])
  SQL (0.6ms)  INSERT INTO "students" ("created_at", "updated_at") VALUES (?, ?)  [["created_at", "2014-09-22 20:47:49.825984"], ["updated_at", "2014-09-22 20:47:49.825984"]]
  SQL (0.1ms)  INSERT INTO "student_parents" ("created_at", "parent_id", "updated_at") VALUES (?, ?, ?)  [["created_at", "2014-09-22 20:47:49.831091"], ["parent_id", 1], ["updated_at", "2014-09-22 20:47:49.831091"]]
  # non-null restriction error would trigger here since student_id is null
  SQL (0.1ms)  UPDATE "student_parents" SET "student_id" = ?, "updated_at" = ? WHERE "student_parents"."id" = 1  [["student_id", 12], ["updated_at", "2014-09-22 20:47:49.832819"]]

Migration

class CreateStudents < ActiveRecord::Migration
  def change
    create_table :students do |t|
      t.timestamps
    end
  end
end
class CreateParents < ActiveRecord::Migration
  def change
    create_table :parents do |t|
      t.timestamps
    end
  end
end
class CreateStudentParents < ActiveRecord::Migration
  def change
    create_table :student_parents do |t|
      # uncommenting null: false will cause non-null restriction errors
      t.belongs_to :student#, null: false
      t.belongs_to :parent#, null: false
      t.timestamps
    end
  end
end

[al2o3cr]

I've made an executable test case for this: https://gist.github.com/al2o3cr/245b9592acd5dfde66bd with logs for both cases.

This appears to reproduce on 4.1.3 through 4.2.0.rc2 as well.

[al2o3cr]

Have to call it a night, but here's my findings so far:

• the behavior stops if you add autosave: false to has_many :parents, through: :student_parents on Student (?!)
• add_autosave_association_callbacks is being called TWICE on both 4.1.0 and 4.1.2. This appears to be caused by code in accepts_nested_attributes_for which always calls add_autosave_association_callbacks, even if the association is already marked for autosave.

This change:
v4.1.1...v4.1.2#diff-829fd5510b886395117cc530518ef7f7L179

which shipped in 4.1.2 altered the behavior of add_autosave_association_callbacks. Before the change, the method was idempotent since it skipped defining callbacks if the methods were already defined. I'm not certain if it is relevant, but it seems like one of the only changes that touches relevant code between 4.1.1 and 4.1.2 - here's all the changes.

When I've got some time, I plan to attempt a git bisect to figure out where this changed.

[al2o3cr]

Git bisect points the finger at 8cb48ab. Still need to determine how this changed things.

[al2o3cr]

Looks like calling add_autosave_association_callbacks twice causes the two callbacks generated on Student to be run in the opposite order in 4.1.2. On 4.1.1, the sequence of events looks like this:

• autosave_associated_records_for_student_parent runs and correctly saves the join record
• autosave_associated_records_for_parents runs and doesn't run any SQL

After 8cb48ab, however, the sequence looks like this:

• autosave_associated_records_for_parents runs and writes a StudentParent with NULL student_id
• autosave_associated_records_for_student_parent runs and updates that record

The fix does not appear to be straightforward, as reinstating the unless method_defined?(save_method) guard causes the test added in 8cb48ab to break - with the same sort of error this PR is reporting 😿

  1) Error:
HasAndBelongsToManyAssociationsTest#test_redefine_habtm:
ActiveRecord::StatementInvalid: SQLite3::ConstraintException: NOT NULL constraint failed: developers_projects.project_id: INSERT INTO "developers_projects" ("developer_id") VALUES (?)
    /var/lib/gems/2.1.0/gems/sqlite3-1.3.10/lib/sqlite3/statement.rb:108:in `step'
    /var/lib/gems/2.1.0/gems/sqlite3-1.3.10/lib/sqlite3/statement.rb:108:in `block in each'
    /var/lib/gems/2.1.0/gems/sqlite3-1.3.10/lib/sqlite3/statement.rb:107:in `loop'
    /var/lib/gems/2.1.0/gems/sqlite3-1.3.10/lib/sqlite3/statement.rb:107:in `each'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:319:in `to_a'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:319:in `block in exec_query'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:373:in `block in log'
    /vagrant/rails/activesupport/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:367:in `log'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:298:in `exec_query'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:68:in `exec_insert'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:95:in `insert'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:14:in `insert'
    /vagrant/rails/activerecord/lib/active_record/relation.rb:64:in `insert'
    /vagrant/rails/activerecord/lib/active_record/persistence.rb:502:in `_create_record'
    /vagrant/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:87:in `_create_record'
    /vagrant/rails/activerecord/lib/active_record/timestamp.rb:57:in `_create_record'
    /vagrant/rails/activerecord/lib/active_record/callbacks.rb:306:in `block in _create_record'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:82:in `run_callbacks'
    /vagrant/rails/activerecord/lib/active_record/callbacks.rb:306:in `_create_record'
    /vagrant/rails/activerecord/lib/active_record/persistence.rb:482:in `create_or_update'
    /vagrant/rails/activerecord/lib/active_record/callbacks.rb:302:in `block in create_or_update'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:113:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:113:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:166:in `block in halting'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:166:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:166:in `block in halting'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:86:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:86:in `run_callbacks'
    /vagrant/rails/activerecord/lib/active_record/callbacks.rb:302:in `create_or_update'
    /vagrant/rails/activerecord/lib/active_record/persistence.rb:125:in `save!'
    /vagrant/rails/activerecord/lib/active_record/validations.rb:57:in `save!'
    /vagrant/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:29:in `save!'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:273:in `block in save!'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:329:in `block in with_transaction_returning_status'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:199:in `transaction'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:208:in `transaction'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:326:in `with_transaction_returning_status'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:273:in `save!'
    /vagrant/rails/activerecord/lib/active_record/associations/has_many_through_association.rb:97:in `save_through_record'
    /vagrant/rails/activerecord/lib/active_record/associations/has_many_through_association.rb:64:in `insert_record'
    /vagrant/rails/activerecord/lib/active_record/autosave_association.rb:365:in `block in save_collection_association'
    /vagrant/rails/activerecord/lib/active_record/autosave_association.rb:356:in `each'
    /vagrant/rails/activerecord/lib/active_record/autosave_association.rb:356:in `save_collection_association'
    /vagrant/rails/activerecord/lib/active_record/autosave_association.rb:189:in `block in add_autosave_association_callbacks'
    /vagrant/rails/activerecord/lib/active_record/autosave_association.rb:157:in `instance_eval'
    /vagrant/rails/activerecord/lib/active_record/autosave_association.rb:157:in `block in define_non_cyclic_method'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:424:in `block in make_lambda'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:221:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:221:in `block in halting_and_conditional'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `block in halting_and_conditional'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `block in halting_and_conditional'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `block in halting_and_conditional'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `block in halting_and_conditional'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `block in halting_and_conditional'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:215:in `block in halting_and_conditional'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:86:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:86:in `run_callbacks'
    /vagrant/rails/activerecord/lib/active_record/callbacks.rb:306:in `_create_record'
    /vagrant/rails/activerecord/lib/active_record/persistence.rb:482:in `create_or_update'
    /vagrant/rails/activerecord/lib/active_record/callbacks.rb:302:in `block in create_or_update'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:113:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:113:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:166:in `block in halting'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:86:in `call'
    /vagrant/rails/activesupport/lib/active_support/callbacks.rb:86:in `run_callbacks'
    /vagrant/rails/activerecord/lib/active_record/callbacks.rb:302:in `create_or_update'
    /vagrant/rails/activerecord/lib/active_record/persistence.rb:103:in `save'
    /vagrant/rails/activerecord/lib/active_record/validations.rb:51:in `save'
    /vagrant/rails/activerecord/lib/active_record/attribute_methods/dirty.rb:21:in `save'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:268:in `block (2 levels) in save'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:329:in `block in with_transaction_returning_status'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:201:in `block in transaction'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:209:in `within_new_transaction'
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:201:in `transaction'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:208:in `transaction'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:326:in `with_transaction_returning_status'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:268:in `block in save'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:283:in `rollback_active_record_state!'
    /vagrant/rails/activerecord/lib/active_record/transactions.rb:267:in `save'
    /vagrant/rails/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb:875:in `test_redefine_habtm'

[griffithchaffee]

In the off chance that it is helpful I will provide my local fix for this issue.

When I was investigating this bug I noticed that the join_table_attributes= setter still worked correctly. My temporary fix was to redefine the association_ids= setter to use this. I'm not sure how this method interacts with add_autosave_association_callbacks but maybe looking into this could provide a solution.

def student_ids=(new_student_ids)
  student_parents_attributes = []
  new_student_ids = new_student_ids.to_a.map(&:to_param).select(&:present?).map(&:to_i)
  # remove missing students
  student_parents.each do |student_parent|
    student_parents_attributes << { id: student_parent.id, _destroy: true } unless student_parent.student_id.in? new_student_ids
  end
 # add new students
  new_student_ids.each do |new_student_id|
    student_parents_attributes << { student_id: new_student_id } unless student_parents.select { |student_parent| student_parent.student_id == new_student_id }
  end
  self.student_parents_attributes = student_parents_attributes if student_parents_attributes.present?
end

[al2o3cr]

Tracked down the fix for this. The tests added in 8cb48ab fail if the method_defined? check is present, because the HABTM is redeclared on a subclass. Changing the check to method_defined_within?(save_method, self) (from ActiveRecord::AttributeMethods) appears to solve the problem.

Will PR the solution in the next couple days.

[pranas]

It's the same issue as in #16494. I started looking at it some time ago and wrote a failing test case: https://github.com/pranas/rails/tree/fix_collection_singular_ids=

@al2o3cr I see you got the solution figured out so I'll leave it to you to make the PR (unless you don't want to?). Feel free to pull my test case.

[kamipo]

It works since 4.2.1.