Match GitHub/GitLab records by their unique webhook URL

GitLab's per-project iid repeats across repositories, so matching pull
requests, merge requests and issues by id linked webhooks to the wrong
work packages. Resolve every lookup by the globally unique webhook URL
and enforce it with a database index. This also lets partial pull
requests created from issue comments be promoted in place rather than
duplicated.

Co-authored-by: Aleksey Kurepin <lesha@kurepin.com>

Author: akabiru

db/migrate/20260608120000_add_unique_index_to_integration_html_urls.rb

@@ -0,0 +1,109 @@
+# frozen_string_literal: true
+
+# The webhook URL is the only globally unique identifier for a pull request,
+# merge request or issue: per-project sequential numbers (GitLab's iid) repeat
+# across repositories. Enforce that uniqueness at the database level so a
+# missed lookup site or a concurrent webhook can never split one entity across
+# two rows. Surviving duplicates are folded into the most recent row first;
+# none are expected, since every write path already keys on the URL.
+class AddUniqueIndexToIntegrationHtmlUrls < ActiveRecord::Migration[8.1]
+  ENTITIES = {
+    "github_pull_requests" => {
+      url_column: "github_html_url",
+      work_packages_join: { table: "github_pull_requests_work_packages", foreign_key: "github_pull_request_id" },
+      children: { "github_check_runs" => "github_pull_request_id", "deploy_status_checks" => "github_pull_request_id" }
+    },
+    "gitlab_issues" => {
+      url_column: "gitlab_html_url",
+      work_packages_join: { table: "gitlab_issues_work_packages", foreign_key: "gitlab_issue_id" },
+      children: {}
+    },
+    "gitlab_merge_requests" => {
+      url_column: "gitlab_html_url",
+      work_packages_join: { table: "gitlab_merge_requests_work_packages", foreign_key: "gitlab_merge_request_id" },
+      children: { "gitlab_pipelines" => "gitlab_merge_request_id" }
+    }
+  }.freeze
+
+  def up
+    ENTITIES.each do |table, config|
+      say_with_time "Deduplicating #{table} by #{config[:url_column]}" do
+        fold_duplicates(table, config)
+      end
+      add_index table, config[:url_column], unique: true
+    end
+  end
+
+  def down
+    ENTITIES.each do |table, config|
+      remove_index table, config[:url_column]
+    end
+  end
+
+  private
+
+  def fold_duplicates(table, config)
+    move_work_packages_to_survivor(table, config)
+    repoint_children(table, config)
+    delete_duplicates(table, config[:url_column])
+  end
+
+  # Move the work packages of doomed rows onto the survivor, skipping links it already has.
+  def move_work_packages_to_survivor(table, config)
+    join_table = config[:work_packages_join][:table]
+    foreign_key = config[:work_packages_join][:foreign_key]
+    duplicates = duplicates_sql(table, config[:url_column])
+
+    execute <<~SQL.squish
+      INSERT INTO #{join_table} (#{foreign_key}, work_package_id)
+      SELECT DISTINCT duplicates.keep_id, links.work_package_id
+      FROM #{join_table} links
+      JOIN (#{duplicates}) duplicates
+        ON duplicates.dup_id = links.#{foreign_key}
+      WHERE NOT EXISTS (
+        SELECT 1 FROM #{join_table} existing
+        WHERE existing.#{foreign_key} = duplicates.keep_id
+          AND existing.work_package_id = links.work_package_id
+      )
+    SQL
+    execute <<~SQL.squish
+      DELETE FROM #{join_table} links
+      USING (#{duplicates}) duplicates
+      WHERE duplicates.dup_id = links.#{foreign_key}
+    SQL
+  end
+
+  def repoint_children(table, config)
+    config[:children].each do |child_table, foreign_key|
+      execute <<~SQL.squish
+        UPDATE #{child_table} child
+        SET #{foreign_key} = duplicates.keep_id
+        FROM (#{duplicates_sql(table, config[:url_column])}) duplicates
+        WHERE duplicates.dup_id = child.#{foreign_key}
+      SQL
+    end
+  end
+
+  def delete_duplicates(table, url_column)
+    execute <<~SQL.squish
+      DELETE FROM #{table} target
+      USING (#{duplicates_sql(table, url_column)}) duplicates
+      WHERE duplicates.dup_id = target.id
+    SQL
+  end
+
+  # Maps every duplicate row to the surviving (highest id) row sharing its URL.
+  def duplicates_sql(table, url_column)
+    <<~SQL.squish
+      SELECT row.id AS dup_id, survivors.keep_id
+      FROM #{table} row
+      JOIN (
+        SELECT #{url_column} AS url, MAX(id) AS keep_id
+        FROM #{table}
+        GROUP BY #{url_column}
+        HAVING COUNT(*) > 1
+      ) survivors ON survivors.url = row.#{url_column}
+      WHERE row.id <> survivors.keep_id
+    SQL
+  end
+end

modules/github_integration/app/models/github_pull_request.rb

@@ -60,16 +60,11 @@ class GithubPullRequest < ApplicationRecord
 
   scope :without_work_package, -> { where.missing(:work_packages) }
 
-  def self.find_by_github_identifiers(id: nil, url: nil, initialize: false)
-    raise ArgumentError, "needs an id and an url" if id.nil? || url.blank?
+  def self.find_by_github_identifiers(url:, id: nil, initialize: false)
+    raise ArgumentError, "needs an url" if url.blank?
 
-    found = find_by(github_id: id, github_html_url: url)
-
-    if found
-      found
-    elsif initialize
-      new(github_id: id, github_html_url: url)
-    end
+    found = find_by(github_html_url: url)
+    found || (new(github_id: id, github_html_url: url) if initialize)
   end
 
   def visible?(user = User.current)

modules/github_integration/lib/open_project/github_integration/notification_handler/pull_request.rb

@@ -73,8 +73,7 @@ def work_packages_to_comment_on(action, work_packages, already_referenced)
       end
 
       def already_referenced_work_packages(payload)
-        pull_request = GithubPullRequest.find_by(github_id: payload.pull_request.id,
-                                                 github_html_url: payload.pull_request.html_url)
+        pull_request = GithubPullRequest.find_by(github_html_url: payload.pull_request.html_url)
 
         pull_request&.work_packages.to_a
       end

modules/github_integration/spec/lib/open_project/github_integration/notification_handler/issue_comment_spec.rb

@@ -94,10 +94,10 @@
       process
       expect(upsert_partial_pull_request_service)
         .to have_received(:call) do |received_payload, work_packages:|
-          expect(received_payload.to_h)
-            .to eql payload
-          expect(work_packages)
-            .to contain_exactly(work_package)
+        expect(received_payload.to_h)
+          .to eql payload
+        expect(work_packages)
+          .to contain_exactly(work_package)
       end
     end
   end

modules/github_integration/spec/lib/open_project/github_integration/notification_handler/pull_request_spec.rb

@@ -209,7 +209,6 @@
       let!(:other_work_package) { create(:work_package) }
       let(:pr_body) { "Mentioning OP##{work_package.id} and OP##{other_work_package.id}" }
 
-      # NOTE: strange behavior, needs to clarification. it received two work_packages instead one.
       it "adds a comment only for the other_work_package" do
         process
         expect(handler_instance).to have_received(:comment_on_referenced_work_packages).with(

modules/github_integration/spec/lib/open_project/github_integration/services/upsert_pull_request_spec.rb

@@ -140,8 +140,19 @@
              state: "closed")
     end
 
-    it "doesn't update the github pull request" do
-      expect { upsert }.not_to change(github_pull_request, :reload)
+    it "promotes the partial pull request in place instead of creating a duplicate" do
+      expect { upsert }.to change { github_pull_request.reload.state }.from("closed").to("open")
+
+      expect(GithubPullRequest.where(github_html_url:).count).to eq(1)
+      expect(github_pull_request).to have_attributes(
+        github_id:,
+        number: 5,
+        title: "The PR title",
+        body: "The PR body",
+        github_html_url:,
+        github_updated_at: Time.zone.parse("20210409T12:13:14Z"),
+        repository: "test_user/repo"
+      )
     end
   end
 

modules/github_integration/spec/models/github_pull_request_spec.rb

@@ -91,68 +91,35 @@
              github_html_url: github_url)
     end
 
-    context "when the github_id attribute matches" do
-      it "finds by github_id" do
-        expect(described_class.find_by_github_identifiers(id: pull_request.github_id, url: github_url))
-          .to eql pull_request
-      end
-    end
-
-    context "when the github_html_url attribute matches" do
-      it "finds by github_html_url" do
-        expect(described_class.find_by_github_identifiers(id: pull_request.github_id, url: pull_request.github_html_url))
-          .to eql pull_request
-      end
+    it "raises an ArgumentError when no url is provided" do
+      expect { described_class.find_by_github_identifiers(id: github_id, url: nil) }
+        .to raise_error(ArgumentError, "needs an url")
     end
 
-    context "when the provided github_id does not match" do
-      it "returns nothing" do
+    context "when the url matches" do
+      it "returns the pull request regardless of the id" do
         expect(described_class.find_by_github_identifiers(id: pull_request.github_id + 1, url: github_url))
-          .to be_nil
-      end
-    end
-
-    context "when the provided github_html_url does not match" do
-      it "returns nothing" do
-        expect(described_class.find_by_github_identifiers(id: pull_request.github_id, url: "#{pull_request.github_html_url}zzzz"))
-          .to be_nil
+          .to eql pull_request
       end
     end
 
-    context "when neither match" do
-      it "returns nothing" do
-        expect(described_class.find_by_github_identifiers(id: pull_request.github_id + 1,
+    context "when the url does not match" do
+      it "returns nothing even when the id matches" do
+        expect(described_class.find_by_github_identifiers(id: pull_request.github_id,
                                                           url: "#{pull_request.github_html_url}zzzz"))
           .to be_nil
       end
     end
 
-    context "when the provided github_html_url does match but the github_id does not" do
-      it "returns nothing" do
-        expect(described_class.find_by_github_identifiers(id: pull_request.github_id + 1,
-                                                          url: pull_request.github_html_url))
-          .to be_nil
-      end
-    end
-
-    context "when neither match but initialize is true" do
+    context "when the url does not match but initialize is true" do
       subject(:finder) do
         described_class.find_by_github_identifiers(id: pull_request.github_id + 1,
                                                    url: "#{pull_request.github_html_url}zzzz",
                                                    initialize: true)
       end
 
-      it "returns a pull reqeust" do
-        expect(finder)
-          .to be_a(described_class)
-      end
-
-      it "returns a new record" do
-        expect(finder)
-          .to be_new_record
-      end
-
-      it "has the provided attributes initialized" do
+      it "returns a new record with the provided attributes" do
+        expect(finder).to be_a(described_class).and be_new_record
         expect(finder.attributes.compact)
           .to eql("github_id" => pull_request.github_id + 1,
                   "github_html_url" => "#{pull_request.github_html_url}zzzz")

modules/gitlab_integration/app/models/gitlab_issue.rb

@@ -52,16 +52,11 @@ class GitlabIssue < ApplicationRecord
 
   scope :without_work_package, -> { where.missing(:work_packages) }
 
-  def self.find_by_gitlab_identifiers(id: nil, url: nil, initialize: false)
-    raise ArgumentError, "needs an id and an url" if id.nil? || url.blank?
+  def self.find_by_gitlab_identifiers(url:, id: nil, initialize: false)
+    raise ArgumentError, "needs an url" if url.blank?
 
-    found = find_by(gitlab_id: id, gitlab_html_url: url)
-
-    if found
-      found
-    elsif initialize
-      new(gitlab_id: id, gitlab_html_url: url)
-    end
+    found = find_by(gitlab_html_url: url)
+    found || (new(gitlab_id: id, gitlab_html_url: url) if initialize)
   end
 
   def partial?

modules/gitlab_integration/app/models/gitlab_merge_request.rb

@@ -55,16 +55,11 @@ class GitlabMergeRequest < ApplicationRecord
 
   scope :without_work_package, -> { where.missing(:work_packages) }
 
-  def self.find_by_gitlab_identifiers(id: nil, url: nil, initialize: false)
-    raise ArgumentError, "needs an id and an url" if id.nil? || url.blank?
+  def self.find_by_gitlab_identifiers(url:, id: nil, initialize: false)
+    raise ArgumentError, "needs an url" if url.blank?
 
-    found = find_by(gitlab_id: id, gitlab_html_url: url)
-
-    if found
-      found
-    elsif initialize
-      new(gitlab_id: id, gitlab_html_url: url)
-    end
+    found = find_by(gitlab_html_url: url)
+    found || (new(gitlab_id: id, gitlab_html_url: url) if initialize)
   end
 
   ##

modules/gitlab_integration/lib/open_project/gitlab_integration/notification_handler/issue_hook.rb

@@ -68,8 +68,7 @@ def generate_notes(_payload)
       def gitlab_issue
         return @gitlab_issue if defined?(@gitlab_issue)
 
-        @gitlab_issue = GitlabIssue.find_by(gitlab_id: payload.object_attributes.iid,
-                                            gitlab_html_url: payload.object_attributes.url)
+        @gitlab_issue = GitlabIssue.find_by(gitlab_html_url: payload.object_attributes.url)
       end
 
       def upsert_issue(work_packages)