From 93625ef07ccf02167a8a883c6d422d35078895ef Mon Sep 17 00:00:00 2001
From: Ted Johansson <ted@discourse.org>
Date: Mon, 21 Oct 2024 10:45:09 +0800
Subject: [PATCH] FIX: Make problem check tracker unique constraint work on
 PG13 (#29272)

In #29169 we added a NULLS NOT DISTINCT option to the unique index on problem_check_trackers. This is to enforce uniqueness even when the target is NULL. (Postgres considers all NULLs to be distinct by default.)

However, this only works in PG15. In PG13 it does nothing.

This commit adds a default dummy string value __NULL__ to target. Since it's a string, PG13 will be able to correctly identify duplicate records.
---
 app/models/problem_check.rb                   |  8 +++++--
 app/models/problem_check_tracker.rb           | 10 +++------
 ..._value_to_problem_check_trackers_target.rb | 22 +++++++++++++++++++
 spec/models/problem_check_tracker_spec.rb     |  3 +--
 4 files changed, 32 insertions(+), 11 deletions(-)
 create mode 100644 db/migrate/20241018031851_add_default_value_to_problem_check_trackers_target.rb

diff --git a/app/models/problem_check.rb b/app/models/problem_check.rb
index bc6a7aa37f7..4235ea0759c 100644
--- a/app/models/problem_check.rb
+++ b/app/models/problem_check.rb
@@ -90,6 +90,10 @@ class ProblemCheck
     ProblemCheck::WatchedWords,
   ].freeze
 
+  # To enforce the unique constraint in Postgres <15 we need a dummy
+  # value, since the index considers NULLs to be distinct.
+  NO_TARGET = "__NULL__"
+
   def self.[](key)
     key = key.to_sym
 
@@ -176,12 +180,12 @@ class ProblemCheck
 
   private
 
-  def tracker(target = nil)
+  def tracker(target = NO_TARGET)
     ProblemCheckTracker[identifier, target]
   end
 
   def targets
-    [nil]
+    [NO_TARGET]
   end
 
   def problem(override_key: nil, override_data: {})
diff --git a/app/models/problem_check_tracker.rb b/app/models/problem_check_tracker.rb
index b9ef0054167..4587c296ed4 100644
--- a/app/models/problem_check_tracker.rb
+++ b/app/models/problem_check_tracker.rb
@@ -7,7 +7,7 @@ class ProblemCheckTracker < ActiveRecord::Base
   scope :failing, -> { where("last_problem_at = last_run_at") }
   scope :passing, -> { where("last_success_at = last_run_at") }
 
-  def self.[](identifier, target = nil)
+  def self.[](identifier, target = ProblemCheck::NO_TARGET)
     find_or_create_by(identifier:, target:)
   end
 
@@ -71,11 +71,7 @@ class ProblemCheckTracker < ActiveRecord::Base
   end
 
   def admin_notice
-    if target.present?
-      AdminNotice.problem.where("details->>'target' = ?", target)
-    else
-      AdminNotice.problem.where("(details->>'target') IS NULL")
-    end
+    AdminNotice.problem.where("details->>'target' = ?", target || ProblemCheck::NO_TARGET)
   end
 end
 
@@ -91,7 +87,7 @@ end
 #  last_success_at :datetime
 #  last_problem_at :datetime
 #  details         :json
-#  target          :string
+#  target          :string           default("__NULL__")
 #
 # Indexes
 #
diff --git a/db/migrate/20241018031851_add_default_value_to_problem_check_trackers_target.rb b/db/migrate/20241018031851_add_default_value_to_problem_check_trackers_target.rb
new file mode 100644
index 00000000000..486057f8529
--- /dev/null
+++ b/db/migrate/20241018031851_add_default_value_to_problem_check_trackers_target.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+class AddDefaultValueToProblemCheckTrackersTarget < ActiveRecord::Migration[7.1]
+  def up
+    change_column_default :problem_check_trackers, :target, "__NULL__"
+
+    execute(<<~SQL)
+      UPDATE problem_check_trackers
+      SET target='__NULL__'
+      WHERE target IS NULL
+    SQL
+  end
+
+  def down
+    change_column_default :problem_check_trackers, :target, nil
+
+    execute(<<~SQL)
+      UPDATE problem_check_trackers
+      SET target = NULL
+      WHERE target = '__NULL__'
+    SQL
+  end
+end
diff --git a/spec/models/problem_check_tracker_spec.rb b/spec/models/problem_check_tracker_spec.rb
index 9b07d47ecb4..dadeca7bb6f 100644
--- a/spec/models/problem_check_tracker_spec.rb
+++ b/spec/models/problem_check_tracker_spec.rb
@@ -187,7 +187,6 @@ RSpec.describe ProblemCheckTracker do
       before { ProblemCheck::TwitterLogin.stubs(:max_blips).returns(1) }
 
       it "does not sound the alarm" do
-        puts ProblemCheck::TwitterLogin.max_blips
         expect { problem_tracker.problem!(next_run_at: 24.hours.from_now) }.not_to change {
           AdminNotice.problem.count
         }
@@ -221,7 +220,7 @@ RSpec.describe ProblemCheckTracker do
     end
 
     context "when there's an alarm sounding" do
-      before { Fabricate(:admin_notice, subject: "problem", identifier: "twitter_login") }
+      before { problem_tracker.problem! }
 
       it "silences the alarm" do
         expect { problem_tracker.no_problem!(next_run_at: 24.hours.from_now) }.to change {