Merge pull request #23614 from opf/bug/stc-652-community-contribution-github-gitlab-fix-incorrect-linking-of-mr-pr-to-work-packages

Bug/STC-652: Match GitHub/GitLab records by webhook URL to fix incorrect MR/PR linking

Author: akabiru

db/migrate/20260608120000_add_unique_index_to_integration_html_urls.rb

@@ -0,0 +1,112 @@
+# frozen_string_literal: true
+
+# The webhook URL is the only globally unique identifier stored for a pull
+# request, merge request or issue: GitLab records hold only the per-project
+# iid, which repeats across repositories, and GitHub records created from
+# comment payloads carry no github_id at all. Enforce the URL's 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]
+  disable_ddl_transaction!
+
+  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
+        transaction { fold_duplicates(table, config) }
+      end
+      add_index table, config[:url_column], unique: true, algorithm: :concurrently, if_not_exists: true
+    end
+  end
+
+  def down
+    ENTITIES.each do |table, config|
+      remove_index table, config[:url_column], algorithm: :concurrently, if_exists: true
+    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

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) the OpenProject GmbH
@@ -41,33 +43,33 @@ class GithubPullRequest < ApplicationRecord
     deployed: "deployed"
   }
 
-  validates_presence_of :github_html_url,
-                        :number,
-                        :repository,
-                        :state,
-                        :title,
-                        :github_updated_at
-  validates_presence_of :body,
-                        :comments_count,
-                        :review_comments_count,
-                        :additions_count,
-                        :deletions_count,
-                        :changed_files_count,
-                        unless: :partial?
+  validates :github_html_url,
+            :number,
+            :repository,
+            :state,
+            :title,
+            :github_updated_at,
+            presence: true
+  validates :body,
+            :comments_count,
+            :review_comments_count,
+            :additions_count,
+            :deletions_count,
+            :changed_files_count,
+            presence: { unless: :partial? }
   validate :validate_labels_schema
 
   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 or an url" if id.nil? && url.blank?
-
-    found = where(github_id: id).or(where(github_html_url: url)).take
+  def self.find_by_github_identifiers(url:, id: nil, initialize: false)
+    raise ArgumentError, "needs an url" if url.blank?
 
-    if found
-      found
-    elsif initialize
-      new(github_id: id, github_html_url: url)
-    end
+    # The URL matches across every payload shape (comment payloads carry no
+    # pull request id). The id fallback covers repository renames: GitHub ids
+    # are globally unique, so a URL miss with an id hit is the same pull
+    # request under a new URL, which the next upsert writes back.
+    found = find_by(github_html_url: url) || (find_by(github_id: id) if id)
+    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

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) the OpenProject GmbH
@@ -71,12 +73,9 @@ def work_packages_to_comment_on(action, work_packages, already_referenced)
       end
 
       def already_referenced_work_packages(payload)
-        pull_request = GithubPullRequest
-          .where(github_id: payload.pull_request.id)
-          .or(GithubPullRequest.where(github_html_url: payload.pull_request.html_url))
-          .take
+        pull_request = GithubPullRequest.find_by(github_html_url: payload.pull_request.html_url)
 
-        pull_request&.work_packages.to_a || []
+        pull_request&.work_packages.to_a
       end
 
       def upsert_pull_request(payload, work_packages)

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

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) the OpenProject GmbH

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

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) the OpenProject GmbH
@@ -40,11 +42,11 @@
       "action" => action,
       "open_project_user_id" => github_system_user.id,
       "pull_request" => {
-        "id" => 123,
+        "id" => github_id,
         "number" => 1,
         "body" => pr_body,
         "title" => "A PR title",
-        "html_url" => "http://pr.url",
+        "html_url" => github_html_url,
         "updated_at" => Time.current.iso8601,
         "state" => "open",
         "draft" => pr_draft,
@@ -81,7 +83,8 @@
   let(:pr_body) { "Mentioning OP##{work_package.id}" }
   let(:pr_merged) { false }
   let(:pr_draft) { false }
-  let(:github_pull_request) { GithubPullRequest.find_by_github_identifiers id: 123 }
+  let(:github_id) { 123 }
+  let(:github_html_url) { "https://github.com/test_user/repo" }
 
   before do
     allow(handler_instance).to receive(:comment_on_referenced_work_packages).and_return(nil)
@@ -130,8 +133,8 @@
   context "with a closed action" do
     let(:action) { "closed" }
     let(:comment) do
-      %(<macro class="github_pull_request" data-pull-request-id="#{github_pull_request.id}"
-        data-pull-request-state="&quot;closed&quot;"></macro>).squish
+      %(<macro class="github_pull_request" data-pull-request-id="#{GithubPullRequest.last.id}"
+      data-pull-request-state="&quot;closed&quot;"></macro>).squish
     end
 
     it_behaves_like "adding a comment"
@@ -142,15 +145,15 @@
     let(:action) { "closed" }
     let(:pr_merged) { true }
     let(:comment) do
-      %(<macro class="github_pull_request" data-pull-request-id="#{github_pull_request.id}"
-        data-pull-request-state="&quot;merged&quot;"></macro>).squish
+      %(<macro class="github_pull_request" data-pull-request-id="#{GithubPullRequest.last.id}"
+      data-pull-request-state="&quot;merged&quot;"></macro>).squish
     end
 
     it_behaves_like "adding a comment"
     it_behaves_like "calls the pull request upsert service"
 
     context "when the work package is already known to the GithubPullRequest" do
-      let!(:github_pull_request) { create(:github_pull_request, github_id: 123, work_packages: [work_package]) }
+      let!(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:, work_packages: [work_package]) }
 
       it_behaves_like "adding a comment"
 
@@ -170,16 +173,17 @@
 
   context "with an edited action" do
     let(:action) { "edited" }
+    let(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:) }
     let(:comment) do
-      %(<macro class="github_pull_request" data-pull-request-id="#{github_pull_request.id}"
+      %(<macro class="github_pull_request" data-pull-request-id="#{GithubPullRequest.last.id}"
         data-pull-request-state="&quot;referenced&quot;"></macro>).squish
     end
 
     it_behaves_like "adding a comment"
     it_behaves_like "calls the pull request upsert service"
 
     context "when a GithubPullRequest exists that is not linked to the mentioned work package yet" do
-      let!(:github_pull_request) { create(:github_pull_request, github_id: 123) }
+      let!(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:) }
 
       it_behaves_like "adding a comment"
 
@@ -190,7 +194,7 @@
     end
 
     context "when the work package is already known to the GithubPullRequest" do
-      let!(:github_pull_request) { create(:github_pull_request, github_id: 123, work_packages: [work_package]) }
+      let!(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:, work_packages: [work_package]) }
 
       it_behaves_like "not adding a comment"
 
@@ -201,7 +205,7 @@
     end
 
     context "when the a work package is already known to the GithubPullRequest but another work package is new" do
-      let!(:github_pull_request) { create(:github_pull_request, github_id: 123, work_packages: [work_package]) }
+      let!(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:, work_packages: [work_package]) }
       let!(:other_work_package) { create(:work_package) }
       let(:pr_body) { "Mentioning OP##{work_package.id} and OP##{other_work_package.id}" }
 
@@ -231,8 +235,9 @@
 
   context "with an opened action" do
     let(:action) { "opened" }
+    let(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:) }
     let(:comment) do
-      %(<macro class="github_pull_request" data-pull-request-id="#{github_pull_request.id}"
+      %(<macro class="github_pull_request" data-pull-request-id="#{GithubPullRequest.last.id}"
         data-pull-request-state="&quot;opened&quot;"></macro>).squish
     end
 
@@ -243,8 +248,9 @@
   context "with an opened action when the PR is a draft" do
     let(:action) { "opened" }
     let(:pr_draft) { true }
+    let(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:) }
     let(:comment) do
-      %(<macro class="github_pull_request" data-pull-request-id="#{github_pull_request.id}"
+      %(<macro class="github_pull_request" data-pull-request-id="#{GithubPullRequest.last.id}"
         data-pull-request-state="&quot;opened&quot;"></macro>).squish
     end
 
@@ -254,8 +260,9 @@
 
   context "with a ready_for_review action" do
     let(:action) { "ready_for_review" }
+    let(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:) }
     let(:comment) do
-      %(<macro class="github_pull_request" data-pull-request-id="#{github_pull_request.id}"
+      %(<macro class="github_pull_request" data-pull-request-id="#{GithubPullRequest.last.id}"
         data-pull-request-state="&quot;ready_for_review&quot;"></macro>).squish
     end
 
@@ -265,8 +272,9 @@
 
   context "with a reopened action" do
     let(:action) { "reopened" }
+    let(:github_pull_request) { create(:github_pull_request, github_id:, github_html_url:) }
     let(:comment) do
-      %(<macro class="github_pull_request" data-pull-request-id="#{github_pull_request.id}"
+      %(<macro class="github_pull_request" data-pull-request-id="#{GithubPullRequest.last.id}"
         data-pull-request-state="&quot;opened&quot;"></macro>).squish
     end