From 04ba5baec034d656ec04c83dfcba22c2df2a5a36 Mon Sep 17 00:00:00 2001 From: Kelv Date: Mon, 16 Dec 2024 19:48:25 +0800 Subject: [PATCH] DEV: ensure rebaking works even when some users have inconsistent data (#30261) * DEV: add db consistency check for UserEmail * DEV: add db consistency check for UserAvatar * DEV: ignore inconsistent data related to user avatars when deciding whether to rebake old posts Co-authored-by: Alan Guo Xiang Tan --------- Co-authored-by: Alan Guo Xiang Tan --- app/jobs/scheduled/ensure_db_consistency.rb | 1 + app/jobs/scheduled/periodical_updates.rb | 5 +- app/models/user_avatar.rb | 7 +++ app/models/user_email.rb | 17 ++++++ spec/jobs/periodical_updates_spec.rb | 57 ++++++++++++++++++++- spec/models/user_avatar_spec.rb | 11 ++++ spec/models/user_email_spec.rb | 22 ++++++++ 7 files changed, 118 insertions(+), 2 deletions(-) diff --git a/app/jobs/scheduled/ensure_db_consistency.rb b/app/jobs/scheduled/ensure_db_consistency.rb index dd38439845e..1e93d7d91f8 100644 --- a/app/jobs/scheduled/ensure_db_consistency.rb +++ b/app/jobs/scheduled/ensure_db_consistency.rb @@ -22,6 +22,7 @@ module Jobs CategoryTagStat, User, UserAvatar, + UserEmail, Category, TopicThumbnail, ].each do |klass| diff --git a/app/jobs/scheduled/periodical_updates.rb b/app/jobs/scheduled/periodical_updates.rb index 222836a9ba1..b58cef4d70f 100644 --- a/app/jobs/scheduled/periodical_updates.rb +++ b/app/jobs/scheduled/periodical_updates.rb @@ -25,7 +25,10 @@ module Jobs # Forces rebake of old posts where needed, as long as no system avatars need updating if !SiteSetting.automatically_download_gravatars || - !UserAvatar.where("last_gravatar_download_attempt IS NULL").limit(1).first + !UserAvatar + .joins(user: :user_emails) + .where(user_emails: { primary: true }, last_gravatar_download_attempt: nil) + .exists? problems = Post.rebake_old(SiteSetting.rebake_old_posts_count, priority: :ultra_low) problems.each do |hash| post_id = hash[:post].id diff --git a/app/models/user_avatar.rb b/app/models/user_avatar.rb index fbf191b454e..0e6f26ade53 100644 --- a/app/models/user_avatar.rb +++ b/app/models/user_avatar.rb @@ -152,6 +152,13 @@ class UserAvatar < ActiveRecord::Base end def self.ensure_consistency!(max_optimized_avatars_to_remove: 20_000) + DB.exec <<~SQL + DELETE FROM user_avatars + USING user_avatars ua + LEFT JOIN users u ON ua.user_id = u.id + WHERE user_avatars.id = ua.id AND u.id IS NULL + SQL + DB.exec <<~SQL UPDATE user_avatars SET gravatar_upload_id = NULL diff --git a/app/models/user_email.rb b/app/models/user_email.rb index 43463e4aeab..dcc6907f5f7 100644 --- a/app/models/user_email.rb +++ b/app/models/user_email.rb @@ -22,6 +22,23 @@ class UserEmail < ActiveRecord::Base before_save -> { destroy_email_tokens(self.email_was) }, if: :will_save_change_to_email? after_destroy { destroy_email_tokens(self.email) } + def self.ensure_consistency! + user_ids_without_primary_email = DB.query_single <<~SQL + SELECT u.id + FROM users u + LEFT JOIN user_emails ue ON u.id = ue.user_id AND ue.primary = true + WHERE ue.id IS NULL; + SQL + + user_ids_without_primary_email.each do |user_id| + UserEmail.create!( + user_id: user_id, + # 64 max character length of local-part for the email address https://datatracker.ietf.org/doc/html/rfc5321#section-4.5.3.1.1 + email: "#{SecureRandom.alphanumeric(64)}@missing-primary-email.invalid", + primary: true, + ) + end + end def normalize_email self.normalized_email = diff --git a/spec/jobs/periodical_updates_spec.rb b/spec/jobs/periodical_updates_spec.rb index 3b0749eb4e4..811c8d9be9a 100644 --- a/spec/jobs/periodical_updates_spec.rb +++ b/spec/jobs/periodical_updates_spec.rb @@ -1,6 +1,13 @@ # frozen_string_literal: true RSpec.describe Jobs::PeriodicalUpdates do + fab!(:admin) + before do + UserAvatar.where(last_gravatar_download_attempt: nil).update_all( + last_gravatar_download_attempt: Time.now, + ) + end + it "works" do # does not blow up, no mocks, everything is called Jobs::PeriodicalUpdates.new.execute(nil) @@ -8,7 +15,7 @@ RSpec.describe Jobs::PeriodicalUpdates do it "can rebake old posts when automatically_download_gravatars is false" do SiteSetting.automatically_download_gravatars = false - post = create_post + post = create_post(user: admin) post.update_columns(baked_at: Time.new(2000, 1, 1), baked_version: -1) Sidekiq::Testing.fake! do @@ -31,4 +38,52 @@ RSpec.describe Jobs::PeriodicalUpdates do post.reload expect(post.baked_at).to eq_time(baked) end + + it "does not rebake old posts when automatically_download_gravatars is true and a valid user avatar needs updating" do + SiteSetting.automatically_download_gravatars = true + UserAvatar.last.update!(last_gravatar_download_attempt: nil) + post = create_post(user: admin) + post.update_columns(baked_at: Time.new(2000, 1, 1), baked_version: -1) + + Sidekiq::Testing.fake! do + Jobs::ProcessPost.jobs.clear + Jobs::PeriodicalUpdates.new.execute + expect(Jobs::ProcessPost.jobs).to be_empty + end + end + + it "does not rebake old posts when there are user avatars that need updating" do + SiteSetting.automatically_download_gravatars = true + + post = create_post(user: admin) + post.update_columns(baked_at: Time.new(2000, 1, 1), baked_version: -1) + UserAvatar.last.update!(last_gravatar_download_attempt: nil) + + Sidekiq::Testing.fake! do + Jobs::ProcessPost.jobs.clear + Jobs::PeriodicalUpdates.new.execute + expect(Jobs::ProcessPost.jobs).to be_empty + end + end + + # inconsistent data will be fixed by ensure_consistency! of the relevant models + it "rebakes old posts when there are user avatars that need updating but have inconsistent data" do + SiteSetting.automatically_download_gravatars = true + + user_avatar_without_user = Fabricate(:user_avatar, last_gravatar_download_attempt: Time.now) + user_avatar_without_user.user.delete + user_without_any_email = Fabricate(:user) + user_without_any_email.user_emails.delete_all + user_without_primary_email = Fabricate(:user) + user_without_primary_email.primary_email.update_column(:primary, false) + + post = create_post(user: admin) + post.update_columns(baked_at: Time.new(2000, 1, 1), baked_version: -1) + + Sidekiq::Testing.fake! do + Jobs::ProcessPost.jobs.clear + Jobs::PeriodicalUpdates.new.execute + expect(Jobs::ProcessPost.jobs.length).to eq(1) + end + end end diff --git a/spec/models/user_avatar_spec.rb b/spec/models/user_avatar_spec.rb index 56d4e7ae287..88f5ed46afd 100644 --- a/spec/models/user_avatar_spec.rb +++ b/spec/models/user_avatar_spec.rb @@ -234,5 +234,16 @@ RSpec.describe UserAvatar do expect(user_avatar.gravatar_upload_id).to eq(nil) expect(user_avatar.custom_upload_id).to eq(nil) end + + it "deletes avatars without users and does not remove avatars with users" do + user_avatar_with_user = Fabricate(:user_avatar) + user_avatar_without_user = Fabricate(:user_avatar) + user_avatar_without_user.user.delete + + UserAvatar.ensure_consistency! + + expect(UserAvatar.exists?(user_avatar_with_user.id)).to eq true + expect(UserAvatar.exists?(user_avatar_without_user.id)).to eq false + end end end diff --git a/spec/models/user_email_spec.rb b/spec/models/user_email_spec.rb index 24014b4afd6..44a4d379825 100644 --- a/spec/models/user_email_spec.rb +++ b/spec/models/user_email_spec.rb @@ -63,4 +63,26 @@ RSpec.describe UserEmail do expect(user.user_emails.count).to eq 3 end end + + describe ".ensure_consistency!" do + context "when some users have no primary emails" do + it "creates primary emails for the users without a primary email" do + user_with_primary_email = Fabricate(:user) + user_without_primary_email = Fabricate(:user) + user_without_any_email = Fabricate(:user) + + user_without_primary_email.primary_email.update_column(:primary, false) + user_without_any_email.user_emails.delete_all + original_email_of_user_with_primary_email = user_with_primary_email.primary_email.email + + described_class.ensure_consistency! + + expect(user_without_primary_email.reload.primary_email).to be_present + expect(user_without_any_email.reload.primary_email).to be_present + expect( + user_with_primary_email.reload.primary_email.email, + ).to eq original_email_of_user_with_primary_email + end + end + end end