has_one relation start deleting existing record even when new record failed passing validation #48330
Comments
|
👋 Hey @nov ! Can you provide a full repro script? I tried this out with the following script: # frozen_string_literal: true
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails", "~> 7.0.5"
gem "debug"
gem "sqlite3"
end
require "active_record"
require "minitest/autorun"
require "logger"
# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Schema.define do
create_table :users, force: true do |t|
end
create_table :totp_settings, force: true do |t|
t.references :user
end
end
class User < ActiveRecord::Base
has_one :totp_setting
end
class TotpSetting < ActiveRecord::Base
belongs_to :user
validates :user_id, presence: true, uniqueness: true
end
class BugTest < Minitest::Test
def test_bug
@user = User.create!
@user.create_totp_setting!
assert_raises ActiveRecord::RecordNotUnique do
@user.create_totp_setting!
end
end
endBut I see the same result on Rails 7.0.4 and Rails 7.0.5: Which seems to be reminiscent of the bug described here. |
yahonda
added
the
more-information-needed
|
Ah, I missed # frozen_string_literal: true
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails", "~> 7.0.5"
gem "debug"
gem "sqlite3"
end
require "active_record"
require "minitest/autorun"
require "logger"
# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Schema.define do
create_table :users, force: true do |t|
end
create_table :totp_settings, force: true do |t|
t.references :user
end
end
class User < ActiveRecord::Base
has_one :totp_setting, dependent: :destroy
end
class TotpSetting < ActiveRecord::Base
belongs_to :user
validates :user_id, presence: true, uniqueness: true
end
class BugTest < Minitest::Test
def test_bug
@user = User.create!
@user.create_totp_setting!
assert_raises ActiveRecord::RecordNotUnique do
@user.create_totp_setting!
end
end
end |
rails-bot
bot
removed
the
more-information-needed
|
Okay, so this appears to be due to changes around With v7.0.5, however, which means that by the time we save it, the original record has been removed, so there is no uniqueness validation error. The thing I find surprising is that, even with Rails 7.0.4, the initial record is deleted even though we do raise an class BugTest < Minitest::Test
def test_bug
@user = User.create!
@user.create_totp_setting!
assert_raises ActiveRecord::RecordInvalid do
@user.create_totp_setting!
end
assert_equal 1, TotpSetting.count, "Expected one TotpSetting to be left"
end
endI get I would expect replacing the record to only take place after we've ensured the new record is valid, but since this behaviour has been constant across 7.0.4 and 7.0.5 I'd like clarification from someone on the Core team as to whether this behaviour is expected. |
|
It looks like this behavior change has been introduced via bdbe58b |
|
@yahonda do you have thoughts on whether the v7.0.4 behaviour would be considered problematic as well? If we addressed the issue of replacing the record after validation, we'd start seeing the expected To me, it seems incorrect that, despite raising the AR error, we delete the original record even in v7.0.4. |
|
@adrianna-chang-shopify Thanks for the detailed investigation. IMO, this bug report concern is about exception handling, not (only) about the number of rows. I think the Rails v7.0.4 behavior is expected at least for the 7-0-stable branch. This issue is complicated, so I also would like to know the opinion from @byroot. |
|
Yes, this change is undesirable. I'll revert the PR right away, however it would be nice to add a test. |
Ref: rails#48330 When replacing the has_one target, it seems more correct to first delete the old record, so that if the associated model has a uniqueness validator, it won't fail. Both the delete and the insert are in a single transaction, so if the new record fail to be saved, the transaction will be rolled back, which seems correct. If `dependent: :destroy` isn't set, Active Record will try to orphan the record, which may or may not be valid depending on the schema and validators. Co-Authored-By: zzak <zzakscott@gmail.com>
|
We reviewed this is #48416, and it seems to us that the newer behavior is more correct, as it allows to replace a has_one even if you have a unicity constraint. We added tests to make sure this behavior is preserved. |
|
so you mean the behaviour changes in this minor version up? |
I'm not sure I understand your question, but in my opinion the old behavior was faulty, and I consider this change a bug fix. Now I understand that some code may have relied on this bug, but it's still a bug fix. It's debatable whether this bugfix should have been backported, but now that it was, I don't think flip-floping would help. |
|
I thought this is unexpected breaking change w/o any changelog mention, but if you say it's bug fix, I can fix our code base for 7.0.5 |
|
I have just come across this issue, but for a different reason when using callbacks. This works with 7.0.4.*... # frozen_string_literal: true
require 'bundler/inline'
gemfile(true) do
source 'https://rubygems.org'
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem 'rails', '~> 7.0.4.3'
gem 'debug'
gem 'sqlite3'
end
require 'active_record'
require 'active_support/testing/assertions'
require 'minitest/autorun'
require 'logger'
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Schema.define do
create_table :appointments, force: true do |t|
t.boolean :paid, default: false
end
create_table :payments, force: true do |t|
t.references :appointment
end
end
$before_create_call_count = 0
class Appointment < ActiveRecord::Base
has_one :payment
after_create do
create_payment
end
end
class Payment < ActiveRecord::Base
belongs_to :appointment
before_create do
$before_create_call_count += 1
appointment.update_attribute :paid, true
end
end
class BugTest < Minitest::Test
include ActiveSupport::Testing::Assertions
def test_bug
assert_difference '$before_create_call_count', 1 do
Appointment.create!
end
end
endBut fails on 7.0.5 What is happening on 7.0.5, is that the Any feedback as to whether this is a new bug or not would be greatly appreciated. thx |
|
I am also having trouble with the same issue. @byroot I know I can maintain the behavior if all implementations don't use the - @user.create_totp_setting!
+ TotpSetting.create!(user: @user)However, this is very difficult, so I'm looking for an easy migration method. |
So you have an |
|
No, what we want to do here is
|
|
Ok, so you want some kind of |
|
not find, but raise exception (or |
|
To be fair, I'm not convinced But using a uniqueness validator certainly wasn't a proper way to do this as it's very racy. In the meantime |
|
yeah, I'm adding those |
|
Changes that add The behavior I want to keep is below.
|
|
Can @byroot or anyone comment on #48330 (comment) please? |
|
@joelmoss same as for the others. Your code should do something like |
|
This change has huge huge huge impact. Rails team should release more carefully. |
Uniqueness validators no longer works on `create_association` when using `dependent: :destroy` option in the commit below. refs: bdbe58b, 3255411 However, we often expect validation errors as well. refs rails#48330, rails#48632 This commit adds an option to run validations before deleting an existing record, to reproduce the previous behavior.
Steps to reproduce
Expected behavior
Actual behavior
System configuration
Rails version: 7.0.5
Ruby version: 3.1.4
The text was updated successfully, but these errors were encountered: