-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ActiveRecord::RecordNotUniqueのエラーが出ないように修正 #8006
base: main
Are you sure you want to change the base?
Conversation
@reckyy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@su-su-su-su
お疲れ様です!
何点かコメントしていますので、ご確認よろしくお願いいたします 🙇
以下、コードとは関係ない部分です。
-
PRの最初のコメント上部に、不要な文字列があります。
-
PRを2つすでにcloseされていますが、何故closeしたかの理由を記載しておいたほうがいいと思います〜。
-
変更前後のコードも、写真ではなくコードブロックで記載していただきたかったです〜!
そちらのほうがみやすいので☺️
app/models/report.rb
Outdated
|
||
def save_with_uniqueness_check | ||
ActiveRecord::Base.transaction do | ||
if new_record? && Report.where(user_id:, reported_on:).exists? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ask]このメソッドって、createアクションでしか呼ばれないので新しいレコードなのは自明だと思いますが、いかがでしょうか。👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ご指摘ありがとうございます。
おっしゃる通り通りで不要だと感じたので削除いたしました。
@@ -134,4 +134,14 @@ def not_wip_previous_of_user | |||
.order(reported_on: :desc) | |||
.second | |||
end | |||
|
|||
def save_with_uniqueness_check | |||
ActiveRecord::Base.transaction do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[must]transactionは一連の操作を「全成功」か「全失敗」させることにより、矛盾を防ぐものです。
そのため、失敗した場合にはRollbackしないといけませんがこのメソッドではコミットされてしまっています。(内部で何も行なっていないのでDB上は違いはありませんが、期待される結果ではありません)
Rollbackされるように修正をお願いいたします!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
元のコードを見るとRollbackされていないことを確認しました。
こちら
def save_with_uniqueness_check
ActiveRecord::Base.transaction do
if Report.where(user_id:, reported_on:).exists?
errors.add(:reported_on, 'はすでに存在します')
raise ActiveRecord::Rollback
end
save
end
end
と明示的にRollbackするように修正いたしました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[must]rollbackされるのが同じ日の日報が存在してた時のみになっています〜。
存在しなかった場合、もし入力内容に不備があってもコミットされてしまっています!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます!
こちらも確認したところRollbackされていませんでした。
def save_with_uniqueness_check
ActiveRecord::Base.transaction do
if Report.where(user_id:, reported_on:).exists?
errors.add(:reported_on, 'はすでに存在します')
raise ActiveRecord::Rollback
end
raise ActiveRecord::Rollback unless save
true
end
end
としてsaveされない場合はRollbackするように修正しました。
同じタイトルが存在した場合:
report2 = Report.new(
user: user,
reported_on: '2022-06-12',
title: 'test55',
description: 'test',
emotion: 'happy'
)
=>
#<Report:0x00007f0dcf0f4e50
...
if report2.save_with_uniqueness_check
puts "レポート2: 保存"
else
puts "レポート2: エラー #{report2.errors.full_messages.join(', ')}"
end
TRANSACTION (0.6ms) BEGIN
Report Exists? (1.5ms) SELECT 1 AS one FROM "reports" WHERE "reports"."user_id" = $1 AND "reports"."reported_on" = $2 LIMIT $3 [["user_id", 253826460], ["reported_on", "2022-06-12"], ["LIMIT", 1]]
Report Exists? (1.0ms) SELECT 1 AS one FROM "reports" WHERE "reports"."title" = $1 AND "reports"."user_id" = $2 LIMIT $3 [["title", "test55"], ["user_id", 253826460], ["LIMIT", 1]]
Report Exists? (0.9ms) SELECT 1 AS one FROM "reports" WHERE "reports"."reported_on" = $1 AND "reports"."user_id" = $2 LIMIT $3 [["reported_on", "2022-06-12"], ["user_id", 253826460], ["LIMIT", 1]]
TRANSACTION (0.6ms) ROLLBACK
レポート2: エラー タイトルはすでに存在します
=> nil
また、
raise ActiveRecord::Rollback unless save
あとのtrue
なのですが、true
返さない形でもデータの保存自体は問題なく行われていました。しかし、以下のコードの実行結果では、保存が成功しているにもかかわらず、elseのレポート2: エラー
が表示されてしまいました。
if report2.save_with_uniqueness_check
puts "レポート2: 保存"
else
puts "レポート2: エラー #{report2.errors.full_messages.join(', ')}"
end
TRANSACTION (0.4ms) BEGIN
Report Exists? (1.4ms) SELECT 1 AS one FROM "reports" WHERE "reports"."user_id" = $1 AND "reports"."reported_on" = $2 LIMIT $3 [["user_id", 253826460], ["reported_on", "2022-06-05"], ["LIMIT", 1]]
Report Exists? (1.0ms) SELECT 1 AS one FROM "reports" WHERE "reports"."title" = $1 AND "reports"."user_id" = $2 LIMIT $3 [["title", "test123"], ["user_id", 253826460], ["LIMIT", 1]]
Report Exists? (0.7ms) SELECT 1 AS one FROM "reports" WHERE "reports"."reported_on" = $1 AND "reports"."user_id" = $2 LIMIT $3 [["reported_on", "2022-06-05"], ["user_id", 253826460], ["LIMIT", 1]]
Report Create (0.9ms) INSERT INTO "reports" ("user_id", "title", "description", "created_at", "updated_at", "reported_on", "emotion") VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING "id" [["user_id", 253826460], ["title", "test123"], ["description", "test"], ["created_at", "2024-08-19 02:44:09.909464"], ["updated_at", "2024-08-19 02:44:09.909464"], ["reported_on", "2022-06-05"], ["emotion", 2]]
Watch Exists? (1.3ms) SELECT 1 AS one FROM "watches" WHERE "watches"."user_id" = $1 AND "watches"."watchable_id" = $2 AND "watches"."watchable_type" = $3 LIMIT $4 [["user_id", 253826460], ["watchable_id", 1066585839], ["watchable_type", "Report"], ["LIMIT", 1]]
Watch Create (0.9ms) INSERT INTO "watches" ("watchable_type", "watchable_id", "created_at", "updated_at", "user_id") VALUES ($1, $2, $3, $4, $5) RETURNING "id" [["watchable_type", "Report"], ["watchable_id", 1066585839], ["created_at", "2024-08-19 02:44:09.938174"], ["updated_at", "2024-08-19 02:44:09.938174"], ["user_id", 253826460]]
User Update (1.1ms) UPDATE "users" SET "updated_at" = $1 WHERE "users"."id" = $2 [["updated_at", "2024-08-19 02:44:09.940861"], ["id", 253826460]]
TRANSACTION (15.1ms) COMMIT
レポート2: エラー
=> nil
これを修正するために正常に保存が完了した場合にtrue
を返すようにしました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ask]save!
を使うのではなく、明示的にtrueを返すようにした理由は何かありますか?
トランザクションは例外をキャッチしたら自動でRollbackを行なってくれます。
Exceptions will force a ROLLBACK that returns the database to the state before the transaction began.
https://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html
ActiveRecord::Base.transaction do | |
ActiveRecord::Base.transaction do | |
if Report.where(user_id:, reported_on:).exists? | |
errors.add(:reported_on, 'はすでに存在します') | |
raise ActiveRecord::Rollback | |
end | |
save! | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
お疲れ様です。
save_with_uniqueness_check
メソッドを以下のようにsave!
に修正した際の挙動についてなのですが、
ActiveRecord::Base.transaction do
if Report.where(user_id:, reported_on:).exists?
errors.add(:reported_on, 'はすでに存在します')
raise ActiveRecord::Rollback
end
save!
end
app/controllers/reports_controller.rb
のcreate
メソッドが if @report.save_with_uniqueness_check
で条件分岐をしています。
def create
@report = Report.new(report_params)
@report.user = current_user
set_wip
canonicalize_learning_times(@report)
if @report.save_with_uniqueness_check
Newspaper.publish(:report_save, { report: @report })
redirect_to redirect_url(@report), notice: notice_message(@report), flash: flash_contents(@report)
else
render :new
end
end
この場合save_with_uniqueness_check
メソッドがtrue
またはfalse
を返すことが期待されています。
しかし、save!
を使って例外を発生させた場合、トランザクションはロールバックされますが、false
は返さないようです。
その結果、
if @report.save_with_uniqueness_check
の部分が処理することができず、エラーになってしまうことを確認しました。
そこで次にどう修正をしていくかなのですが、
save! を使わずに元のコードに戻す方が良いか。
引き続きsave!を使って他を修正する方が良いか。
どちらの方法が良いか、ご意見いただけますと幸いです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@su-su-su-su
お疲れ様です。
createメソッドの分岐、失念していました。。申し訳ないです😭
となると、unless文でRollbackさせていたコードが良いかもです。
不適切な提案してしまい、申し訳ないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reckyy
トランザクションでsave!
を使う意味なども知れたのでむしろ勉強になりました🙇
ありがとうございます!
こちら再度コードを戻すように修正しました。
再度ご確認いただけますでしょうか。
よろしくお願いいたします。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます!
確認しました〜。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます🙇♂️
test/models/report_test.rb
Outdated
@@ -43,4 +43,25 @@ class ReportTest < ActiveSupport::TestCase | |||
test '#interval' do | |||
assert_equal 10, reports(:report32).interval | |||
end | |||
|
|||
test '#reported_on_uniqueness_check' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[must]テストの名前と実際のメソッド名が異なっています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ご指摘ありがとうございます。
モデルのメソッド名に合わせて名前を統一しました。
test/models/report_test.rb
Outdated
report1.title = 'test1' | ||
report1.description = 'test1' | ||
report1.emotion = 'happy' | ||
report1.save_with_uniqueness_check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ask]ここで既存のfixturesを使用しなかった理由はありますか?👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixturesを分かっていなかったです。もし前に勉強していたら忘れてしまったいました。
fixturesを使うように修正いたしました。
test '#save_with_uniqueness_check' do
report1 = reports(:report1)
report1.save_with_uniqueness_check
report2 = report1.dup
report2.title = 'test2'
report2.save_with_uniqueness_check
assert_includes report2.errors.full_messages, '学習日はすでに存在します'
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[imo]以下のコードは必要でしょうか?
report1.save_with_uniqueness_check
report2.title = 'test2'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ご指摘ありがとうございます!
再度確認したところ、
report1.save_with_uniqueness_check
は既に保存しているデータなので不要でした。
またreport2.title = 'test2'
はタイトルが同じだとそのバリデーションも実行されてしまうと思っていたのですが、save_with_uniqueness_check
メソッドで
if Report.where(user_id:, reported_on:).exists?
を先にしてからsaveしていたのでこちらも不要でした。
@reckyy トランザクションを使って失敗すると勝手にロールバックするものだと勘違いしていました。 fixturesも使うことでコードが短くなり、こちらも勉強になりました! コードと関係ない部分も合わせて修正いたしましたのでお手隙の際に再度ご確認をお願いします 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@su-su-su-su
遅くなり申し訳ございません!
2点コメントしていますので、ご確認よろしくお願いいたします。
個人的にお話ししたいのですが、Discordアカウントはお持ちではないですか?チャンネルが見当たらなかったので。
またPRをcloseした理由は、各PRに記載すべきだと思います。closeされたPRを見てから、本PRに辿り着くとは限らないからです。
(closeされた理由を書くというのは、自分が気にしすぎなのかもしれないのでご対応はお任せします。)
@@ -134,4 +134,14 @@ def not_wip_previous_of_user | |||
.order(reported_on: :desc) | |||
.second | |||
end | |||
|
|||
def save_with_uniqueness_check | |||
ActiveRecord::Base.transaction do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[must]rollbackされるのが同じ日の日報が存在してた時のみになっています〜。
存在しなかった場合、もし入力内容に不備があってもコミットされてしまっています!
test/models/report_test.rb
Outdated
report1.title = 'test1' | ||
report1.description = 'test1' | ||
report1.emotion = 'happy' | ||
report1.save_with_uniqueness_check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[imo]以下のコードは必要でしょうか?
report1.save_with_uniqueness_check
report2.title = 'test2'
@reckyy
すみません。確認したところチャンネルがなかったので作成しました!
アドバイスありがとうございます。とても納得しました。closeしたPRにも理由を書きました。 コードの修正はまだ終えていないので改めてご連絡させていただきます。 |
@reckyy |
@su-su-su-su また、Discordでも昨日メッセージを送信しているため、ご返信お待ちしております。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@su-su-su-su
お疲れ様です。
返信が遅くなり申し訳ないです。
私からはOKです🙆
@reckyy |
@komagata |
まず、「変更確認方法」がおかしいように思います。 |
@komagata ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_reports_on_user_id_and_reported_on" DETAIL: Key (user_id, reported_on)=(2018, 2024-06-24) already exists. Report.where(user_id: 2018, reported_on: '2024-06-24')
Report Load (0.5ms) SELECT "reports".* FROM "reports" WHERE "reports"."user_id" = $1 AND "reports"."reported_on" = $2 [["user_id", 2018], ["reported_on", "2024-06-24"]] 次にこの日報と同じ report1 = Report.new(
user_id: 2018,
reported_on: '2024-06-24',
title: 'test1',
description: 'test',
emotion: 'happy'
)
begin
report1.save!(validate: false)
puts "レポート1: 保存"
rescue => e
puts "レポート1: エラー #{e.message}"
end すると以下の結果になりました。 TRANSACTION (0.6ms) BEGIN
Report Create (0.8ms) INSERT INTO "reports" ("user_id", "title", "description", "created_at", "updated_at", "reported_on", "emotion") VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING "id" [["user_id", 2018], ["title", "test1"], ["description", "test"], ["created_at", "2024-09-24 08:09:20.844418"], ["updated_at", "2024-09-24 08:09:20.844418"], ["reported_on", "2024-06-24"], ["emotion", 2]]
User Load (0.8ms) SELECT "users".* FROM "users" WHERE "users"."id" = $1 LIMIT $2 [["id", 2018], ["LIMIT", 1]]
Watch Exists? (0.7ms) SELECT 1 AS one FROM "watches" WHERE "watches"."user_id" IS NULL AND "watches"."watchable_id" = $1 AND "watches"."watchable_type" = $2 LIMIT $3 [["watchable_id", 1066585821], ["watchable_type", "Report"], ["LIMIT", 1]]
TRANSACTION (0.5ms) ROLLBACK
レポート1: エラー バリデーションに失敗しました: Userを入力してください, Userを入力してください
その後 退会したかも調べたのですが、 user = User.find_by(id: 2018)
if user&.retired_on
puts "#{user.name}."
else
puts "ユーザーはいません"
end
ご指摘頂いた「原因がわかる前に、現状存在しないコードで確かめても意味がない」というのは、この |
いいえです。 report1 = Report.new(
user_id: 2018,
reported_on: '2024-06-24',
title: 'test1',
description: 'test',
emotion: 'happy'
)
begin
report1.save!(validate: false)
puts "レポート1: 保存"
rescue => e
puts "レポート1: エラー #{e.message}"
end このコードが現状のbootcampのコードに存在しないとしたら、それで確かめるのは意味がないということになります。 |
@komagata #7888 にて そのため、詳細を確認することが難しい状況です。 |
該当のRollbarのデータは下記ですが、どこのコードというのは残ってないのでコードからどこで起きているのか少し考えて探してみてください〜 |
返信ありがとうございます。 Rollbarのリンク これはRollbarへのアクセス権限に問題があるのでしょうか? それとも
ということから、この挙動で問題ないのでしょうか。 |
こちらはこの情報しか残ってないので気にしないでください。 |
2つPRをcloseしている理由なのですが、push前に
git pull --rebase origin main
しておらず、自分のコミットでないものが入ってしまいました。1度目はそこから修正しようとしましたが余計に分からなくなった為closeしました。2度目は解決方法が分かったので、closeしました。Issue
概要
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_reports_on_user_id_and_reported_on" DETAIL: Key (user_id, reported_on)=(2018, 2024-06-24) already exists.
とエラーが出るのを回避するための修正を行いました。
変更確認方法
ブランチ
bug/fix-unique-violation-cleanup
をローカルに取り込むrails console
でコンソールを立ち上げる以下のコマンドを実行してユーザーを定義
以下のコードを実行し、エラーが出ることを確認
以下のコードを実行し、エラーが出ないでバリデーションが実行されていることを確認
変更前
変更後