Save succeeds despite associated record failing to save

[Mangara]

Steps to reproduce

Models:

class Parent < ApplicationRecord
  has_many :children
end

class Child < ApplicationRecord
  belongs_to :parent
  validates :name, uniqueness: true
end

Code:

parent = Parent.new(children: [Child.new(name: 'Wiske'), Child.new(name: 'Wiske')])
parent.save

Executable test

Expected behavior

The children are invalid, so save should return false and no changes should be made to the database.

Actual behavior

It returns true after saving the parent and one of the children:

parent = Parent.new(children: [Child.new(name: 'Wiske'), Child.new(name: 'Wiske')])
parent.valid? # true
  # Child Exists (0.6ms)  SELECT  1 AS one FROM `children` WHERE `children`.`name` = BINARY 'Wiske' LIMIT 1
  # Child Exists (0.5ms)  SELECT  1 AS one FROM `children` WHERE `children`.`name` = BINARY 'Wiske' LIMIT 1
parent.save # true
  #  (0.6ms)  BEGIN
  # Child Exists (0.6ms)  SELECT  1 AS one FROM `children` WHERE `children`.`name` = BINARY 'Wiske' LIMIT 1
  # Child Exists (0.5ms)  SELECT  1 AS one FROM `children` WHERE `children`.`name` = BINARY 'Wiske' LIMIT 1
  # Parent Create (0.5ms)  INSERT INTO `parents` (`created_at`, `updated_at`) VALUES ('2018-05-20 19:14:06', '2018-05-20 19:14:06')
  # Child Exists (0.6ms)  SELECT  1 AS one FROM `children` WHERE `children`.`name` = BINARY 'Wiske' LIMIT 1
  # Child Create (7.2ms)  INSERT INTO `children` (`name`, `parent_id`) VALUES ('Wiske', 5)
  # Child Exists (0.6ms)  SELECT  1 AS one FROM `children` WHERE `children`.`name` = BINARY 'Wiske' LIMIT 1
  #  (2.8ms)  COMMIT
parent.persisted? # true
parent.children # [#<Child id: 6, name: "Wiske", parent_id: 5>, #<Child id: nil, name: "Wiske", parent_id: 5>]
parent.children[1].errors.full_messages # ["Name has already been taken"]
persisted_parent = Parent.find(parent.id)
persisted_parent.children # [#<Child id: 6, name: "Wiske", parent_id: 5>]

Since there are no uniqueness problems between each child individually and what's already in the database, validation of the parent passes. Each child is revalidated before being saved, so when the second child is saved, the duplication is detected and it is not inserted into the database. For some reason, this does not roll back the transaction.

System configuration

Rails version: 5.2.0

Ruby version: 2.5.1p57 (2018-03-29 revision 63029) [x86_64-darwin17]

(See also #32940)

[gwincr11]

@Mangara I think this will help you. http://api.rubyonrails.org/v4.1.1/classes/ActiveRecord/Validations/ClassMethods.html#method-i-validates_associated

[Mangara]

@gwincr11 Thanks for the pointer! Unfortunately, the behaviour is the same with or without validates_associated. Looking at all the Child Exists queries, it actually runs the validation just fine. However, because uniqueness validation does not catch unpersisted duplicates (see #4568), the problems are only found midway through the transaction.

[utilum]

Here's a reproduction. Saving a model instance succeeds in spite of it specifying validates_associated and it having two associated objects that violate uniqueness.
This is a much simpler case than #4568. That said, I'm not sure it is unexpected behavior ...

[gwincr11]

Oh rightI I always forget that AR doesn't enforce this at the database level. I think you can add unique true to the migration column that will enforce it at the db level but I am not entirely sure how it handles this situation you are showing. 🤔

[Mangara]

@gwincr11 In this case the db constraint would not get hit, which is why I omitted it. AR detects the duplication just before inserting the record, so the db is never in an invalid state. That said, in practice the constraint should definitely be there, to guard against race conditions.

[Mangara]

@utilum The unexpected part, for me, is that save returns true despite only partially succeeding due to a validation error (note that the second child was not actually saved). The method description says:

By default, save always runs validations. If any of them fail the action is cancelled and #save returns +false+, and the record won't be saved.

[utilum]

I do see your point. Also the edginess of the case, as each of the objects involved is valid in and of itself. Would you have save wrap it all in a transaction and roll that back?

[Mangara]

Yes, that sounds like the best solution to me.

[mechanicles]

Added a fix here #32952

[kamipo]

Fixed by #32796.